Add rule to prevent concurrent index creation inside transactions (#335)

Add a rule that warns about creating an index with the `CONCURRENTLY` option inside a transaction, since that's not allowed in Postgres. This will resolve #332

CHANGELOG.md

@@ -7,6 +7,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+## v0.27.0 - 2024-01-11
+
+### Added
+
+- added `ban-concurrent-index-creation-in-transaction` rule. Thanks @alixlahuec! (#335)
+
 ## v0.26.0 - 2023-12-12
 
 ### Changed

cli/Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "squawk"
-version = "0.26.0"
+version = "0.27.0"
 authors = ["Steve Dignam <[email protected]>"]
 edition = "2018"
 license = "GPL-3.0"

docs/docs/ban-concurrent-index-creation-in-transaction.md

@@ -0,0 +1,106 @@
+---
+id: ban-concurrent-index-creation-in-transaction
+title: ban-concurrent-index-creation-in-transaction
+---
+
+## problem
+
+While regular index creation can happen inside a transaction, this is not allowed when the `CONCURRENTLY` option is used.
+
+https://www.postgresql.org/docs/current/sql-createindex.html#SQL-CREATEINDEX-CONCURRENTLY
+
+## solution
+
+Remove surrounding transaction markers if any, and check that the `CREATE INDEX` command is not implicitly wrapped in a transaction.
+
+Instead of:
+
+```sql
+BEGIN;
+-- <any other commands being run transactionally>
+CREATE INDEX CONCURRENTLY "email_idx" ON "app_user" ("email");
+COMMIT;
+```
+
+Use:
+
+```sql
+BEGIN;
+-- <any other commands being run transactionally>
+COMMIT;
+
+CREATE INDEX CONCURRENTLY "email_idx" ON "app_user" ("email");
+```
+
+If you use a migration tool, it may be configured to automatically wrap commands in transactions; if that's the case, check if it supports running commands in a non-transactional context.
+For example, with `alembic`:
+
+```python
+# migrations/*.py
+from alembic import op
+
+def schema_upgrades():
+    # <any other commands being run transactionally>
+
+    # alembic allows non-transactional operations using autocommit
+    with op.get_context().autocommit_block():
+        op.create_index(
+            "email_idx",
+            "app_user",
+            ["email"],
+            schema="app",
+            unique=False,
+            postgresql_concurrently=True,
+        )
+
+    # <any other commands being run transactionally>
+
+def schema_downgrades():
+    # <any other downgrade commands>
+
+    op.drop_index(
+        "email_idx",
+        schema="app",
+    )
+
+    # <any other downgrade commands>
+```
+
+Or alternatively:
+
+```python
+# migrations/*.py
+from alembic import op
+
+def schema_upgrades():
+    # <any other commands being run transactionally>
+
+    # you can also execute BEGIN/COMMIT to delineate transactions
+    op.execute("COMMIT;")
+    op.execute("SET statement_timeout = 0;")
+    op.create_index(
+        "email_idx",
+        "app_user",
+        ["email"],
+        schema="app",
+        unique=False,
+        postgresql_concurrently=True,
+    )
+
+    op.execute("BEGIN;")
+    # <any other commands being run transactionally>
+
+def schema_downgrades():
+    # <any other downgrade commands>
+
+    op.drop_index(
+        "email_idx",
+        schema="app",
+    )
+
+    # <any other downgrade commands>
+```
+
+## links
+
+https://www.postgresql.org/docs/current/sql-createindex.html#SQL-CREATEINDEX-CONCURRENTLY

docs/sidebars.js

@@ -27,6 +27,7 @@ module.exports = {
       "require-concurrent-index-creation",
       "require-concurrent-index-deletion",
       "transaction-nesting",
+      "ban-concurrent-index-creation-in-transaction",
       // generator::new-rule-above
     ],
   },

docs/src/pages/index.js

@@ -178,6 +178,11 @@ const rules = [
     tags: ["locking"],
     description: "Ensure migrations use transactions correctly.",
   },
+  {
+    name: "ban-concurrent-index-creation-in-transaction",
+    tags: ["schema"],
+    description: "Prevent forbidden use of transactions during concurrent index creation.",
+  },
   // generator::new-rule-above
 ]
 

flake.nix

