diff --git a/Cargo.lock b/Cargo.lock index 75a71dd2..4f86003e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2533,6 +2533,7 @@ dependencies = [ "serde", "serde_plain", "squawk-syntax", + "tabled", ] [[package]] diff --git a/crates/squawk/src/reporter.rs b/crates/squawk/src/reporter.rs index bed2f778..bf3af4e6 100644 --- a/crates/squawk/src/reporter.rs +++ b/crates/squawk/src/reporter.rs @@ -543,8 +543,8 @@ SELECT 1; ); assert!(res.is_ok()); - assert_snapshot!(String::from_utf8_lossy(&buff), @r" - main.sql:1:3: warning: require-timeout-settings Missing `set lock_timeout` before potentially slow operations + assert_snapshot!(String::from_utf8_lossy(&buff), @" + main.sql:1:3: warning: require-timeout-settings Missing `set lock_timeout` before potentially slow ACCESS EXCLUSIVE lock operations main.sql:1:3: warning: require-timeout-settings Missing `set statement_timeout` before potentially slow operations main.sql:1:29: warning: adding-required-field Adding a new column that is `NOT NULL` and has no default value to an existing table effectively makes it required. main.sql:1:29: warning: prefer-robust-stmts Missing `IF NOT EXISTS`, the migration can't be rerun if it fails part way through. @@ -636,7 +636,7 @@ SELECT 1; ); assert!(res.is_ok()); - assert_snapshot!(String::from_utf8_lossy(&buff), @r#"[{"file":"main.sql","line":1,"column":3,"level":"Warning","message":"Missing `set lock_timeout` before potentially slow operations","help":"Configure a `lock_timeout` before this statement.","rule_name":"require-timeout-settings","column_end":63,"line_end":1},{"file":"main.sql","line":1,"column":3,"level":"Warning","message":"Missing `set statement_timeout` before potentially slow operations","help":"Configure a `statement_timeout` before this statement","rule_name":"require-timeout-settings","column_end":63,"line_end":1},{"file":"main.sql","line":1,"column":29,"level":"Warning","message":"Adding a new column that is `NOT NULL` and has no default value to an existing table effectively makes it required.","help":"Make the field nullable or add a non-VOLATILE DEFAULT","rule_name":"adding-required-field","column_end":62,"line_end":1},{"file":"main.sql","line":1,"column":29,"level":"Warning","message":"Missing `IF NOT EXISTS`, the migration can't be rerun if it fails part way through.","help":null,"rule_name":"prefer-robust-stmts","column_end":62,"line_end":1},{"file":"main.sql","line":1,"column":46,"level":"Warning","message":"Using 32-bit integer fields can result in hitting the max `int` limit.","help":"Use 64-bit integer values instead to prevent hitting this limit.","rule_name":"prefer-bigint-over-int","column_end":53,"line_end":1},{"file":"main.sql","line":2,"column":23,"level":"Warning","message":"Adding a new column that is `NOT NULL` and has no default value to an existing table effectively makes it required.","help":"Make the field nullable or add a non-VOLATILE DEFAULT","rule_name":"adding-required-field","column_end":56,"line_end":2},{"file":"main.sql","line":2,"column":23,"level":"Warning","message":"Missing `IF NOT EXISTS`, the migration can't be rerun if it fails part way through.","help":null,"rule_name":"prefer-robust-stmts","column_end":56,"line_end":2},{"file":"main.sql","line":2,"column":40,"level":"Warning","message":"Using 32-bit integer fields can result in hitting the max `int` limit.","help":"Use 64-bit integer values instead to prevent hitting this limit.","rule_name":"prefer-bigint-over-int","column_end":47,"line_end":2}]"#); + assert_snapshot!(String::from_utf8_lossy(&buff), @r#"[{"file":"main.sql","line":1,"column":3,"level":"Warning","message":"Missing `set lock_timeout` before potentially slow ACCESS EXCLUSIVE lock operations","help":"Configure a `lock_timeout` before this statement. Statement requires: ACCESS EXCLUSIVE lock; blocking: reads, writes, schema changes.","rule_name":"require-timeout-settings","column_end":63,"line_end":1},{"file":"main.sql","line":1,"column":3,"level":"Warning","message":"Missing `set statement_timeout` before potentially slow operations","help":"Configure a `statement_timeout` before this statement","rule_name":"require-timeout-settings","column_end":63,"line_end":1},{"file":"main.sql","line":1,"column":29,"level":"Warning","message":"Adding a new column that is `NOT NULL` and has no default value to an existing table effectively makes it required.","help":"Make the field nullable or add a non-VOLATILE DEFAULT","rule_name":"adding-required-field","column_end":62,"line_end":1},{"file":"main.sql","line":1,"column":29,"level":"Warning","message":"Missing `IF NOT EXISTS`, the migration can't be rerun if it fails part way through.","help":null,"rule_name":"prefer-robust-stmts","column_end":62,"line_end":1},{"file":"main.sql","line":1,"column":46,"level":"Warning","message":"Using 32-bit integer fields can result in hitting the max `int` limit.","help":"Use 64-bit integer values instead to prevent hitting this limit.","rule_name":"prefer-bigint-over-int","column_end":53,"line_end":1},{"file":"main.sql","line":2,"column":23,"level":"Warning","message":"Adding a new column that is `NOT NULL` and has no default value to an existing table effectively makes it required.","help":"Make the field nullable or add a non-VOLATILE DEFAULT","rule_name":"adding-required-field","column_end":56,"line_end":2},{"file":"main.sql","line":2,"column":23,"level":"Warning","message":"Missing `IF NOT EXISTS`, the migration can't be rerun if it fails part way through.","help":null,"rule_name":"prefer-robust-stmts","column_end":56,"line_end":2},{"file":"main.sql","line":2,"column":40,"level":"Warning","message":"Using 32-bit integer fields can result in hitting the max `int` limit.","help":"Use 64-bit integer values instead to prevent hitting this limit.","rule_name":"prefer-bigint-over-int","column_end":47,"line_end":2}]"#); } #[test] @@ -658,7 +658,7 @@ SELECT 1; assert!(res.is_ok()); - assert_snapshot!(String::from_utf8_lossy(&buff), @r#"[{"description":"Missing `set lock_timeout` before potentially slow operations Suggestion: Configure a `lock_timeout` before this statement.","severity":"minor","fingerprint":"106496a54f0e1a20","location":{"path":"main.sql","lines":{"begin":1,"end":1}},"check_name":"require-timeout-settings"},{"description":"Missing `set statement_timeout` before potentially slow operations Suggestion: Configure a `statement_timeout` before this statement","severity":"minor","fingerprint":"2904632bf27251d2","location":{"path":"main.sql","lines":{"begin":1,"end":1}},"check_name":"require-timeout-settings"},{"description":"Adding a new column that is `NOT NULL` and has no default value to an existing table effectively makes it required. Suggestion: Make the field nullable or add a non-VOLATILE DEFAULT","severity":"minor","fingerprint":"87fbb54d93cdb8c9","location":{"path":"main.sql","lines":{"begin":1,"end":1}},"check_name":"adding-required-field"},{"description":"Missing `IF NOT EXISTS`, the migration can't be rerun if it fails part way through.","severity":"minor","fingerprint":"21df0ee3817ad84","location":{"path":"main.sql","lines":{"begin":1,"end":1}},"check_name":"prefer-robust-stmts"},{"description":"Using 32-bit integer fields can result in hitting the max `int` limit. Suggestion: Use 64-bit integer values instead to prevent hitting this limit.","severity":"minor","fingerprint":"3d0e81dc13bc8757","location":{"path":"main.sql","lines":{"begin":1,"end":1}},"check_name":"prefer-bigint-over-int"},{"description":"Adding a new column that is `NOT NULL` and has no default value to an existing table effectively makes it required. Suggestion: Make the field nullable or add a non-VOLATILE DEFAULT","severity":"minor","fingerprint":"4bdd655ad8e102ad","location":{"path":"main.sql","lines":{"begin":2,"end":2}},"check_name":"adding-required-field"},{"description":"Missing `IF NOT EXISTS`, the migration can't be rerun if it fails part way through.","severity":"minor","fingerprint":"1b2e8c81e717c442","location":{"path":"main.sql","lines":{"begin":2,"end":2}},"check_name":"prefer-robust-stmts"},{"description":"Using 32-bit integer fields can result in hitting the max `int` limit. Suggestion: Use 64-bit integer values instead to prevent hitting this limit.","severity":"minor","fingerprint":"2bed2a431803b811","location":{"path":"main.sql","lines":{"begin":2,"end":2}},"check_name":"prefer-bigint-over-int"}]"#); + assert_snapshot!(String::from_utf8_lossy(&buff), @r#"[{"description":"Missing `set lock_timeout` before potentially slow ACCESS EXCLUSIVE lock operations Suggestion: Configure a `lock_timeout` before this statement. Statement requires: ACCESS EXCLUSIVE lock; blocking: reads, writes, schema changes.","severity":"minor","fingerprint":"c08bce2325e355cd","location":{"path":"main.sql","lines":{"begin":1,"end":1}},"check_name":"require-timeout-settings"},{"description":"Missing `set statement_timeout` before potentially slow operations Suggestion: Configure a `statement_timeout` before this statement","severity":"minor","fingerprint":"2904632bf27251d2","location":{"path":"main.sql","lines":{"begin":1,"end":1}},"check_name":"require-timeout-settings"},{"description":"Adding a new column that is `NOT NULL` and has no default value to an existing table effectively makes it required. Suggestion: Make the field nullable or add a non-VOLATILE DEFAULT","severity":"minor","fingerprint":"87fbb54d93cdb8c9","location":{"path":"main.sql","lines":{"begin":1,"end":1}},"check_name":"adding-required-field"},{"description":"Missing `IF NOT EXISTS`, the migration can't be rerun if it fails part way through.","severity":"minor","fingerprint":"21df0ee3817ad84","location":{"path":"main.sql","lines":{"begin":1,"end":1}},"check_name":"prefer-robust-stmts"},{"description":"Using 32-bit integer fields can result in hitting the max `int` limit. Suggestion: Use 64-bit integer values instead to prevent hitting this limit.","severity":"minor","fingerprint":"3d0e81dc13bc8757","location":{"path":"main.sql","lines":{"begin":1,"end":1}},"check_name":"prefer-bigint-over-int"},{"description":"Adding a new column that is `NOT NULL` and has no default value to an existing table effectively makes it required. Suggestion: Make the field nullable or add a non-VOLATILE DEFAULT","severity":"minor","fingerprint":"4bdd655ad8e102ad","location":{"path":"main.sql","lines":{"begin":2,"end":2}},"check_name":"adding-required-field"},{"description":"Missing `IF NOT EXISTS`, the migration can't be rerun if it fails part way through.","severity":"minor","fingerprint":"1b2e8c81e717c442","location":{"path":"main.sql","lines":{"begin":2,"end":2}},"check_name":"prefer-robust-stmts"},{"description":"Using 32-bit integer fields can result in hitting the max `int` limit. Suggestion: Use 64-bit integer values instead to prevent hitting this limit.","severity":"minor","fingerprint":"2bed2a431803b811","location":{"path":"main.sql","lines":{"begin":2,"end":2}},"check_name":"prefer-bigint-over-int"}]"#); } #[test] diff --git a/crates/squawk/src/snapshots/example.svg b/crates/squawk/src/snapshots/example.svg index 06f67499..0941e31a 100644 --- a/crates/squawk/src/snapshots/example.svg +++ b/crates/squawk/src/snapshots/example.svg @@ -106,7 +106,7 @@ ╰╴ ++++++++++++ - warning[require-timeout-settings]: Missing `set lock_timeout` before potentially slow operations + warning[require-timeout-settings]: Missing `set lock_timeout` before potentially slow SHARE lock operations ╭▸ ../../example.sql:10:1 @@ -118,7 +118,7 @@ │ - ├ help: Configure a `lock_timeout` before this statement. + ├ help: Configure a `lock_timeout` before this statement. Statement requires: SHARE lock; blocking: writes, schema changes. ╭╴ diff --git a/crates/squawk/src/snapshots/squawk__reporter__test_reporter__display_violations_tty.snap b/crates/squawk/src/snapshots/squawk__reporter__test_reporter__display_violations_tty.snap index c38569ea..ea6e5cee 100644 --- a/crates/squawk/src/snapshots/squawk__reporter__test_reporter__display_violations_tty.snap +++ b/crates/squawk/src/snapshots/squawk__reporter__test_reporter__display_violations_tty.snap @@ -2,13 +2,13 @@ source: crates/squawk/src/reporter.rs expression: "strip_ansi_codes(&String::from_utf8_lossy(&buff))" --- -warning[]8;;https://squawkhq.com/docs/require-timeout-settings\require-timeout-settings]8;;\]: Missing `set lock_timeout` before potentially slow operations +warning[]8;;https://squawkhq.com/docs/require-timeout-settings\require-timeout-settings]8;;\]: Missing `set lock_timeout` before potentially slow ACCESS EXCLUSIVE lock operations ╭▸ main.sql:2:4 │ 2 │ ALTER TABLE "core_recipe" ADD COLUMN "foo" integer NOT NULL; │ ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ │ - ├ help: Configure a `lock_timeout` before this statement. + ├ help: Configure a `lock_timeout` before this statement. Statement requires: ACCESS EXCLUSIVE lock; blocking: reads, writes, schema changes. ╭╴ 2 + set lock_timeout = '1s'; ╰╴ diff --git a/crates/squawk/src/snapshots/squawk__reporter__test_reporter__display_violations_tty_and_github_annotations.snap b/crates/squawk/src/snapshots/squawk__reporter__test_reporter__display_violations_tty_and_github_annotations.snap index d2dc3968..5db537a7 100644 --- a/crates/squawk/src/snapshots/squawk__reporter__test_reporter__display_violations_tty_and_github_annotations.snap +++ b/crates/squawk/src/snapshots/squawk__reporter__test_reporter__display_violations_tty_and_github_annotations.snap @@ -2,7 +2,7 @@ source: crates/squawk/src/reporter.rs expression: "strip_ansi_codes(&String::from_utf8_lossy(&buff))" --- -::warning file=main.sql,line=2,col=3,endLine=2,endColumn=63,title=require-timeout-settings::Missing `set lock_timeout` before potentially slow operations +::warning file=main.sql,line=2,col=3,endLine=2,endColumn=63,title=require-timeout-settings::Missing `set lock_timeout` before potentially slow ACCESS EXCLUSIVE lock operations ::warning file=main.sql,line=2,col=3,endLine=2,endColumn=63,title=require-timeout-settings::Missing `set statement_timeout` before potentially slow operations ::warning file=main.sql,line=2,col=29,endLine=2,endColumn=62,title=adding-required-field::Adding a new column that is `NOT NULL` and has no default value to an existing table effectively makes it required. ::warning file=main.sql,line=2,col=29,endLine=2,endColumn=62,title=prefer-robust-stmts::Missing `IF NOT EXISTS`, the migration can't be rerun if it fails part way through. @@ -10,13 +10,13 @@ expression: "strip_ansi_codes(&String::from_utf8_lossy(&buff))" ::warning file=main.sql,line=5,col=2,endLine=6,endColumn=20,title=adding-required-field::Adding a new column that is `NOT NULL` and has no default value to an existing table effectively makes it required. ::warning file=main.sql,line=5,col=2,endLine=6,endColumn=20,title=prefer-robust-stmts::Missing `IF NOT EXISTS`, the migration can't be rerun if it fails part way through. ::warning file=main.sql,line=6,col=4,endLine=6,endColumn=11,title=prefer-bigint-over-int::Using 32-bit integer fields can result in hitting the max `int` limit. -warning[]8;;https://squawkhq.com/docs/require-timeout-settings\require-timeout-settings]8;;\]: Missing `set lock_timeout` before potentially slow operations +warning[]8;;https://squawkhq.com/docs/require-timeout-settings\require-timeout-settings]8;;\]: Missing `set lock_timeout` before potentially slow ACCESS EXCLUSIVE lock operations ╭▸ main.sql:2:4 │ 2 │ ALTER TABLE "core_recipe" ADD COLUMN "foo" integer NOT NULL; │ ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ │ - ├ help: Configure a `lock_timeout` before this statement. + ├ help: Configure a `lock_timeout` before this statement. Statement requires: ACCESS EXCLUSIVE lock; blocking: reads, writes, schema changes. ╭╴ 2 + set lock_timeout = '1s'; ╰╴ diff --git a/crates/squawk/src/snapshots/squawk__reporter__test_reporter__span_offsets.snap b/crates/squawk/src/snapshots/squawk__reporter__test_reporter__span_offsets.snap index 3eb082b5..1a0254dd 100644 --- a/crates/squawk/src/snapshots/squawk__reporter__test_reporter__span_offsets.snap +++ b/crates/squawk/src/snapshots/squawk__reporter__test_reporter__span_offsets.snap @@ -12,9 +12,9 @@ CheckReport { column: 3, range: 5..65, level: Warning, - message: "Missing `set lock_timeout` before potentially slow operations", + message: "Missing `set lock_timeout` before potentially slow ACCESS EXCLUSIVE lock operations", help: Some( - "Configure a `lock_timeout` before this statement.", + "Configure a `lock_timeout` before this statement. Statement requires: ACCESS EXCLUSIVE lock; blocking: reads, writes, schema changes.", ), rule_name: "require-timeout-settings", column_end: 63, diff --git a/crates/squawk_linter/Cargo.toml b/crates/squawk_linter/Cargo.toml index fa6de939..01db657b 100644 --- a/crates/squawk_linter/Cargo.toml +++ b/crates/squawk_linter/Cargo.toml @@ -26,6 +26,7 @@ rustc-hash.workspace = true [dev-dependencies] insta.workspace = true +tabled.workspace = true [lints] workspace = true diff --git a/crates/squawk_linter/src/ignore.rs b/crates/squawk_linter/src/ignore.rs index e5e048b7..18cdd0c3 100644 --- a/crates/squawk_linter/src/ignore.rs +++ b/crates/squawk_linter/src/ignore.rs @@ -302,10 +302,10 @@ create table users ( [ Violation { code: RequireTimeoutSettings, - message: "Missing `set lock_timeout` before potentially slow operations", + message: "Missing `set lock_timeout` before potentially slow ACCESS EXCLUSIVE lock operations", text_range: 0..32, help: Some( - "Configure a `lock_timeout` before this statement.", + "Configure a `lock_timeout` before this statement. Statement requires: ACCESS EXCLUSIVE lock; blocking: reads, writes, schema changes.", ), fix: Some( Fix { @@ -569,7 +569,7 @@ alter table t2 drop column c2 cascade; ), ( RequireTimeoutSettings, - "Missing `set lock_timeout` before potentially slow operations", + "Missing `set lock_timeout` before potentially slow ACCESS EXCLUSIVE lock operations", ), ( RequireTimeoutSettings, diff --git a/crates/squawk_linter/src/rules/require_timeout_settings.rs b/crates/squawk_linter/src/rules/require_timeout_settings.rs index a026601a..e065c2e5 100644 --- a/crates/squawk_linter/src/rules/require_timeout_settings.rs +++ b/crates/squawk_linter/src/rules/require_timeout_settings.rs @@ -41,26 +41,223 @@ enum ReportOnce { Reported, } +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +struct LockImpact(u8); + +impl LockImpact { + const READS: u8 = 0b001; + const SCHEMA_CHANGES: u8 = 0b100; + const WRITES: u8 = 0b010; + + const fn new(blocks: u8) -> Self { + Self(blocks) + } + + fn blocked(self) -> Vec<&'static str> { + let mut items = vec![]; + for (bit, label) in [ + (Self::READS, "reads"), + (Self::WRITES, "writes"), + (Self::SCHEMA_CHANGES, "schema changes"), + ] { + if self.0 & bit != 0 { + items.push(label); + } + } + items + } +} + /// The lock a statement takes. #[derive(Debug, Clone, Copy, PartialEq, Eq)] enum LockKind { - Unknown, + AccessExclusive, + AccessShare, + Exclusive, + RowExclusive, + RowShare, + Share, + ShareRowExclusive, ShareUpdateExclusive, + Unknown, } impl fmt::Display for LockKind { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match self { - LockKind::Unknown => Ok(()), + LockKind::AccessExclusive => f.write_str("ACCESS EXCLUSIVE"), + LockKind::AccessShare => f.write_str("ACCESS SHARE"), + LockKind::Exclusive => f.write_str("EXCLUSIVE"), + LockKind::RowExclusive => f.write_str("ROW EXCLUSIVE"), + LockKind::RowShare => f.write_str("ROW SHARE"), + LockKind::Share => f.write_str("SHARE"), + LockKind::ShareRowExclusive => f.write_str("SHARE ROW EXCLUSIVE"), LockKind::ShareUpdateExclusive => f.write_str("SHARE UPDATE EXCLUSIVE"), + LockKind::Unknown => Ok(()), } } } impl LockKind { + fn from_lock_mode(lock_mode: ast::LockMode) -> LockKind { + match lock_mode { + ast::LockMode::AccessExclusive(_) => LockKind::AccessExclusive, + ast::LockMode::AccessShare(_) => LockKind::AccessShare, + ast::LockMode::Exclusive(_) => LockKind::Exclusive, + ast::LockMode::RowExclusive(_) => LockKind::RowExclusive, + ast::LockMode::RowShare(_) => LockKind::RowShare, + ast::LockMode::Share(_) => LockKind::Share, + ast::LockMode::ShareRowExclusive(_) => LockKind::ShareRowExclusive, + ast::LockMode::ShareUpdateExclusive(_) => LockKind::ShareUpdateExclusive, + } + } + + fn strength(self) -> u8 { + match self { + LockKind::Unknown => 0, + LockKind::AccessShare => 1, + LockKind::RowShare => 2, + LockKind::RowExclusive => 3, + LockKind::ShareUpdateExclusive => 4, + LockKind::Share => 5, + LockKind::ShareRowExclusive => 6, + LockKind::Exclusive => 7, + LockKind::AccessExclusive => 8, + } + } + + fn from_alter_table_action(action: &ast::AlterTableAction) -> LockKind { + match action { + ast::AlterTableAction::ClusterOn(_) + | ast::AlterTableAction::ResetOptions(_) + | ast::AlterTableAction::SetOptions(_) + | ast::AlterTableAction::SetWithoutCluster(_) + | ast::AlterTableAction::ValidateConstraint(_) => LockKind::ShareUpdateExclusive, + ast::AlterTableAction::DetachPartition(_) => LockKind::AccessExclusive, + ast::AlterTableAction::AddConstraint(add_constraint) => { + if let Some(ast::Constraint::ForeignKeyConstraint(_)) = add_constraint.constraint() + { + LockKind::ShareRowExclusive + } else { + LockKind::AccessExclusive + } + } + ast::AlterTableAction::DisableTrigger(_) + | ast::AlterTableAction::EnableAlwaysTrigger(_) + | ast::AlterTableAction::EnableReplicaTrigger(_) + | ast::AlterTableAction::EnableTrigger(_) => LockKind::ShareRowExclusive, + ast::AlterTableAction::AlterColumn(alter_column) => match alter_column.option() { + Some( + ast::AlterColumnOption::ResetOptions(_) + | ast::AlterColumnOption::SetOptions(_) + | ast::AlterColumnOption::SetStatistics(_), + ) => LockKind::ShareUpdateExclusive, + Some( + ast::AlterColumnOption::AddGenerated(_) + | ast::AlterColumnOption::DropDefault(_) + | ast::AlterColumnOption::DropExpression(_) + | ast::AlterColumnOption::DropIdentity(_) + | ast::AlterColumnOption::DropNotNull(_) + | ast::AlterColumnOption::Inherit(_) + | ast::AlterColumnOption::NoInherit(_) + | ast::AlterColumnOption::Restart(_) + | ast::AlterColumnOption::SetCompression(_) + | ast::AlterColumnOption::SetDefault(_) + | ast::AlterColumnOption::SetExpression(_) + | ast::AlterColumnOption::SetGenerated(_) + | ast::AlterColumnOption::SetGeneratedOptions(_) + | ast::AlterColumnOption::SetNotNull(_) + | ast::AlterColumnOption::SetOptionsList(_) + | ast::AlterColumnOption::SetSequenceOption(_) + | ast::AlterColumnOption::SetStorage(_) + | ast::AlterColumnOption::SetType(_), + ) + | None => LockKind::AccessExclusive, + }, + ast::AlterTableAction::AddColumn(_) + | ast::AlterTableAction::AlterConstraint(_) + | ast::AlterTableAction::AttachPartition(_) + | ast::AlterTableAction::DisableRls(_) + | ast::AlterTableAction::DisableRule(_) + | ast::AlterTableAction::DropColumn(_) + | ast::AlterTableAction::DropConstraint(_) + | ast::AlterTableAction::EnableAlwaysRule(_) + | ast::AlterTableAction::EnableReplicaRule(_) + | ast::AlterTableAction::EnableRls(_) + | ast::AlterTableAction::EnableRule(_) + | ast::AlterTableAction::ForceRls(_) + | ast::AlterTableAction::InheritTable(_) + | ast::AlterTableAction::MergePartitions(_) + | ast::AlterTableAction::NoForceRls(_) + | ast::AlterTableAction::NoInheritTable(_) + | ast::AlterTableAction::NotOf(_) + | ast::AlterTableAction::OfType(_) + | ast::AlterTableAction::OptionItemList(_) + | ast::AlterTableAction::OwnerTo(_) + | ast::AlterTableAction::RenameColumn(_) + | ast::AlterTableAction::RenameConstraint(_) + | ast::AlterTableAction::RenameTo(_) + | ast::AlterTableAction::ReplicaIdentity(_) + | ast::AlterTableAction::SetAccessMethod(_) + | ast::AlterTableAction::SetLogged(_) + | ast::AlterTableAction::SetSchema(_) + | ast::AlterTableAction::SetTablespace(_) + | ast::AlterTableAction::SetUnlogged(_) + | ast::AlterTableAction::SetWithoutOids(_) + | ast::AlterTableAction::SplitPartition(_) => LockKind::AccessExclusive, + } + } + fn from(stmt: &ast::Stmt) -> LockKind { match stmt { + ast::Stmt::AlterTable(alter_table) => alter_table + .actions() + .map(|action| Self::from_alter_table_action(&action)) + .max_by_key(|lock| lock.strength()) + .unwrap_or(LockKind::AccessExclusive), + ast::Stmt::Cluster(_) => LockKind::AccessExclusive, ast::Stmt::CommentOn(_) => LockKind::ShareUpdateExclusive, + ast::Stmt::CreateIndex(create_index) => { + if create_index.concurrently_token().is_some() { + LockKind::ShareUpdateExclusive + } else { + LockKind::Share + } + } + ast::Stmt::DropIndex(drop_index) => { + if drop_index.concurrently_token().is_some() { + LockKind::ShareUpdateExclusive + } else { + LockKind::AccessExclusive + } + } + ast::Stmt::DropTable(_) | ast::Stmt::DropView(_) => LockKind::AccessExclusive, + ast::Stmt::Lock(lock) => lock + .lock_mode() + .map(Self::from_lock_mode) + .unwrap_or(LockKind::AccessExclusive), + ast::Stmt::Refresh(refresh) => { + if refresh.concurrently_token().is_some() { + LockKind::Exclusive + } else { + LockKind::AccessExclusive + } + } + ast::Stmt::Reindex(reindex) => { + if reindex.is_concurrently() { + LockKind::ShareUpdateExclusive + } else { + LockKind::AccessExclusive + } + } + ast::Stmt::Truncate(_) => LockKind::AccessExclusive, + ast::Stmt::Vacuum(vacuum) => { + if vacuum.is_full() { + LockKind::AccessExclusive + } else { + LockKind::ShareUpdateExclusive + } + } _ => LockKind::Unknown, } } @@ -74,14 +271,37 @@ impl LockKind { } } - fn help(self) -> &'static str { + fn impact(self) -> Option { match self { - LockKind::Unknown => "Configure a `lock_timeout` before this statement.", - LockKind::ShareUpdateExclusive => { - "Configure a `lock_timeout` before this statement. It takes a SHARE UPDATE \ - EXCLUSIVE lock. It will block other schema changes (VACUUM, ANALYZE, index \ - builds) but not reads or writes." + LockKind::AccessExclusive => Some(LockImpact::new( + LockImpact::READS | LockImpact::WRITES | LockImpact::SCHEMA_CHANGES, + )), + LockKind::AccessShare => Some(LockImpact::new(LockImpact::SCHEMA_CHANGES)), + LockKind::Exclusive | LockKind::Share | LockKind::ShareRowExclusive => Some( + LockImpact::new(LockImpact::WRITES | LockImpact::SCHEMA_CHANGES), + ), + LockKind::RowExclusive | LockKind::RowShare | LockKind::ShareUpdateExclusive => { + Some(LockImpact::new(LockImpact::SCHEMA_CHANGES)) } + LockKind::Unknown => None, + } + } + + fn help(self) -> String { + let help = "Configure a `lock_timeout` before this statement."; + let name = self.to_string(); + let Some(impact) = self.impact() else { + return help.to_string(); + }; + + let blocked = impact.blocked(); + if blocked.is_empty() { + format!("{help} Statement requires: {name} lock.") + } else { + format!( + "{help} Statement requires: {name} lock; blocking: {}.", + blocked.join(", ") + ) } } } @@ -126,7 +346,7 @@ pub(crate) fn require_timeout_settings(ctx: &mut Linter, parse: &Parse String { + let mut builder = Builder::default(); + builder.push_record(["sql", "lock"]); + + for sql in cases { + let file = SourceFile::parse(sql); + assert_eq!(file.errors(), vec![]); + let stmt = file.tree().stmts().next().expect("expected statement"); + let lock = super::LockKind::from(&stmt); + builder.push_record([sql.to_string(), format!("{lock:?}")]); + } + + let mut table = builder.build(); + table.with(Style::psql()); + table.to_string() + } + + #[test] + fn lock_kind_for_statement_variants() { + let cases = [ + "ALTER TABLE t ADD COLUMN c int;", + "CLUSTER t;", + "COMMENT ON TABLE t IS 'x';", + "CREATE INDEX idx ON t (c);", + "CREATE INDEX CONCURRENTLY idx ON t (c);", + "DROP INDEX idx;", + "DROP INDEX CONCURRENTLY idx;", + "DROP TABLE t;", + "DROP VIEW v;", + "REFRESH MATERIALIZED VIEW mv;", + "REFRESH MATERIALIZED VIEW CONCURRENTLY mv;", + "REINDEX INDEX idx;", + "REINDEX INDEX CONCURRENTLY idx;", + "TRUNCATE t;", + "CREATE TYPE mood AS ENUM ('sad', 'ok', 'happy');", + ]; + + assert_snapshot!(lock_kinds(&cases), @" + sql | lock + --------------------------------------------------+---------------------- + ALTER TABLE t ADD COLUMN c int; | AccessExclusive + CLUSTER t; | AccessExclusive + COMMENT ON TABLE t IS 'x'; | ShareUpdateExclusive + CREATE INDEX idx ON t (c); | Share + CREATE INDEX CONCURRENTLY idx ON t (c); | ShareUpdateExclusive + DROP INDEX idx; | AccessExclusive + DROP INDEX CONCURRENTLY idx; | ShareUpdateExclusive + DROP TABLE t; | AccessExclusive + DROP VIEW v; | AccessExclusive + REFRESH MATERIALIZED VIEW mv; | AccessExclusive + REFRESH MATERIALIZED VIEW CONCURRENTLY mv; | Exclusive + REINDEX INDEX idx; | AccessExclusive + REINDEX INDEX CONCURRENTLY idx; | ShareUpdateExclusive + TRUNCATE t; | AccessExclusive + CREATE TYPE mood AS ENUM ('sad', 'ok', 'happy'); | Unknown + "); + } + + #[test] + fn lock_kind_for_alter_table_variants() { + let cases = [ + "ALTER TABLE t ADD COLUMN c int;", + "ALTER TABLE t DROP COLUMN c;", + "ALTER TABLE t ALTER COLUMN c TYPE bigint;", + "ALTER TABLE t VALIDATE CONSTRAINT c;", + "ALTER TABLE t ALTER COLUMN c SET STATISTICS 100;", + "ALTER TABLE t ALTER COLUMN c SET (n_distinct = 5);", + "ALTER TABLE t SET (fillfactor = 70);", + "ALTER TABLE t RESET (fillfactor);", + "ALTER TABLE t CLUSTER ON idx;", + "ALTER TABLE t SET WITHOUT CLUSTER;", + "ALTER TABLE t DETACH PARTITION p;", + "ALTER TABLE t DETACH PARTITION p CONCURRENTLY;", + "ALTER TABLE t ADD CONSTRAINT fk FOREIGN KEY (a) REFERENCES o (id);", + "ALTER TABLE t ADD CONSTRAINT ck CHECK (a > 0);", + "ALTER TABLE t DISABLE TRIGGER trg;", + "ALTER TABLE t ENABLE TRIGGER trg;", + "ALTER TABLE t ENABLE REPLICA TRIGGER trg;", + "ALTER TABLE t ENABLE ALWAYS TRIGGER trg;", + "ALTER TABLE t VALIDATE CONSTRAINT c, ADD COLUMN d int;", + ]; + + assert_snapshot!(lock_kinds(&cases), @" + sql | lock + --------------------------------------------------------------------+---------------------- + ALTER TABLE t ADD COLUMN c int; | AccessExclusive + ALTER TABLE t DROP COLUMN c; | AccessExclusive + ALTER TABLE t ALTER COLUMN c TYPE bigint; | AccessExclusive + ALTER TABLE t VALIDATE CONSTRAINT c; | ShareUpdateExclusive + ALTER TABLE t ALTER COLUMN c SET STATISTICS 100; | ShareUpdateExclusive + ALTER TABLE t ALTER COLUMN c SET (n_distinct = 5); | ShareUpdateExclusive + ALTER TABLE t SET (fillfactor = 70); | ShareUpdateExclusive + ALTER TABLE t RESET (fillfactor); | ShareUpdateExclusive + ALTER TABLE t CLUSTER ON idx; | ShareUpdateExclusive + ALTER TABLE t SET WITHOUT CLUSTER; | ShareUpdateExclusive + ALTER TABLE t DETACH PARTITION p; | AccessExclusive + ALTER TABLE t DETACH PARTITION p CONCURRENTLY; | AccessExclusive + ALTER TABLE t ADD CONSTRAINT fk FOREIGN KEY (a) REFERENCES o (id); | ShareRowExclusive + ALTER TABLE t ADD CONSTRAINT ck CHECK (a > 0); | AccessExclusive + ALTER TABLE t DISABLE TRIGGER trg; | ShareRowExclusive + ALTER TABLE t ENABLE TRIGGER trg; | ShareRowExclusive + ALTER TABLE t ENABLE REPLICA TRIGGER trg; | ShareRowExclusive + ALTER TABLE t ENABLE ALWAYS TRIGGER trg; | ShareRowExclusive + ALTER TABLE t VALIDATE CONSTRAINT c, ADD COLUMN d int; | AccessExclusive + "); + } + + #[test] + fn lock_kind_for_explicit_lock_modes() { + let cases = [ + "LOCK TABLE t;", + "LOCK TABLE t IN ACCESS EXCLUSIVE MODE;", + "LOCK TABLE t IN ACCESS SHARE MODE;", + "LOCK TABLE t IN EXCLUSIVE MODE;", + "LOCK TABLE t IN ROW EXCLUSIVE MODE;", + "LOCK TABLE t IN ROW SHARE MODE;", + "LOCK TABLE t IN SHARE MODE;", + "LOCK TABLE t IN SHARE ROW EXCLUSIVE MODE;", + "LOCK TABLE t IN SHARE UPDATE EXCLUSIVE MODE;", + ]; + + assert_snapshot!(lock_kinds(&cases), @" + sql | lock + ----------------------------------------------+---------------------- + LOCK TABLE t; | AccessExclusive + LOCK TABLE t IN ACCESS EXCLUSIVE MODE; | AccessExclusive + LOCK TABLE t IN ACCESS SHARE MODE; | AccessShare + LOCK TABLE t IN EXCLUSIVE MODE; | Exclusive + LOCK TABLE t IN ROW EXCLUSIVE MODE; | RowExclusive + LOCK TABLE t IN ROW SHARE MODE; | RowShare + LOCK TABLE t IN SHARE MODE; | Share + LOCK TABLE t IN SHARE ROW EXCLUSIVE MODE; | ShareRowExclusive + LOCK TABLE t IN SHARE UPDATE EXCLUSIVE MODE; | ShareUpdateExclusive + "); + } + + #[test] + fn lock_kind_for_reindex_concurrently_options() { + let cases = [ + "REINDEX (CONCURRENTLY) INDEX idx;", + "REINDEX (CONCURRENTLY true) INDEX idx;", + "REINDEX (CONCURRENTLY false) INDEX idx;", + "REINDEX (CONCURRENTLY 'false') INDEX idx;", + "REINDEX (CONCURRENTLY E'false') INDEX idx;", + "REINDEX (CONCURRENTLY U&'false') INDEX idx;", + "REINDEX (CONCURRENTLY off) INDEX idx;", + "REINDEX (CONCURRENTLY 0) INDEX idx;", + ]; + + assert_snapshot!(lock_kinds(&cases), @" + sql | lock + ---------------------------------------------+---------------------- + REINDEX (CONCURRENTLY) INDEX idx; | ShareUpdateExclusive + REINDEX (CONCURRENTLY true) INDEX idx; | ShareUpdateExclusive + REINDEX (CONCURRENTLY false) INDEX idx; | AccessExclusive + REINDEX (CONCURRENTLY 'false') INDEX idx; | AccessExclusive + REINDEX (CONCURRENTLY E'false') INDEX idx; | AccessExclusive + REINDEX (CONCURRENTLY U&'false') INDEX idx; | AccessExclusive + REINDEX (CONCURRENTLY off) INDEX idx; | AccessExclusive + REINDEX (CONCURRENTLY 0) INDEX idx; | AccessExclusive + "); + } + + #[test] + fn lock_kind_for_vacuum_full_options() { + let cases = [ + "VACUUM t;", + "VACUUM FULL t;", + "VACUUM (FULL) t;", + "VACUUM (FULL true) t;", + "VACUUM (FULL false) t;", + "VACUUM (FULL 'false') t;", + "VACUUM (FULL E'false') t;", + "VACUUM (FULL U&'false') t;", + "VACUUM (FULL off) t;", + "VACUUM (FULL 0) t;", + ]; + + assert_snapshot!(lock_kinds(&cases), @" + sql | lock + ----------------------------+---------------------- + VACUUM t; | ShareUpdateExclusive + VACUUM FULL t; | AccessExclusive + VACUUM (FULL) t; | AccessExclusive + VACUUM (FULL true) t; | AccessExclusive + VACUUM (FULL false) t; | ShareUpdateExclusive + VACUUM (FULL 'false') t; | ShareUpdateExclusive + VACUUM (FULL E'false') t; | ShareUpdateExclusive + VACUUM (FULL U&'false') t; | ShareUpdateExclusive + VACUUM (FULL off) t; | ShareUpdateExclusive + VACUUM (FULL 0) t; | ShareUpdateExclusive + "); + } + #[test] fn err_missing_both_timeouts() { let sql = r#" ALTER TABLE t ADD COLUMN c BOOLEAN; "#; assert_snapshot!(lint_errors(sql, Rule::RequireTimeoutSettings), @" - warning[require-timeout-settings]: Missing `set lock_timeout` before potentially slow operations + warning[require-timeout-settings]: Missing `set lock_timeout` before potentially slow ACCESS EXCLUSIVE lock operations ╭▸ 2 │ ALTER TABLE t ADD COLUMN c BOOLEAN; │ ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ │ - ├ help: Configure a `lock_timeout` before this statement. + ├ help: Configure a `lock_timeout` before this statement. Statement requires: ACCESS EXCLUSIVE lock; blocking: reads, writes, schema changes. ╭╴ 2 + set lock_timeout = '1s'; ╰╴ @@ -198,12 +614,12 @@ SET statement_timeout = '5s'; ALTER TABLE t ADD COLUMN c BOOLEAN; "#; assert_snapshot!(lint_errors(sql, Rule::RequireTimeoutSettings), @" - warning[require-timeout-settings]: Missing `set lock_timeout` before potentially slow operations + warning[require-timeout-settings]: Missing `set lock_timeout` before potentially slow ACCESS EXCLUSIVE lock operations ╭▸ 3 │ ALTER TABLE t ADD COLUMN c BOOLEAN; │ ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ │ - ├ help: Configure a `lock_timeout` before this statement. + ├ help: Configure a `lock_timeout` before this statement. Statement requires: ACCESS EXCLUSIVE lock; blocking: reads, writes, schema changes. ╭╴ 2 + set lock_timeout = '1s'; ╰╴ @@ -265,12 +681,12 @@ SET foo.statement_timeout = '5s'; ALTER TABLE t ADD COLUMN c BOOLEAN; "#; assert_snapshot!(lint_errors(sql, Rule::RequireTimeoutSettings), @" - warning[require-timeout-settings]: Missing `set lock_timeout` before potentially slow operations + warning[require-timeout-settings]: Missing `set lock_timeout` before potentially slow ACCESS EXCLUSIVE lock operations ╭▸ 4 │ ALTER TABLE t ADD COLUMN c BOOLEAN; │ ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ │ - ├ help: Configure a `lock_timeout` before this statement. + ├ help: Configure a `lock_timeout` before this statement. Statement requires: ACCESS EXCLUSIVE lock; blocking: reads, writes, schema changes. ╭╴ 2 + set lock_timeout = '1s'; ╰╴ @@ -293,12 +709,12 @@ SET lock_timeout = '1s'; SET statement_timeout = '5s'; "#; assert_snapshot!(lint_errors(sql, Rule::RequireTimeoutSettings), @" - warning[require-timeout-settings]: Missing `set lock_timeout` before potentially slow operations + warning[require-timeout-settings]: Missing `set lock_timeout` before potentially slow ACCESS EXCLUSIVE lock operations ╭▸ 2 │ ALTER TABLE t ADD COLUMN c BOOLEAN; │ ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ │ - ├ help: Configure a `lock_timeout` before this statement. + ├ help: Configure a `lock_timeout` before this statement. Statement requires: ACCESS EXCLUSIVE lock; blocking: reads, writes, schema changes. ╭╴ 2 + set lock_timeout = '1s'; ╰╴ @@ -355,7 +771,7 @@ COMMENT ON COLUMN t.c IS 'an opaque id'; 2 │ COMMENT ON COLUMN t.c IS 'an opaque id'; │ ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ │ - ├ help: Configure a `lock_timeout` before this statement. It takes a SHARE UPDATE EXCLUSIVE lock. It will block other schema changes (VACUUM, ANALYZE, index builds) but not reads or writes. + ├ help: Configure a `lock_timeout` before this statement. Statement requires: SHARE UPDATE EXCLUSIVE lock; blocking: schema changes. ╭╴ 2 + set lock_timeout = '1s'; ╰╴ @@ -371,6 +787,87 @@ COMMENT ON COLUMN t.c IS 'an opaque id'; "); } + #[test] + fn err_reindex_concurrently_false_uses_non_concurrent_lock() { + let sql = r#" +REINDEX (CONCURRENTLY false) INDEX idx; + "#; + assert_snapshot!(lint_errors(sql, Rule::RequireTimeoutSettings), @" + warning[require-timeout-settings]: Missing `set lock_timeout` before potentially slow ACCESS EXCLUSIVE lock operations + ╭▸ + 2 │ REINDEX (CONCURRENTLY false) INDEX idx; + │ ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + │ + ├ help: Configure a `lock_timeout` before this statement. Statement requires: ACCESS EXCLUSIVE lock; blocking: reads, writes, schema changes. + ╭╴ + 2 + set lock_timeout = '1s'; + ╰╴ + warning[require-timeout-settings]: Missing `set statement_timeout` before potentially slow operations + ╭▸ + 2 │ REINDEX (CONCURRENTLY false) INDEX idx; + │ ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + │ + ├ help: Configure a `statement_timeout` before this statement + ╭╴ + 2 + set statement_timeout = '5s'; + ╰╴ + "); + } + + #[test] + fn err_vacuum_full_false_uses_non_full_lock() { + let sql = r#" +VACUUM (FULL false) t; + "#; + assert_snapshot!(lint_errors(sql, Rule::RequireTimeoutSettings), @" + warning[require-timeout-settings]: Missing `set lock_timeout` before potentially slow SHARE UPDATE EXCLUSIVE lock operations + ╭▸ + 2 │ VACUUM (FULL false) t; + │ ━━━━━━━━━━━━━━━━━━━━━━ + │ + ├ help: Configure a `lock_timeout` before this statement. Statement requires: SHARE UPDATE EXCLUSIVE lock; blocking: schema changes. + ╭╴ + 2 + set lock_timeout = '1s'; + ╰╴ + warning[require-timeout-settings]: Missing `set statement_timeout` before potentially slow operations + ╭▸ + 2 │ VACUUM (FULL false) t; + │ ━━━━━━━━━━━━━━━━━━━━━━ + │ + ├ help: Configure a `statement_timeout` before this statement + ╭╴ + 2 + set statement_timeout = '5s'; + ╰╴ + "); + } + + #[test] + fn err_lock_access_share_uses_correct_article() { + let sql = r#" +LOCK TABLE t IN ACCESS SHARE MODE; + "#; + assert_snapshot!(lint_errors(sql, Rule::RequireTimeoutSettings), @" + warning[require-timeout-settings]: Missing `set lock_timeout` before potentially slow ACCESS SHARE lock operations + ╭▸ + 2 │ LOCK TABLE t IN ACCESS SHARE MODE; + │ ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + │ + ├ help: Configure a `lock_timeout` before this statement. Statement requires: ACCESS SHARE lock; blocking: schema changes. + ╭╴ + 2 + set lock_timeout = '1s'; + ╰╴ + warning[require-timeout-settings]: Missing `set statement_timeout` before potentially slow operations + ╭▸ + 2 │ LOCK TABLE t IN ACCESS SHARE MODE; + │ ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + │ + ├ help: Configure a `statement_timeout` before this statement + ╭╴ + 2 + set statement_timeout = '5s'; + ╰╴ + "); + } + #[test] fn ok_other_ddl_with_timeouts() { let sql = r#" diff --git a/crates/squawk_syntax/src/ast/node_ext.rs b/crates/squawk_syntax/src/ast/node_ext.rs index bf149555..f1c29819 100644 --- a/crates/squawk_syntax/src/ast/node_ext.rs +++ b/crates/squawk_syntax/src/ast/node_ext.rs @@ -539,6 +539,23 @@ fn is_falsey_vacuum_option_value(value: &ast::VacuumOptionValue) -> bool { .is_some_and(|token| is_falsey_token(&token)) } +impl ast::Reindex { + pub fn is_concurrently(&self) -> bool { + self.concurrently_token().is_some() + || self.reindex_option_list().is_some_and(|options| { + options.reindex_options().any(|option| { + option.concurrently_token().is_some() + && !option.literal().is_some_and(|literal| { + literal + .syntax() + .first_token() + .is_some_and(|token| is_falsey_token(&token)) + }) + }) + }) + } +} + impl ast::Vacuum { pub fn is_full(&self) -> bool { self.full_token().is_some()