From 2157403acc90c99799654091cd6e5af03e6dbba1 Mon Sep 17 00:00:00 2001 From: Steve Dignam Date: Fri, 1 May 2026 23:18:02 -0400 Subject: [PATCH 1/2] linter: new rule: prefer repack --- crates/squawk_linter/src/lib.rs | 9 +- crates/squawk_linter/src/rules/mod.rs | 2 + .../squawk_linter/src/rules/prefer_repack.rs | 237 ++++++++++++++++++ crates/squawk_linter/src/test_utils.rs | 9 +- .../src/generated/syntax_kind.rs | 1 + crates/squawk_parser/src/grammar.rs | 14 +- .../squawk_syntax/src/ast/generated/nodes.rs | 37 +++ crates/squawk_syntax/src/ast/node_ext.rs | 89 +++++++ crates/squawk_syntax/src/postgresql.ungram | 6 +- docs/docs/prefer-repack.md | 34 +++ docs/sidebars.js | 1 + docs/src/pages/index.js | 5 + 12 files changed, 438 insertions(+), 6 deletions(-) create mode 100644 crates/squawk_linter/src/rules/prefer_repack.rs create mode 100644 docs/docs/prefer-repack.md diff --git a/crates/squawk_linter/src/lib.rs b/crates/squawk_linter/src/lib.rs index 7dca6d6fc..413a2d4aa 100644 --- a/crates/squawk_linter/src/lib.rs +++ b/crates/squawk_linter/src/lib.rs @@ -47,6 +47,7 @@ use rules::identifier_too_long; use rules::prefer_bigint_over_int; use rules::prefer_bigint_over_smallint; use rules::prefer_identity; +use rules::prefer_repack; use rules::prefer_robust_stmts; use rules::prefer_text_field; use rules::prefer_timestamptz; @@ -79,6 +80,7 @@ pub enum Rule { PreferBigintOverInt, PreferBigintOverSmallint, PreferIdentity, + PreferRepack, PreferRobustStmts, PreferTextField, PreferTimestampTz, @@ -131,6 +133,7 @@ impl TryFrom<&str> for Rule { "prefer-bigint-over-int" => Ok(Rule::PreferBigintOverInt), "prefer-bigint-over-smallint" => Ok(Rule::PreferBigintOverSmallint), "prefer-identity" => Ok(Rule::PreferIdentity), + "prefer-repack" => Ok(Rule::PreferRepack), "prefer-robust-stmts" => Ok(Rule::PreferRobustStmts), "prefer-text-field" => Ok(Rule::PreferTextField), // this is typo'd so we just support both @@ -199,6 +202,7 @@ impl fmt::Display for Rule { Rule::PreferBigintOverInt => "prefer-bigint-over-int", Rule::PreferBigintOverSmallint => "prefer-bigint-over-smallint", Rule::PreferIdentity => "prefer-identity", + Rule::PreferRepack => "prefer-repack", Rule::PreferRobustStmts => "prefer-robust-stmts", Rule::PreferTextField => "prefer-text-field", Rule::PreferTimestampTz => "prefer-timestamp-tz", @@ -332,7 +336,7 @@ impl Violation { } } -#[derive(Default)] +#[derive(Clone, Default)] pub struct LinterSettings { pub pg_version: Version, pub assume_in_transaction: bool, @@ -410,6 +414,9 @@ impl Linter { if self.rules.contains(&Rule::PreferIdentity) { prefer_identity(self, file); } + if self.rules.contains(&Rule::PreferRepack) { + prefer_repack(self, file); + } if self.rules.contains(&Rule::PreferRobustStmts) { prefer_robust_stmts(self, file); } diff --git a/crates/squawk_linter/src/rules/mod.rs b/crates/squawk_linter/src/rules/mod.rs index 922c3bc24..cbd136d46 100644 --- a/crates/squawk_linter/src/rules/mod.rs +++ b/crates/squawk_linter/src/rules/mod.rs @@ -20,6 +20,7 @@ pub(crate) mod identifier_too_long; pub(crate) mod prefer_bigint_over_int; pub(crate) mod prefer_bigint_over_smallint; pub(crate) mod prefer_identity; +pub(crate) mod prefer_repack; pub(crate) mod prefer_robust_stmts; pub(crate) mod prefer_text_field; pub(crate) mod prefer_timestamptz; @@ -57,6 +58,7 @@ pub(crate) use identifier_too_long::identifier_too_long; pub(crate) use prefer_bigint_over_int::prefer_bigint_over_int; pub(crate) use prefer_bigint_over_smallint::prefer_bigint_over_smallint; pub(crate) use prefer_identity::prefer_identity; +pub(crate) use prefer_repack::prefer_repack; pub(crate) use prefer_robust_stmts::prefer_robust_stmts; pub(crate) use prefer_text_field::prefer_text_field; pub(crate) use prefer_timestamptz::prefer_timestamptz; diff --git a/crates/squawk_linter/src/rules/prefer_repack.rs b/crates/squawk_linter/src/rules/prefer_repack.rs new file mode 100644 index 000000000..d78b6e362 --- /dev/null +++ b/crates/squawk_linter/src/rules/prefer_repack.rs @@ -0,0 +1,237 @@ +use squawk_syntax::{ + Parse, SourceFile, + ast::{self, AstNode}, +}; + +use crate::{Edit, Fix, Linter, Rule, Version, Violation}; + +fn vacuum_full_fix(vacuum: &ast::Vacuum) -> Option { + let tables = vacuum.table_and_columns_list()?; + let tables = tables.syntax(); + let replacement = format!("repack (concurrently) {tables}"); + let edit = Edit::replace(vacuum.syntax().text_range(), replacement); + Some(Fix::new("Replace with `repack (concurrently)`", vec![edit])) +} + +fn cluster_fix(cluster: &ast::Cluster) -> Option { + let replacement = if let Some(on_path) = cluster.on_path() { + let index = cluster.path()?; + let table = on_path.path()?; + format!( + "repack (concurrently) {} using index {}", + table.syntax(), + index.syntax() + ) + } else if let Some(table) = cluster.path() { + if let Some(index_name) = cluster.using_method().and_then(|method| method.name_ref()) { + format!( + "repack (concurrently) {} using index {}", + table.syntax(), + index_name.syntax() + ) + } else { + format!("repack (concurrently) {}", table.syntax()) + } + } else { + "repack (concurrently)".to_string() + }; + let edit = Edit::replace(cluster.syntax().text_range(), replacement); + Some(Fix::new("Replace with `repack (concurrently)`", vec![edit])) +} + +pub(crate) fn prefer_repack(ctx: &mut Linter, parse: &Parse) { + if ctx.settings.pg_version < Version::new(19, None, None) { + return; + } + for stmt in parse.tree().stmts() { + match stmt { + ast::Stmt::Cluster(cluster) => { + let fix = cluster_fix(&cluster); + ctx.report( + Violation::for_node( + Rule::PreferRepack, + "`cluster` requires an `ACCESS EXCLUSIVE` lock which blocks reads and writes to the table.".into(), + cluster.syntax(), + ) + .help("Use `repack` to rewrite the table without blocking reads and writes.") + .fix(fix), + ); + } + ast::Stmt::Vacuum(vacuum) => { + if vacuum.is_full() { + let fix = vacuum_full_fix(&vacuum); + ctx.report( + Violation::for_node( + Rule::PreferRepack, + "`vacuum full` requires an `ACCESS EXCLUSIVE` lock which blocks reads and writes to the table.".into(), + vacuum.syntax(), + ) + .help("Use `repack` to rewrite the table without blocking reads and writes.") + .fix(fix), + ); + } + } + _ => (), + } + } +} + +#[cfg(test)] +mod test { + use insta::assert_snapshot; + + use crate::{ + LinterSettings, Rule, + test_utils::{fix_sql_with, lint_errors_with, lint_ok, lint_ok_with}, + }; + + fn pg19() -> LinterSettings { + LinterSettings { + pg_version: "19".parse().expect("Invalid PostgreSQL version"), + ..Default::default() + } + } + + #[test] + fn cluster_err() { + let sql = "CLUSTER foo;"; + assert_snapshot!(lint_errors_with(sql, pg19(), Rule::PreferRepack), @" + warning[prefer-repack]: `cluster` requires an `ACCESS EXCLUSIVE` lock which blocks reads and writes to the table. + ╭▸ + 1 │ CLUSTER foo; + │ ━━━━━━━━━━━ + │ + ├ help: Use `repack` to rewrite the table without blocking reads and writes. + ╭╴ + 1 - CLUSTER foo; + 1 + repack (concurrently) foo; + ╰╴ + "); + } + + #[test] + fn cluster_no_path_err() { + let sql = "CLUSTER;"; + assert_snapshot!(lint_errors_with(sql, pg19(), Rule::PreferRepack), @" + warning[prefer-repack]: `cluster` requires an `ACCESS EXCLUSIVE` lock which blocks reads and writes to the table. + ╭▸ + 1 │ CLUSTER; + │ ━━━━━━━ + │ + ├ help: Use `repack` to rewrite the table without blocking reads and writes. + ╭╴ + 1 - CLUSTER; + 1 + repack (concurrently); + ╰╴ + "); + } + + #[test] + fn vacuum_full_err() { + let sql = "VACUUM FULL foo;"; + assert_snapshot!(lint_errors_with(sql, pg19(), Rule::PreferRepack), @" + warning[prefer-repack]: `vacuum full` requires an `ACCESS EXCLUSIVE` lock which blocks reads and writes to the table. + ╭▸ + 1 │ VACUUM FULL foo; + │ ━━━━━━━━━━━━━━━ + │ + ├ help: Use `repack` to rewrite the table without blocking reads and writes. + ╭╴ + 1 - VACUUM FULL foo; + 1 + repack (concurrently) foo; + ╰╴ + "); + } + + #[test] + fn vacuum_full_option_list_err() { + let sql = "VACUUM (FULL) foo;"; + assert_snapshot!(lint_errors_with(sql, pg19(), Rule::PreferRepack), @" + warning[prefer-repack]: `vacuum full` requires an `ACCESS EXCLUSIVE` lock which blocks reads and writes to the table. + ╭▸ + 1 │ VACUUM (FULL) foo; + │ ━━━━━━━━━━━━━━━━━ + │ + ├ help: Use `repack` to rewrite the table without blocking reads and writes. + ╭╴ + 1 - VACUUM (FULL) foo; + 1 + repack (concurrently) foo; + ╰╴ + "); + } + + #[test] + fn vacuum_full_false_option_list_ok() { + let sql = "VACUUM (FULL FALSE) foo;"; + lint_ok_with(sql, pg19(), Rule::PreferRepack); + } + + #[test] + fn cluster_below_pg19_ok() { + let sql = "CLUSTER foo;"; + lint_ok(sql, Rule::PreferRepack); + } + + #[test] + fn vacuum_full_below_pg19_ok() { + let sql = "VACUUM FULL foo;"; + lint_ok(sql, Rule::PreferRepack); + } + + #[test] + fn vacuum_no_full_ok() { + let sql = "VACUUM foo;"; + lint_ok_with(sql, pg19(), Rule::PreferRepack); + } + + #[test] + fn vacuum_analyze_ok() { + let sql = "VACUUM ANALYZE foo;"; + lint_ok_with(sql, pg19(), Rule::PreferRepack); + } + + #[test] + fn vacuum_freeze_ok() { + let sql = "VACUUM FREEZE foo;"; + lint_ok_with(sql, pg19(), Rule::PreferRepack); + } + + fn fix(sql: &str) -> String { + fix_sql_with(sql, pg19(), Rule::PreferRepack) + } + + #[test] + fn fix_vacuum_full() { + assert_snapshot!(fix("VACUUM FULL foo;"), @"repack (concurrently) foo;"); + } + + #[test] + fn fix_vacuum_full_option_list() { + assert_snapshot!(fix("VACUUM (FULL) foo;"), @"repack (concurrently) foo;"); + } + + #[test] + fn fix_vacuum_full_multiple_tables() { + assert_snapshot!(fix("VACUUM FULL foo, bar;"), @"repack (concurrently) foo, bar;"); + } + + #[test] + fn fix_cluster_no_index() { + assert_snapshot!(fix("CLUSTER foo;"), @"repack (concurrently) foo;"); + } + + #[test] + fn fix_cluster_with_index() { + assert_snapshot!(fix("CLUSTER foo USING foo_pkey;"), @"repack (concurrently) foo using index foo_pkey;"); + } + + #[test] + fn fix_cluster_legacy_on_syntax() { + assert_snapshot!(fix("CLUSTER verbose foo_pkey ON foo;"), @"repack (concurrently) foo using index foo_pkey;"); + } + + #[test] + fn fix_cluster_no_path() { + assert_snapshot!(fix("CLUSTER;"), @"repack (concurrently);"); + } +} diff --git a/crates/squawk_linter/src/test_utils.rs b/crates/squawk_linter/src/test_utils.rs index b58936061..4e5209177 100644 --- a/crates/squawk_linter/src/test_utils.rs +++ b/crates/squawk_linter/src/test_utils.rs @@ -54,8 +54,8 @@ pub(crate) fn lint_errors_with(sql: &str, settings: LinterSettings, rule: Rule) format_violations(sql, &errors) } -pub(crate) fn fix_sql(sql: &str, rule: Rule) -> String { - let errors = lint(sql, rule); +pub(crate) fn fix_sql_with(sql: &str, settings: LinterSettings, rule: Rule) -> String { + let errors = lint_settings(sql, settings.clone(), rule); assert!(!errors.is_empty(), "Should start with linter errors"); let fixes = errors.into_iter().flat_map(|x| x.fix).collect::>(); @@ -80,6 +80,7 @@ pub(crate) fn fix_sql(sql: &str, rule: Rule) -> String { "Shouldn't introduce any syntax errors" ); let mut linter = Linter::from([rule]); + linter.settings = settings; let errors = linter.lint(&file, &result); assert_eq!( errors.len(), @@ -90,6 +91,10 @@ pub(crate) fn fix_sql(sql: &str, rule: Rule) -> String { result } +pub(crate) fn fix_sql(sql: &str, rule: Rule) -> String { + fix_sql_with(sql, LinterSettings::default(), rule) +} + fn format_violations(sql: &str, violations: &[Violation]) -> String { let mut buf = String::new(); let renderer = Renderer::plain().decor_style(DecorStyle::Unicode); diff --git a/crates/squawk_parser/src/generated/syntax_kind.rs b/crates/squawk_parser/src/generated/syntax_kind.rs index e687aea33..4555b0d7b 100644 --- a/crates/squawk_parser/src/generated/syntax_kind.rs +++ b/crates/squawk_parser/src/generated/syntax_kind.rs @@ -1006,6 +1006,7 @@ pub enum SyntaxKind { ON_COMMIT, ON_CONFLICT_CLAUSE, ON_DELETE_ACTION, + ON_PATH, ON_TABLE, ON_UPDATE_ACTION, OP, diff --git a/crates/squawk_parser/src/grammar.rs b/crates/squawk_parser/src/grammar.rs index cca950f46..19062fb29 100644 --- a/crates/squawk_parser/src/grammar.rs +++ b/crates/squawk_parser/src/grammar.rs @@ -8464,13 +8464,23 @@ fn cluster(p: &mut Parser<'_>) -> CompletedMarker { opt_option_list(p); } let has_name = opt_path_name_ref(p).is_some(); - if has_name && p.eat(ON_KW) { - path_name_ref(p); + if has_name { + opt_on_path(p); } opt_using_method(p); m.complete(p, CLUSTER) } +fn opt_on_path(p: &mut Parser<'_>) { + let m = p.start(); + if p.eat(ON_KW) { + path_name_ref(p); + m.complete(p, ON_PATH); + } else { + m.abandon(p); + } +} + fn repack(p: &mut Parser<'_>) -> CompletedMarker { assert!(p.at(REPACK_KW)); let m = p.start(); diff --git a/crates/squawk_syntax/src/ast/generated/nodes.rs b/crates/squawk_syntax/src/ast/generated/nodes.rs index 4befec300..661cffe2d 100644 --- a/crates/squawk_syntax/src/ast/generated/nodes.rs +++ b/crates/squawk_syntax/src/ast/generated/nodes.rs @@ -2860,6 +2860,10 @@ pub struct Cluster { pub(crate) syntax: SyntaxNode, } impl Cluster { + #[inline] + pub fn on_path(&self) -> Option { + support::child(&self.syntax) + } #[inline] pub fn option_item_list(&self) -> Option { support::child(&self.syntax) @@ -12499,6 +12503,21 @@ impl OnDeleteAction { } } +#[derive(Debug, Clone, PartialEq, Eq, Hash)] +pub struct OnPath { + pub(crate) syntax: SyntaxNode, +} +impl OnPath { + #[inline] + pub fn path(&self) -> Option { + support::child(&self.syntax) + } + #[inline] + pub fn on_token(&self) -> Option { + support::token(&self.syntax, SyntaxKind::ON_KW) + } +} + #[derive(Debug, Clone, PartialEq, Eq, Hash)] pub struct OnTable { pub(crate) syntax: SyntaxNode, @@ -27007,6 +27026,24 @@ impl AstNode for OnDeleteAction { &self.syntax } } +impl AstNode for OnPath { + #[inline] + fn can_cast(kind: SyntaxKind) -> bool { + kind == SyntaxKind::ON_PATH + } + #[inline] + fn cast(syntax: SyntaxNode) -> Option { + if Self::can_cast(syntax.kind()) { + Some(Self { syntax }) + } else { + None + } + } + #[inline] + fn syntax(&self) -> &SyntaxNode { + &self.syntax + } +} impl AstNode for OnTable { #[inline] fn can_cast(kind: SyntaxKind) -> bool { diff --git a/crates/squawk_syntax/src/ast/node_ext.rs b/crates/squawk_syntax/src/ast/node_ext.rs index 2c07b1c45..e7f93b6e7 100644 --- a/crates/squawk_syntax/src/ast/node_ext.rs +++ b/crates/squawk_syntax/src/ast/node_ext.rs @@ -415,6 +415,33 @@ impl ast::CharType { } } +fn is_falsey_option(text: &str) -> bool { + text == "0" || text.eq_ignore_ascii_case("false") || text.eq_ignore_ascii_case("off") +} + +impl ast::Vacuum { + pub fn is_full(&self) -> bool { + self.full_token().is_some() + // TODO: we need a better way of handling option lists + || self.vacuum_option_list().is_some_and(|opt_list| { + opt_list.vacuum_options().any(|opt| { + let mut tokens = opt + .syntax() + .descendants_with_tokens() + .filter_map(|child| child.into_token()) + .filter(|token| !token.kind().is_trivia()); + + tokens + .next() + .is_some_and(|token| token.text().eq_ignore_ascii_case("full")) + && tokens + .next() + .is_none_or(|token| !is_falsey_option(token.text())) + }) + }) + } +} + impl ast::OpSig { #[inline] pub fn lhs(&self) -> Option { @@ -767,3 +794,65 @@ fn cast_sig() { assert_snapshot!(lhs.syntax().text(), @"text"); assert_snapshot!(rhs.syntax().text(), @"int"); } + +#[cfg(test)] +fn extract_vacuum(sql: &str) -> ast::Vacuum { + let parse = SourceFile::parse(sql); + assert!(parse.errors().is_empty()); + let file: SourceFile = parse.tree(); + let stmt = file.stmts().next().unwrap(); + let ast::Stmt::Vacuum(vacuum) = stmt else { + unreachable!() + }; + vacuum +} + +#[test] +fn vacuum_full_is_full() { + assert!(extract_vacuum("VACUUM FULL foo;").is_full()); +} + +#[test] +fn vacuum_option_list_full_is_full() { + assert!(extract_vacuum("VACUUM (FULL) foo;").is_full()); +} + +#[test] +fn vacuum_full_true_is_full() { + assert!(extract_vacuum("VACUUM (FULL TRUE) foo;").is_full()); +} + +#[test] +fn vacuum_full_on_is_full() { + assert!(extract_vacuum("VACUUM (FULL ON) foo;").is_full()); +} + +#[test] +fn vacuum_full_1_is_full() { + assert!(extract_vacuum("VACUUM (FULL 1) foo;").is_full()); +} + +#[test] +fn vacuum_no_full_is_not_full() { + assert!(!extract_vacuum("VACUUM foo;").is_full()); +} + +#[test] +fn vacuum_other_option_is_not_full() { + assert!(!extract_vacuum("VACUUM (FREEZE) foo;").is_full()); +} + +#[test] +fn vacuum_full_false_is_not_full() { + assert!(!extract_vacuum("VACUUM (FULL FALSE) foo;").is_full()); +} + +#[test] +fn vacuum_full_off_is_not_full() { + assert!(!extract_vacuum("VACUUM (FULL OFF) foo;").is_full()); +} + +#[test] +fn vacuum_full_0_is_not_full() { + assert!(!extract_vacuum("VACUUM (FULL 0) foo;").is_full()); +} diff --git a/crates/squawk_syntax/src/postgresql.ungram b/crates/squawk_syntax/src/postgresql.ungram index 3a2b7ee9f..e9a2f1f2a 100644 --- a/crates/squawk_syntax/src/postgresql.ungram +++ b/crates/squawk_syntax/src/postgresql.ungram @@ -2289,7 +2289,11 @@ Cluster = 'cluster' ('verbose' | OptionItemList)? Path? - UsingMethod? + (UsingMethod | OnPath)? + +OnPath = + 'on' + Path Repack = 'repack' diff --git a/docs/docs/prefer-repack.md b/docs/docs/prefer-repack.md new file mode 100644 index 000000000..8c1388bff --- /dev/null +++ b/docs/docs/prefer-repack.md @@ -0,0 +1,34 @@ +--- +id: prefer-repack +title: prefer-repack +--- + +## problem + +`vacuum full` and `cluster` both require an `ACCESS EXCLUSIVE` lock which blocks reads and writes to the table. + +```sql +-- blocks reads and writes +vacuum full t; + +-- blocks reads and writes +cluster t using t_pkey; +``` + +## solution + +As of Postgres 19, use the `repack` command, which replaces `vacuum` and `cluster` and adds a `concurrently` option which doesn't require an `ACCESS EXCLUSIVE` lock. + +```sql +-- repack without ordering (equivalent to vacuum full) +repack (concurrently) t; + +-- repack with ordering (equivalent to cluster) +repack (concurrently) t using index t_pkey; +``` + +## links + +- [PostgreSQL's Docs on `repack`](https://www.postgresql.org/docs/devel/sql-repack.html) +- [PostgreSQL 19: The "repack" command](https://www.dbi-services.com/blog/postgresql-19-the-repack-command/) +- [River Queue's Post on Repack Concurrently](https://riverqueue.com/blog/repack-concurrently) diff --git a/docs/sidebars.js b/docs/sidebars.js index 5d7b13282..8486312b9 100644 --- a/docs/sidebars.js +++ b/docs/sidebars.js @@ -39,6 +39,7 @@ module.exports = { "identifier-too-long", "require-concurrent-partition-detach", "require-concurrent-reindex", + "prefer-repack", // xtask:new-rule:error-name ], }, diff --git a/docs/src/pages/index.js b/docs/src/pages/index.js index 5f48116cc..a55e6c38d 100644 --- a/docs/src/pages/index.js +++ b/docs/src/pages/index.js @@ -227,6 +227,11 @@ const rules = [ tags: ["locking"], description: "Prevent blocking reads/writes to table while index is reindexed.", }, + { + name: "prefer-repack", + tags: ["locking"], + description: "Prevent blocking reads/writes to table when rebuilding.", + }, // xtask:new-rule:rule-doc-meta ] From e58bb580882cf4a870961ba602ff3f0d01b6de79 Mon Sep 17 00:00:00 2001 From: Steve Dignam Date: Fri, 1 May 2026 23:19:43 -0400 Subject: [PATCH 2/2] fix --- .../tests/snapshots/tests__cluster_ok.snap | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/crates/squawk_parser/tests/snapshots/tests__cluster_ok.snap b/crates/squawk_parser/tests/snapshots/tests__cluster_ok.snap index 2b0fccf24..a314f9666 100644 --- a/crates/squawk_parser/tests/snapshots/tests__cluster_ok.snap +++ b/crates/squawk_parser/tests/snapshots/tests__cluster_ok.snap @@ -99,16 +99,17 @@ SOURCE_FILE NAME_REF IDENT "f" WHITESPACE " " - ON_KW "on" - WHITESPACE " " - PATH + ON_PATH + ON_KW "on" + WHITESPACE " " PATH + PATH + PATH_SEGMENT + NAME_REF + IDENT "foo" + DOT "." PATH_SEGMENT NAME_REF - IDENT "foo" - DOT "." - PATH_SEGMENT - NAME_REF - IDENT "bar" + IDENT "bar" SEMICOLON ";" WHITESPACE "\n\n"