diff --git a/CHANGELOG.md b/CHANGELOG.md index bc17d5ad..195d08b6 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.27.0 - 2024-01-11 + +### Added + +- added `ban-concurrent-index-creation-in-transaction` rule. Thanks @alixlahuec! (#335) + ## v0.26.0 - 2023-12-12 ### Changed diff --git a/Cargo.lock b/Cargo.lock index 89de50ce..a3e5361b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1595,7 +1595,7 @@ checksum = "6e63cff320ae2c57904679ba7cb63280a3dc4613885beafb148ee7bf9aa9042d" [[package]] name = "squawk" -version = "0.26.0" +version = "0.27.0" dependencies = [ "atty", "base64 0.12.3", diff --git a/cli/Cargo.toml b/cli/Cargo.toml index 4b006c8d..ee62fe4f 100644 --- a/cli/Cargo.toml +++ b/cli/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "squawk" -version = "0.26.0" +version = "0.27.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 new file mode 100644 index 00000000..06a82acc --- /dev/null +++ b/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; +-- +CREATE INDEX CONCURRENTLY "email_idx" ON "app_user" ("email"); +COMMIT; +``` + +Use: + +```sql +BEGIN; +-- +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(): + # + + # 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, + ) + + # + +def schema_downgrades(): + # + + op.drop_index( + "email_idx", + schema="app", + ) + + # +``` + +Or alternatively: + +```python +# migrations/*.py +from alembic import op + +def schema_upgrades(): + # + + # 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;") + # + +def schema_downgrades(): + # + + op.drop_index( + "email_idx", + schema="app", + ) + + # +``` + +## links + +https://www.postgresql.org/docs/current/sql-createindex.html#SQL-CREATEINDEX-CONCURRENTLY \ No newline at end of file diff --git a/docs/sidebars.js b/docs/sidebars.js index ba14340b..bbf7c784 100644 --- a/docs/sidebars.js +++ b/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 ], }, diff --git a/docs/src/pages/index.js b/docs/src/pages/index.js index f8672b73..81ee46ec 100644 --- a/docs/src/pages/index.js +++ b/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 ] diff --git a/flake.nix b/flake.nix index 7cc1c956..3b6db310 100644 --- a/flake.nix +++ b/flake.nix @@ -18,7 +18,7 @@ { squawk = final.rustPlatform.buildRustPackage { pname = "squawk"; - version = "0.26.0"; + version = "0.27.0"; cargoLock = { lockFile = ./Cargo.lock; diff --git a/linter/src/lib.rs b/linter/src/lib.rs index 4d63b997..ae4abaec 100644 --- a/linter/src/lib.rs +++ b/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, diff --git a/linter/src/rules/ban_concurrent_index_creation_in_transaction.rs b/linter/src/rules/ban_concurrent_index_creation_in_transaction.rs new file mode 100644 index 00000000..e8421223 --- /dev/null +++ b/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, + assume_in_transaction: bool, +) -> Vec { + 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 { + check_sql_with_rule( + sql, + &RuleViolationKind::BanConcurrentIndexCreationInTransaction, + None, + false, + ) + .unwrap() + } + + fn lint_sql_assuming_in_transaction(sql: &str) -> Vec { + 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)); + } +} diff --git a/linter/src/rules/mod.rs b/linter/src/rules/mod.rs index 97a5a164..8261b8f8 100644 --- a/linter/src/rules/mod.rs +++ b/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::*; diff --git a/linter/src/rules/snapshots/squawk_linter__rules__ban_concurrent_index_creation_in_transaction__test_rules__adding_index_concurrently_in_transaction-2.snap b/linter/src/rules/snapshots/squawk_linter__rules__ban_concurrent_index_creation_in_transaction__test_rules__adding_index_concurrently_in_transaction-2.snap new file mode 100644 index 00000000..20689329 --- /dev/null +++ b/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) +--- +[] diff --git a/linter/src/rules/snapshots/squawk_linter__rules__ban_concurrent_index_creation_in_transaction__test_rules__adding_index_concurrently_in_transaction.snap b/linter/src/rules/snapshots/squawk_linter__rules__ban_concurrent_index_creation_in_transaction__test_rules__adding_index_concurrently_in_transaction.snap new file mode 100644 index 00000000..f55b9641 --- /dev/null +++ b/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.", + ), + ], + }, +] 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-2.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-2.snap new file mode 100644 index 00000000..ae384db7 --- /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-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) +--- +[] 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 new file mode 100644 index 00000000..6072d9c8 --- /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.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.", + ), + ], + }, +] diff --git a/linter/src/snapshots/squawk_linter__test_rules__rule_names_debug_snap.snap b/linter/src/snapshots/squawk_linter__test_rules__rule_names_debug_snap.snap index 18f5a32a..83322f6d 100644 --- a/linter/src/snapshots/squawk_linter__test_rules__rule_names_debug_snap.snap +++ b/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", diff --git a/linter/src/snapshots/squawk_linter__test_rules__rule_names_display_snap.snap b/linter/src/snapshots/squawk_linter__test_rules__rule_names_display_snap.snap index aacc6475..12a30380 100644 --- a/linter/src/snapshots/squawk_linter__test_rules__rule_names_display_snap.snap +++ b/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 diff --git a/linter/src/violations.rs b/linter/src/violations.rs index 7aa3eb06..0a753ebd 100644 --- a/linter/src/violations.rs +++ b/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 } diff --git a/package.json b/package.json index 6688f7be..1f3cdca9 100644 --- a/package.json +++ b/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": "git@github.com:sbdchd/squawk.git", "author": "Steve Dignam ",