@@ -18,7 +18,7 @@
           {
             squawk = final.rustPlatform.buildRustPackage {
               pname = "squawk";
-              version = "0.26.0";
+              version = "0.27.0";
 
               cargoLock = {
                 lockFile = ./Cargo.lock;

linter/src/lib.rs

@@ -9,6 +9,7 @@ extern crate lazy_static;
 
 use crate::errors::CheckSqlError;
 use crate::rules::adding_required_field;
+use crate::rules::ban_concurrent_index_creation_in_transaction;
 use crate::rules::ban_drop_not_null;
 use crate::rules::prefer_big_int;
 use crate::rules::prefer_identity;
@@ -106,6 +107,18 @@ lazy_static! {
             ),
         ]
     },
+    SquawkRule {
+        name: RuleViolationKind::BanConcurrentIndexCreationInTransaction,
+        func: ban_concurrent_index_creation_in_transaction,
+        messages: vec![
+            ViolationMessage::Note(
+                "Concurrent index creation is not allowed inside a transaction.".into()
+            ),
+            ViolationMessage::Help(
+                "Build the index outside any transactions.".into()
+            ),
+        ],
+    },
     SquawkRule {
         name: RuleViolationKind::BanDropColumn,
         func: ban_drop_column,

linter/src/rules/ban_concurrent_index_creation_in_transaction.rs

@@ -0,0 +1,102 @@
+use crate::versions::Version;
+use crate::violations::{RuleViolation, RuleViolationKind};
+
+use squawk_parser::ast::{RawStmt, Stmt, TransactionStmtKind};
+
+#[must_use]
+pub fn ban_concurrent_index_creation_in_transaction(
+    tree: &[RawStmt],
+    _pg_version: Option<Version>,
+    assume_in_transaction: bool,
+) -> Vec<RuleViolation> {
+    let mut in_transaction = assume_in_transaction;
+    let mut errs = vec![];
+    for raw_stmt in tree {
+        match &raw_stmt.stmt {
+            Stmt::TransactionStmt(stmt) => {
+                if stmt.kind == TransactionStmtKind::Begin && !in_transaction {
+                    in_transaction = true;
+                }
+                if stmt.kind == TransactionStmtKind::Commit {
+                    in_transaction = false;
+                }
+            }
+            Stmt::IndexStmt(stmt) => {
+                if stmt.concurrent && in_transaction {
+                    errs.push(RuleViolation::new(
+                        RuleViolationKind::BanConcurrentIndexCreationInTransaction,
+                        raw_stmt.into(),
+                        None,
+                    ));
+                }
+            }
+            _ => continue,
+        }
+    }
+    errs
+}
+
+#[cfg(test)]
+mod test_rules {
+    use insta::assert_debug_snapshot;
+
+    use crate::{
+        check_sql_with_rule,
+        violations::{RuleViolation, RuleViolationKind},
+    };
+
+    fn lint_sql(sql: &str) -> Vec<RuleViolation> {
+        check_sql_with_rule(
+            sql,
+            &RuleViolationKind::BanConcurrentIndexCreationInTransaction,
+            None,
+            false,
+        )
+        .unwrap()
+    }
+
+    fn lint_sql_assuming_in_transaction(sql: &str) -> Vec<RuleViolation> {
+        check_sql_with_rule(
+            sql,
+            &RuleViolationKind::BanConcurrentIndexCreationInTransaction,
+            None,
+            true,
+        )
+        .unwrap()
+    }
+
+    #[test]
+    fn test_adding_index_concurrently_in_transaction() {
+        let bad_sql = r#"
+  -- instead of
+  BEGIN;
+  CREATE INDEX CONCURRENTLY "field_name_idx" ON "table_name" ("field_name");
+  COMMIT;
+  "#;
+
+        assert_debug_snapshot!(lint_sql(bad_sql));
+
+        let ok_sql = r#"
+  -- run outside a transaction
+  CREATE INDEX CONCURRENTLY "field_name_idx" ON "table_name" ("field_name");
+  "#;
+        assert_debug_snapshot!(lint_sql(ok_sql));
+    }
+
+    #[test]
+    fn test_adding_index_concurrently_in_transaction_with_assume_in_transaction() {
+        let bad_sql = r#"
+  -- instead of
+  CREATE INDEX CONCURRENTLY "field_name_idx" ON "table_name" ("field_name");
+  "#;
+
+        assert_debug_snapshot!(lint_sql_assuming_in_transaction(bad_sql));
+
+        let ok_sql = r#"
+  -- run outside a transaction
+  COMMIT;
+  CREATE INDEX CONCURRENTLY "field_name_idx" ON "table_name" ("field_name");
+  "#;
+        assert_debug_snapshot!(lint_sql_assuming_in_transaction(ok_sql));
+    }
+}

linter/src/rules/mod.rs

@@ -50,3 +50,5 @@ pub mod transaction_nesting;
 pub use transaction_nesting::*;
 pub mod adding_required_field;
 pub use adding_required_field::*;
+pub mod ban_concurrent_index_creation_in_transaction;
+pub use ban_concurrent_index_creation_in_transaction::*;

linter/src/rules/snapshots/squawk_linter__rules__ban_concurrent_index_creation_in_transaction__test_rules__adding_index_concurrently_in_transaction-2.snap

@@ -0,0 +1,5 @@
+---
+source: linter/src/rules/ban_concurrent_index_creation_in_transaction.rs
+expression: lint_sql(ok_sql)
+---
+[]

linter/src/rules/snapshots/squawk_linter__rules__ban_concurrent_index_creation_in_transaction__test_rules__adding_index_concurrently_in_transaction.snap

@@ -0,0 +1,23 @@
+---
+source: linter/src/rules/ban_concurrent_index_creation_in_transaction.rs
+expression: lint_sql(bad_sql)
+---
+[
+    RuleViolation {
+        kind: BanConcurrentIndexCreationInTransaction,
+        span: Span {
+            start: 25,
+            len: Some(
+                76,
+            ),
+        },
+        messages: [
+            Note(
+                "Concurrent index creation is not allowed inside a transaction.",
+            ),
+            Help(
+                "Build the index outside any transactions.",
+            ),
+        ],
+    },
+]

linter/src/rules/snapshots/squawk_linter__rules__ban_concurrent_index_creation_in_transaction__test_rules__adding_index_concurrently_in_transaction_with_assume_in_transaction-2.snap

@@ -0,0 +1,5 @@
+---
+source: linter/src/rules/ban_concurrent_index_creation_in_transaction.rs
+expression: lint_sql_assuming_in_transaction(ok_sql)
+---
+[]

linter/src/rules/snapshots/squawk_linter__rules__ban_concurrent_index_creation_in_transaction__test_rules__adding_index_concurrently_in_transaction_with_assume_in_transaction.snap

@@ -0,0 +1,23 @@
+---
+source: linter/src/rules/ban_concurrent_index_creation_in_transaction.rs
+expression: lint_sql_assuming_in_transaction(bad_sql)
+---
+[
+    RuleViolation {
+        kind: BanConcurrentIndexCreationInTransaction,
+        span: Span {
+            start: 0,
+            len: Some(
+                92,
+            ),
+        },
+        messages: [
+            Note(
+                "Concurrent index creation is not allowed inside a transaction.",
+            ),
+            Help(
+                "Build the index outside any transactions.",
+            ),
+        ],
+    },
+]

linter/src/snapshots/squawk_linter__test_rules__rule_names_debug_snap.snap

@@ -9,6 +9,7 @@ expression: rule_names
     "adding-required-field",
     "adding-serial-primary-key-field",
     "ban-char-field",
+    "ban-concurrent-index-creation-in-transaction",
     "ban-drop-column",
     "ban-drop-database",
     "ban-drop-not-null",

linter/src/snapshots/squawk_linter__test_rules__rule_names_display_snap.snap

@@ -8,6 +8,7 @@ adding-not-nullable-field
 adding-required-field
 adding-serial-primary-key-field
 ban-char-field
+ban-concurrent-index-creation-in-transaction
 ban-drop-column
 ban-drop-database
 ban-drop-not-null

linter/src/violations.rs

@@ -59,6 +59,8 @@ pub enum RuleViolationKind {
     TransactionNesting,
     #[serde(rename = "adding-required-field")]
     AddingRequiredField,
+    #[serde(rename = "ban-concurrent-index-creation-in-transaction")]
+    BanConcurrentIndexCreationInTransaction,
     // generator::new-rule-above
 }
 

package.json

@@ -1,6 +1,6 @@
 {
   "name": "squawk-cli",
-  "version": "0.26.0",
+  "version": "0.27.0",
   "description": "linter for PostgreSQL, focused on migrations",
   "repository": "[email protected]:sbdchd/squawk.git",
   "author": "Steve Dignam <[email protected]>",