linter: new rule: prefer repack

Author: sbdchd

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);
         }

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;

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<Fix> {
+    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<Fix> {
+    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<SourceFile>) {
+    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);");
+    }
+}

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::<Vec<_>>();
@@ -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);

crates/squawk_parser/src/generated/syntax_kind.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();