Issue-338: Adjust handling of ban-concurrent-index-creation-in-transa…

…ction (#339) This changes the logic of `ban-concurrent-index-creation-in-transaction` when `assume_in_transaction` is set. Tools like `golang-migrate` always wrap migrations in transactions but support individual statements like `CREATE INDEX CONCURRENTLY` by not wrapping them in transactions. To support this, this commit alters the logic of `ban-concurrent-index-creation-in-transaction` to allow a standalone index create command even when `assume_in_transaction` is `true`.
sbdchd · Jan 13, 2024 · 7de8133 · 7de8133
1 parent 5e0b0a9
commit 7de8133
Show file tree

Hide file tree

Showing 9 changed files with 41 additions and 9 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -7,6 +7,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+## v0.28.0 - 2024-01-12
+
+### Changed
+
+- add exceptions for `ban-concurrent-index-creation-in-transaction` to handle golang-migrate. Thanks @janrueth! (#339)
+
 ## v0.27.0 - 2024-01-11
 
 ### Added

diff --git a/Cargo.lock b/Cargo.lock
diff --git a/cli/Cargo.toml b/cli/Cargo.toml
@@ -1,6 +1,6 @@
 [package]
 name = "squawk"
-version = "0.27.0"
+version = "0.28.0"
 authors = ["Steve Dignam <[email protected]>"]
 edition = "2018"
 license = "GPL-3.0"

diff --git a/docs/docs/ban-concurrent-index-creation-in-transaction.md b/docs/docs/ban-concurrent-index-creation-in-transaction.md
@@ -11,7 +11,8 @@ https://www.postgresql.org/docs/current/sql-createindex.html#SQL-CREATEINDEX-CON
 
 ## solution
 
-Remove surrounding transaction markers if any, and check that the `CREATE INDEX` command is not implicitly wrapped in a transaction.
+Remove surrounding transaction markers if any.
+For migrations that are implicitly wrapped in a transaction, ensure that the `CREATE INDEX` command is the only command in the migration to allow migration tool to detect that no transaction is needed.
 
 Instead of:
 
@@ -101,6 +102,8 @@ def schema_downgrades():
     # <any other downgrade commands>
 ```
 
+`golang-migrate` wraps migrations in transactions but is clever enough to perform the migration if the migration file does not contain further commands next to the `CREATE INDEX` command.
+
 ## links
 
 https://www.postgresql.org/docs/current/sql-createindex.html#SQL-CREATEINDEX-CONCURRENTLY
diff --git a/flake.nix b/flake.nix
@@ -18,7 +18,7 @@
           {
             squawk = final.rustPlatform.buildRustPackage {
               pname = "squawk";
-              version = "0.27.0";
+              version = "0.28.0";
 
               cargoLock = {
                 lockFile = ./Cargo.lock;

diff --git a/linter/src/rules/ban_concurrent_index_creation_in_transaction.rs b/linter/src/rules/ban_concurrent_index_creation_in_transaction.rs
@@ -23,6 +23,10 @@ pub fn ban_concurrent_index_creation_in_transaction(
             }
             Stmt::IndexStmt(stmt) => {
                 if stmt.concurrent && in_transaction {
+                    if assume_in_transaction && tree.len() == 1 {
+                        // Migration tools should not require the transaction here so this is usually safe
+                        continue;
+                    }
                     errs.push(RuleViolation::new(
                         RuleViolationKind::BanConcurrentIndexCreationInTransaction,
                         raw_stmt.into(),
@@ -87,16 +91,29 @@ mod test_rules {
     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");
+  CREATE UNIQUE INDEX CONCURRENTLY "field_name_idx" ON "table_name" ("field_name");
+  ALTER TABLE "table_name" ADD CONSTRAINT "field_name_id" UNIQUE USING INDEX "field_name_idx";
   "#;
 
         assert_debug_snapshot!(lint_sql_assuming_in_transaction(bad_sql));
 
         let ok_sql = r#"
-  -- run outside a transaction
+  -- run index creation in a standalone migration
+  CREATE UNIQUE INDEX CONCURRENTLY "field_name_idx" ON "table_name" ("field_name");
+  "#;
+        assert_debug_snapshot!(lint_sql_assuming_in_transaction(ok_sql));
+    }
+
+    #[test]
+    fn test_adding_index_concurrently_in_transaction_with_assume_in_transaction_but_outside() {
+        let ok_sql = r#"
+  -- the following will work too
   COMMIT;
-  CREATE INDEX CONCURRENTLY "field_name_idx" ON "table_name" ("field_name");
+  CREATE UNIQUE INDEX CONCURRENTLY "field_name_idx" ON "table_name" ("field_name");
+  BEGIN;
+  ALTER TABLE "table_name" ADD CONSTRAINT "field_name_id" UNIQUE USING INDEX "field_name_idx";
   "#;
+
         assert_debug_snapshot!(lint_sql_assuming_in_transaction(ok_sql));
     }
 }
diff --git a/...ion__test_rules__adding_index_concurrently_in_transaction_with_assume_in_transaction.snap b/...ion__test_rules__adding_index_concurrently_in_transaction_with_assume_in_transaction.snap
@@ -8,7 +8,7 @@ expression: lint_sql_assuming_in_transaction(bad_sql)
         span: Span {
             start: 0,
             len: Some(
-                92,
+                99,
             ),
         },
         messages: [

diff --git a/...les__adding_index_concurrently_in_transaction_with_assume_in_transaction_but_outside.snap b/...les__adding_index_concurrently_in_transaction_with_assume_in_transaction_but_outside.snap
@@ -0,0 +1,6 @@
+---
+source: linter/src/rules/ban_concurrent_index_creation_in_transaction.rs
+expression: lint_sql_assuming_in_transaction(ok_sql)
+
+---
+[]
diff --git a/package.json b/package.json
@@ -1,6 +1,6 @@
 {
   "name": "squawk-cli",
-  "version": "0.27.0",
+  "version": "0.28.0",
   "description": "linter for PostgreSQL, focused on migrations",
   "repository": "[email protected]:sbdchd/squawk.git",
   "author": "Steve Dignam <[email protected]>",