From 7de8133783e16d4505ae51ea6d241c13f45f71bc Mon Sep 17 00:00:00 2001 From: Jan <1324490+janrueth@users.noreply.github.com> Date: Sat, 13 Jan 2024 01:38:59 +0100 Subject: [PATCH] Issue-338: Adjust handling of ban-concurrent-index-creation-in-transaction (#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`. --- CHANGELOG.md | 6 +++++ Cargo.lock | 2 +- cli/Cargo.toml | 2 +- ...oncurrent-index-creation-in-transaction.md | 5 +++- flake.nix | 2 +- ...oncurrent_index_creation_in_transaction.rs | 23 ++++++++++++++++--- ...ransaction_with_assume_in_transaction.snap | 2 +- ...ith_assume_in_transaction_but_outside.snap | 6 +++++ package.json | 2 +- 9 files changed, 41 insertions(+), 9 deletions(-) create mode 100644 linter/src/rules/snapshots/squawk_linter__rules__ban_concurrent_index_creation_in_transaction__test_rules__adding_index_concurrently_in_transaction_with_assume_in_transaction_but_outside.snap diff --git a/CHANGELOG.md b/CHANGELOG.md index 195d08b6..32fcbdb0 100644 --- 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 index a3e5361b..7389d223 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1595,7 +1595,7 @@ checksum = "6e63cff320ae2c57904679ba7cb63280a3dc4613885beafb148ee7bf9aa9042d" [[package]] name = "squawk" -version = "0.27.0" +version = "0.28.0" dependencies = [ "atty", "base64 0.12.3", diff --git a/cli/Cargo.toml b/cli/Cargo.toml index ee62fe4f..ad827431 100644 --- 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 "] 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 index 06a82acc..20fcbb99 100644 --- 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(): # ``` +`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 \ No newline at end of file diff --git a/flake.nix b/flake.nix index 3b6db310..47ecb351 100644 --- 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 index e8421223..28aea18c 100644 --- 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/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 b/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 index 6072d9c8..7a497e46 100644 --- a/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 +++ b/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 @@ -8,7 +8,7 @@ expression: lint_sql_assuming_in_transaction(bad_sql) span: Span { start: 0, len: Some( - 92, + 99, ), }, messages: [ diff --git a/linter/src/rules/snapshots/squawk_linter__rules__ban_concurrent_index_creation_in_transaction__test_rules__adding_index_concurrently_in_transaction_with_assume_in_transaction_but_outside.snap b/linter/src/rules/snapshots/squawk_linter__rules__ban_concurrent_index_creation_in_transaction__test_rules__adding_index_concurrently_in_transaction_with_assume_in_transaction_but_outside.snap new file mode 100644 index 00000000..7b120670 --- /dev/null +++ b/linter/src/rules/snapshots/squawk_linter__rules__ban_concurrent_index_creation_in_transaction__test_rules__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 index 1f3cdca9..e3dee940 100644 --- 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": "git@github.com:sbdchd/squawk.git", "author": "Steve Dignam ",