From c84d70d8be096c2116cf00343a9bdcdf5296340b Mon Sep 17 00:00:00 2001 From: Steve Dignam Date: Fri, 20 Feb 2026 22:29:07 -0500 Subject: [PATCH 1/2] linter: warn about foreign key constraints in create table defs we still need to take locks on the related table --- crates/squawk_ide/src/binder.rs | 2 +- crates/squawk_ide/src/document_symbols.rs | 2 +- crates/squawk_linter/src/analyze.rs | 7 +- .../rules/adding_foreign_key_constraint.rs | 166 ++++++++++++------ .../src/rules/adding_not_null_field.rs | 2 +- ...__create_table_with_column_references.snap | 10 ++ ...ate_table_with_foreign_key_constraint.snap | 12 ++ .../squawk_syntax/src/ast/generated/nodes.rs | 4 +- crates/squawk_syntax/src/postgresql.ungram | 4 +- 9 files changed, 142 insertions(+), 67 deletions(-) create mode 100644 crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__adding_foreign_key_constraint__test__create_table_with_column_references.snap create mode 100644 crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__adding_foreign_key_constraint__test__create_table_with_foreign_key_constraint.snap diff --git a/crates/squawk_ide/src/binder.rs b/crates/squawk_ide/src/binder.rs index 7c23cafc..5bd1e770 100644 --- a/crates/squawk_ide/src/binder.rs +++ b/crates/squawk_ide/src/binder.rs @@ -281,7 +281,7 @@ fn bind_stmt(b: &mut Binder, stmt: ast::Stmt) { ast::Stmt::Listen(listen) => bind_listen(b, listen), ast::Stmt::Set(set) => bind_set(b, set), ast::Stmt::CreatePolicy(create_policy) => bind_create_policy(b, create_policy), - _ => {} + _ => (), } } diff --git a/crates/squawk_ide/src/document_symbols.rs b/crates/squawk_ide/src/document_symbols.rs index 974fe3df..6b98017b 100644 --- a/crates/squawk_ide/src/document_symbols.rs +++ b/crates/squawk_ide/src/document_symbols.rs @@ -201,7 +201,7 @@ pub fn document_symbols(file: &ast::SourceFile) -> Vec { } } - _ => {} + _ => (), } } diff --git a/crates/squawk_linter/src/analyze.rs b/crates/squawk_linter/src/analyze.rs index 6bf35a30..4c165725 100644 --- a/crates/squawk_linter/src/analyze.rs +++ b/crates/squawk_linter/src/analyze.rs @@ -8,13 +8,14 @@ fn has_foreign_key_constraint(create_table: &ast::CreateTable) -> bool { return true; } ast::TableArg::Column(column) => { - if let Some(ast::ColumnConstraint::ReferencesConstraint(_)) = - column.constraint() + if column + .constraints() + .any(|c| matches!(c, ast::ColumnConstraint::ReferencesConstraint(_))) { return true; } } - _ => {} + _ => (), } } } diff --git a/crates/squawk_linter/src/rules/adding_foreign_key_constraint.rs b/crates/squawk_linter/src/rules/adding_foreign_key_constraint.rs index 900600d7..e6423ba8 100644 --- a/crates/squawk_linter/src/rules/adding_foreign_key_constraint.rs +++ b/crates/squawk_linter/src/rules/adding_foreign_key_constraint.rs @@ -15,63 +15,103 @@ pub(crate) fn adding_foreign_key_constraint(ctx: &mut Linter, parse: &Parse { - if add_constraint.not_valid().is_some() - || tables_created.contains(&Identifier::new(&table_name.text())) - { - // Adding foreign key is okay when: - // - NOT VALID is specified. - // - The table is created in the same transaction - continue; + match stmt { + ast::Stmt::AlterTable(alter_table) => { + if let Some(table_name) = alter_table + .relation_name() + .and_then(|x| x.path()) + .and_then(|x| x.segment()) + .and_then(|x| x.name_ref()) + { + for action in alter_table.actions() { + match action { + ast::AlterTableAction::AddConstraint(add_constraint) => { + if add_constraint.not_valid().is_some() + || tables_created.contains(&Identifier::new(&table_name.text())) + { + // Adding foreign key is okay when: + // - NOT VALID is specified. + // - The table is created in the same transaction + continue; + } + if let Some(constraint) = add_constraint.constraint() { + if matches!( + constraint, + ast::Constraint::ForeignKeyConstraint(_) + | ast::Constraint::ReferencesConstraint(_) + ) { + ctx.report( + Violation::for_node( + Rule::AddingForeignKeyConstraint, + message.into(), + constraint.syntax(), + ) + .help(help), + ) + } + } } - if let Some(constraint) = add_constraint.constraint() { - if matches!( - constraint, - ast::Constraint::ForeignKeyConstraint(_) - | ast::Constraint::ReferencesConstraint(_) - ) { - ctx.report( - Violation::for_node( - Rule::AddingForeignKeyConstraint, - message.into(), - constraint.syntax(), + ast::AlterTableAction::AddColumn(add_column) => { + for constraint in add_column.constraints() { + if matches!( + constraint, + ast::Constraint::ForeignKeyConstraint(_) + | ast::Constraint::ReferencesConstraint(_) + ) { + ctx.report( + Violation::for_node( + Rule::AddingForeignKeyConstraint, + message.into(), + constraint.syntax(), + ) + .help(help), ) - .help(help), - ) + } } } + _ => continue, } - ast::AlterTableAction::AddColumn(add_column) => { - for constraint in add_column.constraints() { - if matches!( - constraint, - ast::Constraint::ForeignKeyConstraint(_) - | ast::Constraint::ReferencesConstraint(_) - ) { - ctx.report( - Violation::for_node( - Rule::AddingForeignKeyConstraint, - message.into(), - constraint.syntax(), - ) - .help(help), + } + } + } + ast::Stmt::CreateTable(create_table) => { + if let Some(table_arg_list) = create_table.table_arg_list() { + for arg in table_arg_list.args() { + match arg { + ast::TableArg::TableConstraint( + ast::TableConstraint::ForeignKeyConstraint(fk), + ) => { + ctx.report( + Violation::for_node( + Rule::AddingForeignKeyConstraint, + message.into(), + fk.syntax(), ) + .help(help), + ); + } + ast::TableArg::Column(column) => { + for constraint in column.constraints() { + if let ast::ColumnConstraint::ReferencesConstraint(refs) = + constraint + { + ctx.report( + Violation::for_node( + Rule::AddingForeignKeyConstraint, + message.into(), + refs.syntax(), + ) + .help(help), + ); + } } } + _ => () } - _ => continue, } } } + _ => () } } } @@ -90,20 +130,32 @@ mod test { #[test] fn create_table_with_foreign_key_constraint() { let sql = r#" - BEGIN; - CREATE TABLE email ( - id BIGINT GENERATED ALWAYS AS IDENTITY, - user_id BIGINT, - email TEXT, - PRIMARY KEY(id), - CONSTRAINT fk_user - FOREIGN KEY ("user_id") - REFERENCES "user" ("id") - ); - COMMIT; +BEGIN; +CREATE TABLE email ( + id BIGINT GENERATED ALWAYS AS IDENTITY, + user_id BIGINT, + email TEXT, + PRIMARY KEY(id), + CONSTRAINT fk_user + FOREIGN KEY ("user_id") + REFERENCES "user" ("id") +); +COMMIT; "#; - lint_ok(sql, Rule::AddingForeignKeyConstraint); + assert_snapshot!(lint_errors(sql, Rule::AddingForeignKeyConstraint)); + } + + #[test] + fn create_table_with_column_references() { + let sql = r#" +CREATE TABLE table_name ( + id BIGSERIAL NOT NULL, + my_fk_id INTEGER NOT NULL REFERENCES other_table (id) ON DELETE CASCADE +); + "#; + + assert_snapshot!(lint_errors(sql, Rule::AddingForeignKeyConstraint)); } #[test] diff --git a/crates/squawk_linter/src/rules/adding_not_null_field.rs b/crates/squawk_linter/src/rules/adding_not_null_field.rs index dd711e33..013a14f8 100644 --- a/crates/squawk_linter/src/rules/adding_not_null_field.rs +++ b/crates/squawk_linter/src/rules/adding_not_null_field.rs @@ -124,7 +124,7 @@ pub(crate) fn adding_not_null_field(ctx: &mut Linter, parse: &Parse) .help("Make the field nullable and use a `CHECK` constraint instead."), ); } - _ => {} + _ => () } } } diff --git a/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__adding_foreign_key_constraint__test__create_table_with_column_references.snap b/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__adding_foreign_key_constraint__test__create_table_with_column_references.snap new file mode 100644 index 00000000..99f84a8b --- /dev/null +++ b/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__adding_foreign_key_constraint__test__create_table_with_column_references.snap @@ -0,0 +1,10 @@ +--- +source: crates/squawk_linter/src/rules/adding_foreign_key_constraint.rs +expression: "lint_errors(sql, Rule::AddingForeignKeyConstraint)" +--- +warning[adding-foreign-key-constraint]: Adding a foreign key constraint requires a table scan and a `SHARE ROW EXCLUSIVE` lock on both tables, which blocks writes to each table. + ╭▸ +4 │ my_fk_id INTEGER NOT NULL REFERENCES other_table (id) ON DELETE CASCADE + │ ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + │ + ╰ help: Add `NOT VALID` to the constraint in one transaction and then VALIDATE the constraint in a separate transaction. diff --git a/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__adding_foreign_key_constraint__test__create_table_with_foreign_key_constraint.snap b/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__adding_foreign_key_constraint__test__create_table_with_foreign_key_constraint.snap new file mode 100644 index 00000000..2f9766d8 --- /dev/null +++ b/crates/squawk_linter/src/rules/snapshots/squawk_linter__rules__adding_foreign_key_constraint__test__create_table_with_foreign_key_constraint.snap @@ -0,0 +1,12 @@ +--- +source: crates/squawk_linter/src/rules/adding_foreign_key_constraint.rs +expression: "lint_errors(sql, Rule::AddingForeignKeyConstraint)" +--- +warning[adding-foreign-key-constraint]: Adding a foreign key constraint requires a table scan and a `SHARE ROW EXCLUSIVE` lock on both tables, which blocks writes to each table. + ╭▸ + 8 │ ┏ CONSTRAINT fk_user + 9 │ ┃ FOREIGN KEY ("user_id") +10 │ ┃ REFERENCES "user" ("id") + │ ┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┛ + │ + ╰ help: Add `NOT VALID` to the constraint in one transaction and then VALIDATE the constraint in a separate transaction. diff --git a/crates/squawk_syntax/src/ast/generated/nodes.rs b/crates/squawk_syntax/src/ast/generated/nodes.rs index 8ef2dc87..2238d440 100644 --- a/crates/squawk_syntax/src/ast/generated/nodes.rs +++ b/crates/squawk_syntax/src/ast/generated/nodes.rs @@ -2579,8 +2579,8 @@ impl Column { support::child(&self.syntax) } #[inline] - pub fn constraint(&self) -> Option { - support::child(&self.syntax) + pub fn constraints(&self) -> AstChildren { + support::children(&self.syntax) } #[inline] pub fn deferrable_constraint_option(&self) -> Option { diff --git a/crates/squawk_syntax/src/postgresql.ungram b/crates/squawk_syntax/src/postgresql.ungram index b6497bfe..0219a517 100644 --- a/crates/squawk_syntax/src/postgresql.ungram +++ b/crates/squawk_syntax/src/postgresql.ungram @@ -604,11 +604,11 @@ CompressionMethod = Column = 'period'? ( - Name WithOptions? constraint:ColumnConstraint? + Name WithOptions? constraints:ColumnConstraint* DeferrableConstraintOption? NotDeferrableConstraintOption? InitiallyDeferredConstraintOption? InitiallyImmediateConstraintOption? NotEnforced? Enforced? - | Name Type Storage? CompressionMethod? Collate? constraint:ColumnConstraint? + | Name Type Storage? CompressionMethod? Collate? constraints:ColumnConstraint* DeferrableConstraintOption? NotDeferrableConstraintOption? InitiallyDeferredConstraintOption? InitiallyImmediateConstraintOption? NotEnforced? Enforced? From 50388850e666f2e9d8af22cae801106b2e7bfeb8 Mon Sep 17 00:00:00 2001 From: Steve Dignam Date: Fri, 20 Feb 2026 22:35:40 -0500 Subject: [PATCH 2/2] lint --- .../squawk_linter/src/rules/adding_foreign_key_constraint.rs | 4 ++-- crates/squawk_linter/src/rules/adding_not_null_field.rs | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/squawk_linter/src/rules/adding_foreign_key_constraint.rs b/crates/squawk_linter/src/rules/adding_foreign_key_constraint.rs index e6423ba8..42c4c192 100644 --- a/crates/squawk_linter/src/rules/adding_foreign_key_constraint.rs +++ b/crates/squawk_linter/src/rules/adding_foreign_key_constraint.rs @@ -106,12 +106,12 @@ pub(crate) fn adding_foreign_key_constraint(ctx: &mut Linter, parse: &Parse () + _ => (), } } } } - _ => () + _ => (), } } } diff --git a/crates/squawk_linter/src/rules/adding_not_null_field.rs b/crates/squawk_linter/src/rules/adding_not_null_field.rs index 013a14f8..4cdb252b 100644 --- a/crates/squawk_linter/src/rules/adding_not_null_field.rs +++ b/crates/squawk_linter/src/rules/adding_not_null_field.rs @@ -124,7 +124,7 @@ pub(crate) fn adding_not_null_field(ctx: &mut Linter, parse: &Parse) .help("Make the field nullable and use a `CHECK` constraint instead."), ); } - _ => () + _ => (), } } }