linter: new rule: require-concurrent-reindex

crates/squawk_linter/src/lib.rs

@@ -55,6 +55,7 @@ use rules::renaming_table;
 use rules::require_concurrent_index_creation;
 use rules::require_concurrent_index_deletion;
 use rules::require_concurrent_partition_detach;
+use rules::require_concurrent_reindex;
 use rules::require_enum_value_ordering;
 use rules::require_table_schema;
 use rules::require_timeout_settings;
@@ -98,6 +99,7 @@ pub enum Rule {
     RequireTableSchema,
     IdentifierTooLong,
     RequireConcurrentPartitionDetach,
+    RequireConcurrentReindex,
     // xtask:new-rule:error-name
 }
 
@@ -152,6 +154,7 @@ impl TryFrom<&str> for Rule {
             "require-table-schema" => Ok(Rule::RequireTableSchema),
             "identifier-too-long" => Ok(Rule::IdentifierTooLong),
             "require-concurrent-partition-detach" => Ok(Rule::RequireConcurrentPartitionDetach),
+            "require-concurrent-reindex" => Ok(Rule::RequireConcurrentReindex),
             // xtask:new-rule:str-name
             _ => Err(format!("Unknown violation name: {s}")),
         }
@@ -218,6 +221,7 @@ impl fmt::Display for Rule {
             Rule::RequireTableSchema => "require-table-schema",
             Rule::IdentifierTooLong => "identifier-too-long",
             Rule::RequireConcurrentPartitionDetach => "require-concurrent-partition-detach",
+            Rule::RequireConcurrentReindex => "require-concurrent-reindex",
             // xtask:new-rule:variant-to-name
         };
         write!(f, "{val}")
@@ -457,6 +461,9 @@ impl Linter {
         if self.rules.contains(&Rule::RequireConcurrentPartitionDetach) {
             require_concurrent_partition_detach(self, file);
         }
+        if self.rules.contains(&Rule::RequireConcurrentReindex) {
+            require_concurrent_reindex(self, file);
+        }
         // xtask:new-rule:rule-call
 
         // locate any ignores in the file

crates/squawk_linter/src/rules/mod.rs

@@ -28,6 +28,7 @@ pub(crate) mod renaming_table;
 pub(crate) mod require_concurrent_index_creation;
 pub(crate) mod require_concurrent_index_deletion;
 pub(crate) mod require_concurrent_partition_detach;
+pub(crate) mod require_concurrent_reindex;
 pub(crate) mod require_enum_value_ordering;
 pub(crate) mod require_table_schema;
 pub(crate) mod require_timeout_settings;
@@ -64,6 +65,7 @@ pub(crate) use renaming_table::renaming_table;
 pub(crate) use require_concurrent_index_creation::require_concurrent_index_creation;
 pub(crate) use require_concurrent_index_deletion::require_concurrent_index_deletion;
 pub(crate) use require_concurrent_partition_detach::require_concurrent_partition_detach;
+pub(crate) use require_concurrent_reindex::require_concurrent_reindex;
 pub(crate) use require_enum_value_ordering::require_enum_value_ordering;
 pub(crate) use require_table_schema::require_table_schema;
 pub(crate) use require_timeout_settings::require_timeout_settings;

crates/squawk_linter/src/rules/require_concurrent_reindex.rs

@@ -0,0 +1,155 @@
+use squawk_syntax::{
+    Parse, SourceFile,
+    ast::{self, AstNode},
+};
+
+use crate::{Edit, Fix, Linter, Rule, Violation};
+
+fn concurrently_fix(reindex: &ast::Reindex) -> Option<Fix> {
+    let kw_token = reindex
+        .table_token()
+        .or_else(|| reindex.index_token())
+        .or_else(|| reindex.schema_token())
+        .or_else(|| reindex.database_token())?;
+    let at = kw_token.text_range().end();
+    let edit = Edit::insert(" concurrently", at);
+    Some(Fix::new("Add `concurrently`", vec![edit]))
+}
+
+pub(crate) fn require_concurrent_reindex(ctx: &mut Linter, parse: &Parse<SourceFile>) {
+    for stmt in parse.tree().stmts() {
+        if let ast::Stmt::Reindex(reindex) = stmt {
+            // REINDEX SYSTEM does not support CONCURRENTLY
+            if reindex.system_token().is_some() {
+                continue;
+            }
+            if reindex.concurrently_token().is_none() {
+                let fix = concurrently_fix(&reindex);
+                ctx.report(
+                    Violation::for_node(
+                        Rule::RequireConcurrentReindex,
+                        "Reindexing a table or index without `concurrently` blocks reads and writes."
+                            .into(),
+                        reindex.syntax(),
+                    )
+                    .help("Use `concurrently` to avoid blocking reads and writes.")
+                    .fix(fix),
+                );
+            }
+        }
+    }
+}
+
+#[cfg(test)]
+mod test {
+    use insta::assert_snapshot;
+
+    use crate::{
+        Rule,
+        test_utils::{fix_sql, lint_errors, lint_ok},
+    };
+
+    fn fix(sql: &str) -> String {
+        fix_sql(sql, Rule::RequireConcurrentReindex)
+    }
+
+    #[test]
+    fn reindex_table_no_concurrently_err() {
+        let sql = "REINDEX TABLE foo;";
+        assert_snapshot!(lint_errors(sql, Rule::RequireConcurrentReindex), @"
+        warning[require-concurrent-reindex]: Reindexing a table or index without `concurrently` blocks reads and writes.
+          ╭▸ 
+        1 │ REINDEX TABLE foo;
+          │ ━━━━━━━━━━━━━━━━━
+          │
+          ├ help: Use `concurrently` to avoid blocking reads and writes.
+          ╭╴
+        1 │ REINDEX TABLE concurrently foo;
+          ╰╴              ++++++++++++
+        ");
+    }
+
+    #[test]
+    fn reindex_index_no_concurrently_err() {
+        let sql = "REINDEX INDEX foo;";
+        assert_snapshot!(lint_errors(sql, Rule::RequireConcurrentReindex), @"
+        warning[require-concurrent-reindex]: Reindexing a table or index without `concurrently` blocks reads and writes.
+          ╭▸ 
+        1 │ REINDEX INDEX foo;
+          │ ━━━━━━━━━━━━━━━━━
+          │
+          ├ help: Use `concurrently` to avoid blocking reads and writes.
+          ╭╴
+        1 │ REINDEX INDEX concurrently foo;
+          ╰╴              ++++++++++++
+        ");
+    }
+
+    #[test]
+    fn reindex_schema_no_concurrently_err() {
+        let sql = "REINDEX SCHEMA foo;";
+        assert_snapshot!(lint_errors(sql, Rule::RequireConcurrentReindex), @"
+        warning[require-concurrent-reindex]: Reindexing a table or index without `concurrently` blocks reads and writes.
+          ╭▸ 
+        1 │ REINDEX SCHEMA foo;
+          │ ━━━━━━━━━━━━━━━━━━
+          │
+          ├ help: Use `concurrently` to avoid blocking reads and writes.
+          ╭╴
+        1 │ REINDEX SCHEMA concurrently foo;
+          ╰╴               ++++++++++++
+        ");
+    }
+
+    #[test]
+    fn reindex_database_no_concurrently_err() {
+        let sql = "REINDEX DATABASE foo;";
+        assert_snapshot!(lint_errors(sql, Rule::RequireConcurrentReindex), @"
+        warning[require-concurrent-reindex]: Reindexing a table or index without `concurrently` blocks reads and writes.
+          ╭▸ 
+        1 │ REINDEX DATABASE foo;
+          │ ━━━━━━━━━━━━━━━━━━━━
+          │
+          ├ help: Use `concurrently` to avoid blocking reads and writes.
+          ╭╴
+        1 │ REINDEX DATABASE concurrently foo;
+          ╰╴                 ++++++++++++
+        ");
+    }
+
+    #[test]
+    fn reindex_table_concurrently_ok() {
+        let sql = "REINDEX TABLE CONCURRENTLY foo;";
+        lint_ok(sql, Rule::RequireConcurrentReindex);
+    }
+
+    #[test]
+    fn reindex_index_concurrently_ok() {
+        let sql = "REINDEX INDEX CONCURRENTLY foo;";
+        lint_ok(sql, Rule::RequireConcurrentReindex);
+    }
+
+    #[test]
+    fn reindex_options_concurrently_ok() {
+        let sql = "REINDEX (CONCURRENTLY) TABLE foo;";
+        lint_ok(sql, Rule::RequireConcurrentReindex);
+    }
+
+    #[test]
+    fn reindex_system_ok() {
+        let sql = "REINDEX SYSTEM mydb;";
+        lint_ok(sql, Rule::RequireConcurrentReindex);
+    }
+
+    #[test]
+    fn fix_reindex_table() {
+        let sql = "REINDEX TABLE foo;";
+        assert_snapshot!(fix(sql), @"REINDEX TABLE concurrently foo;");
+    }
+
+    #[test]
+    fn fix_reindex_index() {
+        let sql = "REINDEX INDEX foo;";
+        assert_snapshot!(fix(sql), @"REINDEX INDEX concurrently foo;");
+    }
+}

crates/squawk_syntax/src/postgresql.ungram

@@ -2953,7 +2953,7 @@ SetTransaction =
   )
 
 Reindex =
-  'reindex' ('table' | 'index' | 'schema' | 'database' | 'system')? Path?
+  'reindex' ('table' | 'index' | 'schema' | 'database' | 'system')? 'concurrently'? Path?
 
 CreateView =
   'create' OrReplace? Persistence? 'recursive'? 'view' Path ColumnList?

crates/xtask/src/new_rule.rs

@@ -126,7 +126,7 @@ fn update_lib(name: &str) -> Result<()> {
             "// xtask:new-rule:rule-call",
             format!(
                 r#"if self.rules.contains(&Rule::{name_pascal}) {{
-            {name_snake}(self, &file);
+            {name_snake}(self, file);
         }}
         "#
             ),

docs/docs/require-concurrent-reindex.md

@@ -0,0 +1,32 @@
+---
+id: require-concurrent-reindex
+title: require-concurrent-reindex
+---
+
+## problem
+
+`REINDEX` without `CONCURRENTLY` requires an `ACCESS EXCLUSIVE` lock on the index's table, blocking reads and writes.
+
+## solution
+
+Use `REINDEX ... CONCURRENTLY`.
+
+Instead of:
+
+```sql
+-- blocks reads and writes
+REINDEX TABLE foo;
+REINDEX INDEX foo_idx;
+```
+
+Use:
+
+```sql
+-- avoids blocking reads and writes
+REINDEX TABLE CONCURRENTLY foo;
+REINDEX INDEX CONCURRENTLY foo_idx;
+```
+
+## links
+
+- <https://www.postgresql.org/docs/current/sql-reindex.html#SQL-REINDEX-CONCURRENTLY>

docs/sidebars.js

@@ -38,6 +38,7 @@ module.exports = {
       "require-table-schema",
       "identifier-too-long",
       "require-concurrent-partition-detach",
+      "require-concurrent-reindex",
       // xtask:new-rule:error-name
     ],
   },

docs/src/pages/index.js

@@ -222,6 +222,11 @@ const rules = [
     tags: ["schema", "locking"],
     description: "Prevent blocking reads/writes to table while partition is detached.",
   },
+  {
+    name: "require-concurrent-reindex",
+    tags: ["locking"],
+    description: "Prevent blocking reads/writes to table while index is reindexed.",
+  },
   // xtask:new-rule:rule-doc-meta
 ]