From 47684c62ff37f345b8cc983cd00f669c6134b757 Mon Sep 17 00:00:00 2001 From: psteinroe Date: Sun, 19 Oct 2025 12:26:26 +0200 Subject: [PATCH 1/8] feat(analyser): add serial column --- .gitignore | 3 +- crates/pgt_analyser/CONTRIBUTING.md | 52 +++++++ crates/pgt_analyser/src/lint/safety.rs | 3 +- .../src/lint/safety/add_serial_column.rs | 141 ++++++++++++++++++ crates/pgt_analyser/src/options.rs | 2 + .../specs/safety/addSerialColumn/basic.sql | 3 + .../safety/addSerialColumn/basic.sql.snap | 21 +++ .../safety/addSerialColumn/bigserial.sql | 3 + .../safety/addSerialColumn/bigserial.sql.snap | 21 +++ .../addSerialColumn/generated_stored.sql | 3 + .../addSerialColumn/generated_stored.sql.snap | 21 +++ .../safety/addSerialColumn/smallserial.sql | 3 + .../addSerialColumn/smallserial.sql.snap | 21 +++ .../addSerialColumn/valid_create_table.sql | 7 + .../valid_create_table.sql.snap | 16 ++ .../valid_generated_virtual.sql | 3 + .../valid_generated_virtual.sql.snap | 12 ++ .../addSerialColumn/valid_regular_column.sql | 3 + .../valid_regular_column.sql.snap | 12 ++ .../src/analyser/linter/rules.rs | 137 ++++++++++------- .../src/categories.rs | 1 + docs/reference/rule_sources.md | 1 + docs/reference/rules.md | 1 + docs/reference/rules/add-serial-column.md | 99 ++++++++++++ docs/schema.json | 11 ++ .../backend-jsonrpc/src/workspace.ts | 11 +- 26 files changed, 548 insertions(+), 63 deletions(-) create mode 100644 crates/pgt_analyser/src/lint/safety/add_serial_column.rs create mode 100644 crates/pgt_analyser/tests/specs/safety/addSerialColumn/basic.sql create mode 100644 crates/pgt_analyser/tests/specs/safety/addSerialColumn/basic.sql.snap create mode 100644 crates/pgt_analyser/tests/specs/safety/addSerialColumn/bigserial.sql create mode 100644 crates/pgt_analyser/tests/specs/safety/addSerialColumn/bigserial.sql.snap create mode 100644 crates/pgt_analyser/tests/specs/safety/addSerialColumn/generated_stored.sql create mode 100644 crates/pgt_analyser/tests/specs/safety/addSerialColumn/generated_stored.sql.snap create mode 100644 crates/pgt_analyser/tests/specs/safety/addSerialColumn/smallserial.sql create mode 100644 crates/pgt_analyser/tests/specs/safety/addSerialColumn/smallserial.sql.snap create mode 100644 crates/pgt_analyser/tests/specs/safety/addSerialColumn/valid_create_table.sql create mode 100644 crates/pgt_analyser/tests/specs/safety/addSerialColumn/valid_create_table.sql.snap create mode 100644 crates/pgt_analyser/tests/specs/safety/addSerialColumn/valid_generated_virtual.sql create mode 100644 crates/pgt_analyser/tests/specs/safety/addSerialColumn/valid_generated_virtual.sql.snap create mode 100644 crates/pgt_analyser/tests/specs/safety/addSerialColumn/valid_regular_column.sql create mode 100644 crates/pgt_analyser/tests/specs/safety/addSerialColumn/valid_regular_column.sql.snap create mode 100644 docs/reference/rules/add-serial-column.md diff --git a/.gitignore b/.gitignore index 94835bc44..4a0ea18eb 100644 --- a/.gitignore +++ b/.gitignore @@ -26,8 +26,9 @@ node_modules/ .claude-session-id squawk/ +eugene/ # Auto generated treesitter files crates/pgt_treesitter_grammar/src/grammar.json crates/pgt_treesitter_grammar/src/node-types.json -crates/pgt_treesitter_grammar/src/parser.c \ No newline at end of file +crates/pgt_treesitter_grammar/src/parser.c diff --git a/crates/pgt_analyser/CONTRIBUTING.md b/crates/pgt_analyser/CONTRIBUTING.md index 8eadbc4b1..3f1252699 100644 --- a/crates/pgt_analyser/CONTRIBUTING.md +++ b/crates/pgt_analyser/CONTRIBUTING.md @@ -309,6 +309,58 @@ You should: If you run the test, you'll see any diagnostics your rule created in your console. +#### Snapshot Tests + +After implementing your rule, you must create snapshot tests in the `tests/specs///` directory. + +Each test file should: +1. Start with a comment describing what the test is checking +2. Include either `-- expect_lint//` or `-- expect_no_diagnostics` on the first line +3. Contain valid SQL that should either trigger or not trigger your rule + +**Example test structure for `addSerialColumn` rule:** + +``` +tests/specs/safety/addSerialColumn/ +├── basic.sql # Basic case that triggers the rule +├── basic.sql.snap # Auto-generated snapshot +├── bigserial.sql # Test bigserial type +├── bigserial.sql.snap +├── generated_stored.sql # Test GENERATED ... STORED +├── generated_stored.sql.snap +├── valid_regular_column.sql # Valid case - should NOT trigger +└── valid_regular_column.sql.snap +``` + +**Invalid test example (should trigger diagnostic):** +```sql +-- expect_lint/safety/addSerialColumn +-- Test adding serial column to existing table +ALTER TABLE prices ADD COLUMN id serial; +``` + +**Valid test example (should NOT trigger diagnostic):** +```sql +-- Test adding regular column (should be safe) +-- expect_no_diagnostics +ALTER TABLE prices ADD COLUMN name text; +``` + +**Running and updating tests:** + +```shell +# Run tests and generate snapshots +cargo test -p pgt_analyser --test rules_tests + +# Review and accept new/changed snapshots +cargo insta test --accept + +# Or review snapshots interactively +cargo insta review +``` + +The snapshot files (`.sql.snap`) are auto-generated and should be committed to the repository. They capture the expected diagnostic output for each test case. + ### Code generation For simplicity, use `just` to run all the commands with: diff --git a/crates/pgt_analyser/src/lint/safety.rs b/crates/pgt_analyser/src/lint/safety.rs index 0ad0f2c56..0bbb8c35c 100644 --- a/crates/pgt_analyser/src/lint/safety.rs +++ b/crates/pgt_analyser/src/lint/safety.rs @@ -1,6 +1,7 @@ //! Generated file, do not edit by hand, see `xtask/codegen` use pgt_analyse::declare_lint_group; +pub mod add_serial_column; pub mod adding_field_with_default; pub mod adding_foreign_key_constraint; pub mod adding_not_null_field; @@ -29,4 +30,4 @@ pub mod renaming_table; pub mod require_concurrent_index_creation; pub mod require_concurrent_index_deletion; pub mod transaction_nesting; -declare_lint_group! { pub Safety { name : "safety" , rules : [self :: adding_field_with_default :: AddingFieldWithDefault , self :: adding_foreign_key_constraint :: AddingForeignKeyConstraint , self :: adding_not_null_field :: AddingNotNullField , self :: adding_primary_key_constraint :: AddingPrimaryKeyConstraint , self :: adding_required_field :: AddingRequiredField , self :: ban_char_field :: BanCharField , self :: ban_concurrent_index_creation_in_transaction :: BanConcurrentIndexCreationInTransaction , self :: ban_drop_column :: BanDropColumn , self :: ban_drop_database :: BanDropDatabase , self :: ban_drop_not_null :: BanDropNotNull , self :: ban_drop_table :: BanDropTable , self :: ban_truncate_cascade :: BanTruncateCascade , self :: changing_column_type :: ChangingColumnType , self :: constraint_missing_not_valid :: ConstraintMissingNotValid , self :: disallow_unique_constraint :: DisallowUniqueConstraint , self :: prefer_big_int :: PreferBigInt , self :: prefer_bigint_over_int :: PreferBigintOverInt , self :: prefer_bigint_over_smallint :: PreferBigintOverSmallint , self :: prefer_identity :: PreferIdentity , self :: prefer_jsonb :: PreferJsonb , self :: prefer_robust_stmts :: PreferRobustStmts , self :: prefer_text_field :: PreferTextField , self :: prefer_timestamptz :: PreferTimestamptz , self :: renaming_column :: RenamingColumn , self :: renaming_table :: RenamingTable , self :: require_concurrent_index_creation :: RequireConcurrentIndexCreation , self :: require_concurrent_index_deletion :: RequireConcurrentIndexDeletion , self :: transaction_nesting :: TransactionNesting ,] } } +declare_lint_group! { pub Safety { name : "safety" , rules : [self :: add_serial_column :: AddSerialColumn , self :: adding_field_with_default :: AddingFieldWithDefault , self :: adding_foreign_key_constraint :: AddingForeignKeyConstraint , self :: adding_not_null_field :: AddingNotNullField , self :: adding_primary_key_constraint :: AddingPrimaryKeyConstraint , self :: adding_required_field :: AddingRequiredField , self :: ban_char_field :: BanCharField , self :: ban_concurrent_index_creation_in_transaction :: BanConcurrentIndexCreationInTransaction , self :: ban_drop_column :: BanDropColumn , self :: ban_drop_database :: BanDropDatabase , self :: ban_drop_not_null :: BanDropNotNull , self :: ban_drop_table :: BanDropTable , self :: ban_truncate_cascade :: BanTruncateCascade , self :: changing_column_type :: ChangingColumnType , self :: constraint_missing_not_valid :: ConstraintMissingNotValid , self :: disallow_unique_constraint :: DisallowUniqueConstraint , self :: prefer_big_int :: PreferBigInt , self :: prefer_bigint_over_int :: PreferBigintOverInt , self :: prefer_bigint_over_smallint :: PreferBigintOverSmallint , self :: prefer_identity :: PreferIdentity , self :: prefer_jsonb :: PreferJsonb , self :: prefer_robust_stmts :: PreferRobustStmts , self :: prefer_text_field :: PreferTextField , self :: prefer_timestamptz :: PreferTimestamptz , self :: renaming_column :: RenamingColumn , self :: renaming_table :: RenamingTable , self :: require_concurrent_index_creation :: RequireConcurrentIndexCreation , self :: require_concurrent_index_deletion :: RequireConcurrentIndexDeletion , self :: transaction_nesting :: TransactionNesting ,] } } diff --git a/crates/pgt_analyser/src/lint/safety/add_serial_column.rs b/crates/pgt_analyser/src/lint/safety/add_serial_column.rs new file mode 100644 index 000000000..b907f1ac1 --- /dev/null +++ b/crates/pgt_analyser/src/lint/safety/add_serial_column.rs @@ -0,0 +1,141 @@ +use pgt_analyse::{Rule, RuleDiagnostic, RuleSource, context::RuleContext, declare_lint_rule}; +use pgt_console::markup; +use pgt_diagnostics::Severity; + +declare_lint_rule! { + /// Adding a column with a SERIAL type or GENERATED ALWAYS AS ... STORED causes a full table rewrite. + /// + /// When adding a column with a SERIAL type (serial, bigserial, smallserial) or a GENERATED ALWAYS AS ... STORED column + /// to an existing table, PostgreSQL must rewrite the entire table while holding an ACCESS EXCLUSIVE lock. + /// This blocks all reads and writes to the table for the duration of the rewrite operation. + /// + /// SERIAL types are implemented using sequences and DEFAULT values, while GENERATED ... STORED columns require + /// computing and storing values for all existing rows. Both operations require rewriting every row in the table. + /// + /// ## Examples + /// + /// ### Invalid + /// + /// ```sql,expect_diagnostic + /// ALTER TABLE prices ADD COLUMN id serial; + /// ``` + /// + /// ```sql,expect_diagnostic + /// ALTER TABLE prices ADD COLUMN id bigserial; + /// ``` + /// + /// ```sql,expect_diagnostic + /// ALTER TABLE prices ADD COLUMN total int GENERATED ALWAYS AS (price * quantity) STORED; + /// ``` + /// + pub AddSerialColumn { + version: "next", + name: "addSerialColumn", + severity: Severity::Error, + recommended: true, + sources: &[RuleSource::Eugene("E11")], + } +} + +impl Rule for AddSerialColumn { + type Options = (); + + fn run(ctx: &RuleContext) -> Vec { + let mut diagnostics = Vec::new(); + + if let pgt_query::NodeEnum::AlterTableStmt(stmt) = &ctx.stmt() { + for cmd in &stmt.cmds { + if let Some(pgt_query::NodeEnum::AlterTableCmd(cmd)) = &cmd.node { + if cmd.subtype() == pgt_query::protobuf::AlterTableType::AtAddColumn { + if let Some(pgt_query::NodeEnum::ColumnDef(col_def)) = + &cmd.def.as_ref().and_then(|d| d.node.as_ref()) + { + // Check for SERIAL types + if let Some(type_name) = &col_def.type_name { + let type_str = get_type_name(type_name); + if is_serial_type(&type_str) { + diagnostics.push( + RuleDiagnostic::new( + rule_category!(), + None, + markup! { + "Adding a column with type "{type_str}" requires a table rewrite." + }, + ) + .detail(None, "SERIAL types require rewriting the entire table with an ACCESS EXCLUSIVE lock, blocking all reads and writes.") + .note("SERIAL types cannot be added to existing tables without a full table rewrite. Consider using a non-serial type with a sequence instead."), + ); + continue; + } + } + + // Check for GENERATED ALWAYS AS ... STORED + let has_stored_generated = + col_def.constraints.iter().any(|constraint| { + if let Some(pgt_query::NodeEnum::Constraint(c)) = + &constraint.node + { + c.contype() + == pgt_query::protobuf::ConstrType::ConstrGenerated + && c.generated_when == "a" // 'a' = ALWAYS + } else { + false + } + }); + + if has_stored_generated { + diagnostics.push( + RuleDiagnostic::new( + rule_category!(), + None, + markup! { + "Adding a column with ""GENERATED ALWAYS AS ... STORED"" requires a table rewrite." + }, + ) + .detail(None, "GENERATED ... STORED columns require rewriting the entire table with an ACCESS EXCLUSIVE lock, blocking all reads and writes.") + .note("GENERATED ... STORED columns cannot be added to existing tables without a full table rewrite."), + ); + } + } + } + } + } + } + + diagnostics + } +} + +fn get_type_name(type_name: &pgt_query::protobuf::TypeName) -> String { + type_name + .names + .iter() + .filter_map(|n| { + if let Some(pgt_query::NodeEnum::String(s)) = &n.node { + Some(s.sval.as_str()) + } else { + None + } + }) + .collect::>() + .join(".") +} + +fn is_serial_type(type_name: &str) -> bool { + matches!( + type_name, + "serial" + | "bigserial" + | "smallserial" + | "pg_catalog.serial" + | "pg_catalog.bigserial" + | "pg_catalog.smallserial" + // Also check for serial2, serial4, serial8 which are aliases + | "serial2" + | "serial4" + | "serial8" + | "pg_catalog.serial2" + | "pg_catalog.serial4" + | "pg_catalog.serial8" + ) +} diff --git a/crates/pgt_analyser/src/options.rs b/crates/pgt_analyser/src/options.rs index 693e20b4c..a0f4d36d5 100644 --- a/crates/pgt_analyser/src/options.rs +++ b/crates/pgt_analyser/src/options.rs @@ -1,6 +1,8 @@ //! Generated file, do not edit by hand, see `xtask/codegen` use crate::lint; +pub type AddSerialColumn = + ::Options; pub type AddingFieldWithDefault = ::Options; pub type AddingForeignKeyConstraint = < lint :: safety :: adding_foreign_key_constraint :: AddingForeignKeyConstraint as pgt_analyse :: Rule > :: Options ; diff --git a/crates/pgt_analyser/tests/specs/safety/addSerialColumn/basic.sql b/crates/pgt_analyser/tests/specs/safety/addSerialColumn/basic.sql new file mode 100644 index 000000000..cec29a48b --- /dev/null +++ b/crates/pgt_analyser/tests/specs/safety/addSerialColumn/basic.sql @@ -0,0 +1,3 @@ +-- expect_lint/safety/addSerialColumn +-- Test adding serial column to existing table +ALTER TABLE prices ADD COLUMN id serial; diff --git a/crates/pgt_analyser/tests/specs/safety/addSerialColumn/basic.sql.snap b/crates/pgt_analyser/tests/specs/safety/addSerialColumn/basic.sql.snap new file mode 100644 index 000000000..bd78acb55 --- /dev/null +++ b/crates/pgt_analyser/tests/specs/safety/addSerialColumn/basic.sql.snap @@ -0,0 +1,21 @@ +--- +source: crates/pgt_analyser/tests/rules_tests.rs +expression: snapshot +snapshot_kind: text +--- +# Input +``` +-- expect_lint/safety/addSerialColumn +-- Test adding serial column to existing table +ALTER TABLE prices ADD COLUMN id serial; + +``` + +# Diagnostics +lint/safety/addSerialColumn ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + × Adding a column with type serial requires a table rewrite. + + i SERIAL types require rewriting the entire table with an ACCESS EXCLUSIVE lock, blocking all reads and writes. + + i SERIAL types cannot be added to existing tables without a full table rewrite. Consider using a non-serial type with a sequence instead. diff --git a/crates/pgt_analyser/tests/specs/safety/addSerialColumn/bigserial.sql b/crates/pgt_analyser/tests/specs/safety/addSerialColumn/bigserial.sql new file mode 100644 index 000000000..e13610a01 --- /dev/null +++ b/crates/pgt_analyser/tests/specs/safety/addSerialColumn/bigserial.sql @@ -0,0 +1,3 @@ +-- expect_lint/safety/addSerialColumn +-- Test adding bigserial column to existing table +ALTER TABLE prices ADD COLUMN big_id bigserial; diff --git a/crates/pgt_analyser/tests/specs/safety/addSerialColumn/bigserial.sql.snap b/crates/pgt_analyser/tests/specs/safety/addSerialColumn/bigserial.sql.snap new file mode 100644 index 000000000..234b92a0b --- /dev/null +++ b/crates/pgt_analyser/tests/specs/safety/addSerialColumn/bigserial.sql.snap @@ -0,0 +1,21 @@ +--- +source: crates/pgt_analyser/tests/rules_tests.rs +expression: snapshot +snapshot_kind: text +--- +# Input +``` +-- expect_lint/safety/addSerialColumn +-- Test adding bigserial column to existing table +ALTER TABLE prices ADD COLUMN big_id bigserial; + +``` + +# Diagnostics +lint/safety/addSerialColumn ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + × Adding a column with type bigserial requires a table rewrite. + + i SERIAL types require rewriting the entire table with an ACCESS EXCLUSIVE lock, blocking all reads and writes. + + i SERIAL types cannot be added to existing tables without a full table rewrite. Consider using a non-serial type with a sequence instead. diff --git a/crates/pgt_analyser/tests/specs/safety/addSerialColumn/generated_stored.sql b/crates/pgt_analyser/tests/specs/safety/addSerialColumn/generated_stored.sql new file mode 100644 index 000000000..3d51f3ad2 --- /dev/null +++ b/crates/pgt_analyser/tests/specs/safety/addSerialColumn/generated_stored.sql @@ -0,0 +1,3 @@ +-- expect_lint/safety/addSerialColumn +-- Test adding GENERATED ALWAYS AS ... STORED column to existing table +ALTER TABLE prices ADD COLUMN total integer GENERATED ALWAYS AS (price * quantity) STORED; diff --git a/crates/pgt_analyser/tests/specs/safety/addSerialColumn/generated_stored.sql.snap b/crates/pgt_analyser/tests/specs/safety/addSerialColumn/generated_stored.sql.snap new file mode 100644 index 000000000..8ec47219e --- /dev/null +++ b/crates/pgt_analyser/tests/specs/safety/addSerialColumn/generated_stored.sql.snap @@ -0,0 +1,21 @@ +--- +source: crates/pgt_analyser/tests/rules_tests.rs +expression: snapshot +snapshot_kind: text +--- +# Input +``` +-- expect_lint/safety/addSerialColumn +-- Test adding GENERATED ALWAYS AS ... STORED column to existing table +ALTER TABLE prices ADD COLUMN total integer GENERATED ALWAYS AS (price * quantity) STORED; + +``` + +# Diagnostics +lint/safety/addSerialColumn ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + × Adding a column with GENERATED ALWAYS AS ... STORED requires a table rewrite. + + i GENERATED ... STORED columns require rewriting the entire table with an ACCESS EXCLUSIVE lock, blocking all reads and writes. + + i GENERATED ... STORED columns cannot be added to existing tables without a full table rewrite. diff --git a/crates/pgt_analyser/tests/specs/safety/addSerialColumn/smallserial.sql b/crates/pgt_analyser/tests/specs/safety/addSerialColumn/smallserial.sql new file mode 100644 index 000000000..3af9a6edf --- /dev/null +++ b/crates/pgt_analyser/tests/specs/safety/addSerialColumn/smallserial.sql @@ -0,0 +1,3 @@ +-- expect_lint/safety/addSerialColumn +-- Test adding smallserial column to existing table +ALTER TABLE prices ADD COLUMN small_id smallserial; diff --git a/crates/pgt_analyser/tests/specs/safety/addSerialColumn/smallserial.sql.snap b/crates/pgt_analyser/tests/specs/safety/addSerialColumn/smallserial.sql.snap new file mode 100644 index 000000000..0a988b7f5 --- /dev/null +++ b/crates/pgt_analyser/tests/specs/safety/addSerialColumn/smallserial.sql.snap @@ -0,0 +1,21 @@ +--- +source: crates/pgt_analyser/tests/rules_tests.rs +expression: snapshot +snapshot_kind: text +--- +# Input +``` +-- expect_lint/safety/addSerialColumn +-- Test adding smallserial column to existing table +ALTER TABLE prices ADD COLUMN small_id smallserial; + +``` + +# Diagnostics +lint/safety/addSerialColumn ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + × Adding a column with type smallserial requires a table rewrite. + + i SERIAL types require rewriting the entire table with an ACCESS EXCLUSIVE lock, blocking all reads and writes. + + i SERIAL types cannot be added to existing tables without a full table rewrite. Consider using a non-serial type with a sequence instead. diff --git a/crates/pgt_analyser/tests/specs/safety/addSerialColumn/valid_create_table.sql b/crates/pgt_analyser/tests/specs/safety/addSerialColumn/valid_create_table.sql new file mode 100644 index 000000000..d06c3b051 --- /dev/null +++ b/crates/pgt_analyser/tests/specs/safety/addSerialColumn/valid_create_table.sql @@ -0,0 +1,7 @@ +-- Test CREATE TABLE with serial column (should be safe, rule only applies to ALTER TABLE) +-- expect_no_diagnostics +CREATE TABLE products ( + id serial PRIMARY KEY, + name text NOT NULL, + price numeric +); diff --git a/crates/pgt_analyser/tests/specs/safety/addSerialColumn/valid_create_table.sql.snap b/crates/pgt_analyser/tests/specs/safety/addSerialColumn/valid_create_table.sql.snap new file mode 100644 index 000000000..6e80d5be6 --- /dev/null +++ b/crates/pgt_analyser/tests/specs/safety/addSerialColumn/valid_create_table.sql.snap @@ -0,0 +1,16 @@ +--- +source: crates/pgt_analyser/tests/rules_tests.rs +expression: snapshot +snapshot_kind: text +--- +# Input +``` +-- Test CREATE TABLE with serial column (should be safe, rule only applies to ALTER TABLE) +-- expect_no_diagnostics +CREATE TABLE products ( + id serial PRIMARY KEY, + name text NOT NULL, + price numeric +); + +``` diff --git a/crates/pgt_analyser/tests/specs/safety/addSerialColumn/valid_generated_virtual.sql b/crates/pgt_analyser/tests/specs/safety/addSerialColumn/valid_generated_virtual.sql new file mode 100644 index 000000000..80c9f6263 --- /dev/null +++ b/crates/pgt_analyser/tests/specs/safety/addSerialColumn/valid_generated_virtual.sql @@ -0,0 +1,3 @@ +-- Test adding regular column with integer type (should be safe) +-- expect_no_diagnostics +ALTER TABLE prices ADD COLUMN quantity integer DEFAULT 0; diff --git a/crates/pgt_analyser/tests/specs/safety/addSerialColumn/valid_generated_virtual.sql.snap b/crates/pgt_analyser/tests/specs/safety/addSerialColumn/valid_generated_virtual.sql.snap new file mode 100644 index 000000000..a72863226 --- /dev/null +++ b/crates/pgt_analyser/tests/specs/safety/addSerialColumn/valid_generated_virtual.sql.snap @@ -0,0 +1,12 @@ +--- +source: crates/pgt_analyser/tests/rules_tests.rs +expression: snapshot +snapshot_kind: text +--- +# Input +``` +-- Test adding regular column with integer type (should be safe) +-- expect_no_diagnostics +ALTER TABLE prices ADD COLUMN quantity integer DEFAULT 0; + +``` diff --git a/crates/pgt_analyser/tests/specs/safety/addSerialColumn/valid_regular_column.sql b/crates/pgt_analyser/tests/specs/safety/addSerialColumn/valid_regular_column.sql new file mode 100644 index 000000000..9e10b6c15 --- /dev/null +++ b/crates/pgt_analyser/tests/specs/safety/addSerialColumn/valid_regular_column.sql @@ -0,0 +1,3 @@ +-- Test adding regular column (should be safe) +-- expect_no_diagnostics +ALTER TABLE prices ADD COLUMN name text; diff --git a/crates/pgt_analyser/tests/specs/safety/addSerialColumn/valid_regular_column.sql.snap b/crates/pgt_analyser/tests/specs/safety/addSerialColumn/valid_regular_column.sql.snap new file mode 100644 index 000000000..0b8989590 --- /dev/null +++ b/crates/pgt_analyser/tests/specs/safety/addSerialColumn/valid_regular_column.sql.snap @@ -0,0 +1,12 @@ +--- +source: crates/pgt_analyser/tests/rules_tests.rs +expression: snapshot +snapshot_kind: text +--- +# Input +``` +-- Test adding regular column (should be safe) +-- expect_no_diagnostics +ALTER TABLE prices ADD COLUMN name text; + +``` diff --git a/crates/pgt_configuration/src/analyser/linter/rules.rs b/crates/pgt_configuration/src/analyser/linter/rules.rs index 8b23048fa..3788c81b4 100644 --- a/crates/pgt_configuration/src/analyser/linter/rules.rs +++ b/crates/pgt_configuration/src/analyser/linter/rules.rs @@ -141,6 +141,9 @@ pub struct Safety { #[doc = r" It enables ALL rules for this group."] #[serde(skip_serializing_if = "Option::is_none")] pub all: Option, + #[doc = "Adding a column with a SERIAL type or GENERATED ALWAYS AS ... STORED causes a full table rewrite."] + #[serde(skip_serializing_if = "Option::is_none")] + pub add_serial_column: Option>, #[doc = "Adding a column with a DEFAULT value may lead to a table rewrite while holding an ACCESS EXCLUSIVE lock."] #[serde(skip_serializing_if = "Option::is_none")] pub adding_field_with_default: @@ -240,6 +243,7 @@ pub struct Safety { impl Safety { const GROUP_NAME: &'static str = "safety"; pub(crate) const GROUP_RULES: &'static [&'static str] = &[ + "addSerialColumn", "addingFieldWithDefault", "addingForeignKeyConstraint", "addingNotNullField", @@ -274,10 +278,11 @@ impl Safety { RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[1]), RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[2]), RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[3]), - RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[6]), + RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[4]), RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[7]), - RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[9]), + RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[8]), RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[10]), + RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[11]), ]; const ALL_RULES_AS_FILTERS: &'static [RuleFilter<'static>] = &[ RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[0]), @@ -308,6 +313,7 @@ impl Safety { RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[25]), RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[26]), RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[27]), + RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[28]), ]; #[doc = r" Retrieves the recommended rules"] pub(crate) fn is_recommended_true(&self) -> bool { @@ -324,290 +330,300 @@ impl Safety { } pub(crate) fn get_enabled_rules(&self) -> FxHashSet> { let mut index_set = FxHashSet::default(); - if let Some(rule) = self.adding_field_with_default.as_ref() { + if let Some(rule) = self.add_serial_column.as_ref() { if rule.is_enabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[0])); } } - if let Some(rule) = self.adding_foreign_key_constraint.as_ref() { + if let Some(rule) = self.adding_field_with_default.as_ref() { if rule.is_enabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[1])); } } - if let Some(rule) = self.adding_not_null_field.as_ref() { + if let Some(rule) = self.adding_foreign_key_constraint.as_ref() { if rule.is_enabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[2])); } } - if let Some(rule) = self.adding_primary_key_constraint.as_ref() { + if let Some(rule) = self.adding_not_null_field.as_ref() { if rule.is_enabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[3])); } } - if let Some(rule) = self.adding_required_field.as_ref() { + if let Some(rule) = self.adding_primary_key_constraint.as_ref() { if rule.is_enabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[4])); } } - if let Some(rule) = self.ban_char_field.as_ref() { + if let Some(rule) = self.adding_required_field.as_ref() { if rule.is_enabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[5])); } } - if let Some(rule) = self.ban_concurrent_index_creation_in_transaction.as_ref() { + if let Some(rule) = self.ban_char_field.as_ref() { if rule.is_enabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[6])); } } - if let Some(rule) = self.ban_drop_column.as_ref() { + if let Some(rule) = self.ban_concurrent_index_creation_in_transaction.as_ref() { if rule.is_enabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[7])); } } - if let Some(rule) = self.ban_drop_database.as_ref() { + if let Some(rule) = self.ban_drop_column.as_ref() { if rule.is_enabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[8])); } } - if let Some(rule) = self.ban_drop_not_null.as_ref() { + if let Some(rule) = self.ban_drop_database.as_ref() { if rule.is_enabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[9])); } } - if let Some(rule) = self.ban_drop_table.as_ref() { + if let Some(rule) = self.ban_drop_not_null.as_ref() { if rule.is_enabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[10])); } } - if let Some(rule) = self.ban_truncate_cascade.as_ref() { + if let Some(rule) = self.ban_drop_table.as_ref() { if rule.is_enabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[11])); } } - if let Some(rule) = self.changing_column_type.as_ref() { + if let Some(rule) = self.ban_truncate_cascade.as_ref() { if rule.is_enabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[12])); } } - if let Some(rule) = self.constraint_missing_not_valid.as_ref() { + if let Some(rule) = self.changing_column_type.as_ref() { if rule.is_enabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[13])); } } - if let Some(rule) = self.disallow_unique_constraint.as_ref() { + if let Some(rule) = self.constraint_missing_not_valid.as_ref() { if rule.is_enabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[14])); } } - if let Some(rule) = self.prefer_big_int.as_ref() { + if let Some(rule) = self.disallow_unique_constraint.as_ref() { if rule.is_enabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[15])); } } - if let Some(rule) = self.prefer_bigint_over_int.as_ref() { + if let Some(rule) = self.prefer_big_int.as_ref() { if rule.is_enabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[16])); } } - if let Some(rule) = self.prefer_bigint_over_smallint.as_ref() { + if let Some(rule) = self.prefer_bigint_over_int.as_ref() { if rule.is_enabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[17])); } } - if let Some(rule) = self.prefer_identity.as_ref() { + if let Some(rule) = self.prefer_bigint_over_smallint.as_ref() { if rule.is_enabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[18])); } } - if let Some(rule) = self.prefer_jsonb.as_ref() { + if let Some(rule) = self.prefer_identity.as_ref() { if rule.is_enabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[19])); } } - if let Some(rule) = self.prefer_robust_stmts.as_ref() { + if let Some(rule) = self.prefer_jsonb.as_ref() { if rule.is_enabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[20])); } } - if let Some(rule) = self.prefer_text_field.as_ref() { + if let Some(rule) = self.prefer_robust_stmts.as_ref() { if rule.is_enabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[21])); } } - if let Some(rule) = self.prefer_timestamptz.as_ref() { + if let Some(rule) = self.prefer_text_field.as_ref() { if rule.is_enabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[22])); } } - if let Some(rule) = self.renaming_column.as_ref() { + if let Some(rule) = self.prefer_timestamptz.as_ref() { if rule.is_enabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[23])); } } - if let Some(rule) = self.renaming_table.as_ref() { + if let Some(rule) = self.renaming_column.as_ref() { if rule.is_enabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[24])); } } - if let Some(rule) = self.require_concurrent_index_creation.as_ref() { + if let Some(rule) = self.renaming_table.as_ref() { if rule.is_enabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[25])); } } - if let Some(rule) = self.require_concurrent_index_deletion.as_ref() { + if let Some(rule) = self.require_concurrent_index_creation.as_ref() { if rule.is_enabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[26])); } } - if let Some(rule) = self.transaction_nesting.as_ref() { + if let Some(rule) = self.require_concurrent_index_deletion.as_ref() { if rule.is_enabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[27])); } } + if let Some(rule) = self.transaction_nesting.as_ref() { + if rule.is_enabled() { + index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[28])); + } + } index_set } pub(crate) fn get_disabled_rules(&self) -> FxHashSet> { let mut index_set = FxHashSet::default(); - if let Some(rule) = self.adding_field_with_default.as_ref() { + if let Some(rule) = self.add_serial_column.as_ref() { if rule.is_disabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[0])); } } - if let Some(rule) = self.adding_foreign_key_constraint.as_ref() { + if let Some(rule) = self.adding_field_with_default.as_ref() { if rule.is_disabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[1])); } } - if let Some(rule) = self.adding_not_null_field.as_ref() { + if let Some(rule) = self.adding_foreign_key_constraint.as_ref() { if rule.is_disabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[2])); } } - if let Some(rule) = self.adding_primary_key_constraint.as_ref() { + if let Some(rule) = self.adding_not_null_field.as_ref() { if rule.is_disabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[3])); } } - if let Some(rule) = self.adding_required_field.as_ref() { + if let Some(rule) = self.adding_primary_key_constraint.as_ref() { if rule.is_disabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[4])); } } - if let Some(rule) = self.ban_char_field.as_ref() { + if let Some(rule) = self.adding_required_field.as_ref() { if rule.is_disabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[5])); } } - if let Some(rule) = self.ban_concurrent_index_creation_in_transaction.as_ref() { + if let Some(rule) = self.ban_char_field.as_ref() { if rule.is_disabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[6])); } } - if let Some(rule) = self.ban_drop_column.as_ref() { + if let Some(rule) = self.ban_concurrent_index_creation_in_transaction.as_ref() { if rule.is_disabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[7])); } } - if let Some(rule) = self.ban_drop_database.as_ref() { + if let Some(rule) = self.ban_drop_column.as_ref() { if rule.is_disabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[8])); } } - if let Some(rule) = self.ban_drop_not_null.as_ref() { + if let Some(rule) = self.ban_drop_database.as_ref() { if rule.is_disabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[9])); } } - if let Some(rule) = self.ban_drop_table.as_ref() { + if let Some(rule) = self.ban_drop_not_null.as_ref() { if rule.is_disabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[10])); } } - if let Some(rule) = self.ban_truncate_cascade.as_ref() { + if let Some(rule) = self.ban_drop_table.as_ref() { if rule.is_disabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[11])); } } - if let Some(rule) = self.changing_column_type.as_ref() { + if let Some(rule) = self.ban_truncate_cascade.as_ref() { if rule.is_disabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[12])); } } - if let Some(rule) = self.constraint_missing_not_valid.as_ref() { + if let Some(rule) = self.changing_column_type.as_ref() { if rule.is_disabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[13])); } } - if let Some(rule) = self.disallow_unique_constraint.as_ref() { + if let Some(rule) = self.constraint_missing_not_valid.as_ref() { if rule.is_disabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[14])); } } - if let Some(rule) = self.prefer_big_int.as_ref() { + if let Some(rule) = self.disallow_unique_constraint.as_ref() { if rule.is_disabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[15])); } } - if let Some(rule) = self.prefer_bigint_over_int.as_ref() { + if let Some(rule) = self.prefer_big_int.as_ref() { if rule.is_disabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[16])); } } - if let Some(rule) = self.prefer_bigint_over_smallint.as_ref() { + if let Some(rule) = self.prefer_bigint_over_int.as_ref() { if rule.is_disabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[17])); } } - if let Some(rule) = self.prefer_identity.as_ref() { + if let Some(rule) = self.prefer_bigint_over_smallint.as_ref() { if rule.is_disabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[18])); } } - if let Some(rule) = self.prefer_jsonb.as_ref() { + if let Some(rule) = self.prefer_identity.as_ref() { if rule.is_disabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[19])); } } - if let Some(rule) = self.prefer_robust_stmts.as_ref() { + if let Some(rule) = self.prefer_jsonb.as_ref() { if rule.is_disabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[20])); } } - if let Some(rule) = self.prefer_text_field.as_ref() { + if let Some(rule) = self.prefer_robust_stmts.as_ref() { if rule.is_disabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[21])); } } - if let Some(rule) = self.prefer_timestamptz.as_ref() { + if let Some(rule) = self.prefer_text_field.as_ref() { if rule.is_disabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[22])); } } - if let Some(rule) = self.renaming_column.as_ref() { + if let Some(rule) = self.prefer_timestamptz.as_ref() { if rule.is_disabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[23])); } } - if let Some(rule) = self.renaming_table.as_ref() { + if let Some(rule) = self.renaming_column.as_ref() { if rule.is_disabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[24])); } } - if let Some(rule) = self.require_concurrent_index_creation.as_ref() { + if let Some(rule) = self.renaming_table.as_ref() { if rule.is_disabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[25])); } } - if let Some(rule) = self.require_concurrent_index_deletion.as_ref() { + if let Some(rule) = self.require_concurrent_index_creation.as_ref() { if rule.is_disabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[26])); } } - if let Some(rule) = self.transaction_nesting.as_ref() { + if let Some(rule) = self.require_concurrent_index_deletion.as_ref() { if rule.is_disabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[27])); } } + if let Some(rule) = self.transaction_nesting.as_ref() { + if rule.is_disabled() { + index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[28])); + } + } index_set } #[doc = r" Checks if, given a rule name, matches one of the rules contained in this category"] @@ -637,6 +653,7 @@ impl Safety { } pub(crate) fn severity(rule_name: &str) -> Severity { match rule_name { + "addSerialColumn" => Severity::Error, "addingFieldWithDefault" => Severity::Warning, "addingForeignKeyConstraint" => Severity::Warning, "addingNotNullField" => Severity::Warning, @@ -673,6 +690,10 @@ impl Safety { rule_name: &str, ) -> Option<(RulePlainConfiguration, Option)> { match rule_name { + "addSerialColumn" => self + .add_serial_column + .as_ref() + .map(|conf| (conf.level(), conf.get_options())), "addingFieldWithDefault" => self .adding_field_with_default .as_ref() diff --git a/crates/pgt_diagnostics_categories/src/categories.rs b/crates/pgt_diagnostics_categories/src/categories.rs index 416416ef9..856f9f983 100644 --- a/crates/pgt_diagnostics_categories/src/categories.rs +++ b/crates/pgt_diagnostics_categories/src/categories.rs @@ -24,6 +24,7 @@ define_categories! { "lint/safety/banDropDatabase": "https://pgtools.dev/latest/rules/ban-drop-database", "lint/safety/banDropNotNull": "https://pgtools.dev/latest/rules/ban-drop-not-null", "lint/safety/banDropTable": "https://pgtools.dev/latest/rules/ban-drop-table", + "lint/safety/addSerialColumn": "https://pgtools.dev/latest/rules/add-serial-column", "lint/safety/banTruncateCascade": "https://pgtools.dev/latest/rules/ban-truncate-cascade", "lint/safety/changingColumnType": "https://pgtools.dev/latest/rules/changing-column-type", "lint/safety/constraintMissingNotValid": "https://pgtools.dev/latest/rules/constraint-missing-not-valid", diff --git a/docs/reference/rule_sources.md b/docs/reference/rule_sources.md index d8aea6fc6..98f401c2f 100644 --- a/docs/reference/rule_sources.md +++ b/docs/reference/rule_sources.md @@ -6,6 +6,7 @@ _No exclusive rules available._ ### Eugene | Eugene Rule Name | Rule Name | | ---- | ---- | +| [E11](https://kaveland.no/eugene/hints/E11/index.html) |[addSerialColumn](../rules/add-serial-column) | | [E3](https://kaveland.no/eugene/hints/E3/index.html) |[preferJsonb](../rules/prefer-jsonb) | ### Squawk | Squawk Rule Name | Rule Name | diff --git a/docs/reference/rules.md b/docs/reference/rules.md index 6a946ff62..836b2c982 100644 --- a/docs/reference/rules.md +++ b/docs/reference/rules.md @@ -12,6 +12,7 @@ Rules that detect potential safety issues in your code. | Rule name | Description | Properties | | --- | --- | --- | +| [addSerialColumn](./add-serial-column) | Adding a column with a SERIAL type or GENERATED ALWAYS AS ... STORED causes a full table rewrite. | ✅ | | [addingFieldWithDefault](./adding-field-with-default) | Adding a column with a DEFAULT value may lead to a table rewrite while holding an ACCESS EXCLUSIVE lock. | ✅ | | [addingForeignKeyConstraint](./adding-foreign-key-constraint) | Adding a foreign key constraint requires a table scan and a SHARE ROW EXCLUSIVE lock on both tables, which blocks writes. | ✅ | | [addingNotNullField](./adding-not-null-field) | Setting a column NOT NULL blocks reads while the table is scanned. | ✅ | diff --git a/docs/reference/rules/add-serial-column.md b/docs/reference/rules/add-serial-column.md new file mode 100644 index 000000000..70fab7601 --- /dev/null +++ b/docs/reference/rules/add-serial-column.md @@ -0,0 +1,99 @@ +# addSerialColumn +**Diagnostic Category: `lint/safety/addSerialColumn`** + +**Since**: `vnext` + +> [!NOTE] +> This rule is recommended. A diagnostic error will appear when linting your code. + +**Sources**: +- Inspired from: eugene/E11 + +## Description +Adding a column with a SERIAL type or GENERATED ALWAYS AS ... STORED causes a full table rewrite. + +When adding a column with a SERIAL type (serial, bigserial, smallserial) or a GENERATED ALWAYS AS ... STORED column +to an existing table, PostgreSQL must rewrite the entire table while holding an ACCESS EXCLUSIVE lock. +This blocks all reads and writes to the table for the duration of the rewrite operation. + +SERIAL types are implemented using sequences and DEFAULT values, while GENERATED ... STORED columns require +computing and storing values for all existing rows. Both operations require rewriting every row in the table. + +## Examples + +### Invalid + +```sql +ALTER TABLE prices ADD COLUMN id serial; +``` + +```sh +code-block.sql:1:1 lint/safety/addSerialColumn ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + × Adding a column with type serial requires a table rewrite. + + > 1 │ ALTER TABLE prices ADD COLUMN id serial; + │ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + 2 │ + + i SERIAL types require rewriting the entire table with an ACCESS EXCLUSIVE lock, blocking all reads and writes. + + i SERIAL types cannot be added to existing tables without a full table rewrite. Consider using a non-serial type with a sequence instead. + + +``` + +```sql +ALTER TABLE prices ADD COLUMN id bigserial; +``` + +```sh +code-block.sql:1:1 lint/safety/addSerialColumn ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + × Adding a column with type bigserial requires a table rewrite. + + > 1 │ ALTER TABLE prices ADD COLUMN id bigserial; + │ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + 2 │ + + i SERIAL types require rewriting the entire table with an ACCESS EXCLUSIVE lock, blocking all reads and writes. + + i SERIAL types cannot be added to existing tables without a full table rewrite. Consider using a non-serial type with a sequence instead. + + +``` + +```sql +ALTER TABLE prices ADD COLUMN total int GENERATED ALWAYS AS (price * quantity) STORED; +``` + +```sh +code-block.sql:1:1 lint/safety/addSerialColumn ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + × Adding a column with GENERATED ALWAYS AS ... STORED requires a table rewrite. + + > 1 │ ALTER TABLE prices ADD COLUMN total int GENERATED ALWAYS AS (price * quantity) STORED; + │ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + 2 │ + + i GENERATED ... STORED columns require rewriting the entire table with an ACCESS EXCLUSIVE lock, blocking all reads and writes. + + i GENERATED ... STORED columns cannot be added to existing tables without a full table rewrite. + + +``` + +## How to configure +```json + +{ + "linter": { + "rules": { + "safety": { + "addSerialColumn": "error" + } + } + } +} + +``` diff --git a/docs/schema.json b/docs/schema.json index 55f1abbfb..e3c4ab147 100644 --- a/docs/schema.json +++ b/docs/schema.json @@ -356,6 +356,17 @@ "description": "A list of rules that belong to this group", "type": "object", "properties": { + "addSerialColumn": { + "description": "Adding a column with a SERIAL type or GENERATED ALWAYS AS ... STORED causes a full table rewrite.", + "anyOf": [ + { + "$ref": "#/definitions/RuleConfiguration" + }, + { + "type": "null" + } + ] + }, "addingFieldWithDefault": { "description": "Adding a column with a DEFAULT value may lead to a table rewrite while holding an ACCESS EXCLUSIVE lock.", "anyOf": [ diff --git a/packages/@postgrestools/backend-jsonrpc/src/workspace.ts b/packages/@postgrestools/backend-jsonrpc/src/workspace.ts index 24ea5d503..379eef876 100644 --- a/packages/@postgrestools/backend-jsonrpc/src/workspace.ts +++ b/packages/@postgrestools/backend-jsonrpc/src/workspace.ts @@ -74,6 +74,7 @@ export type Category = | "lint/safety/banDropDatabase" | "lint/safety/banDropNotNull" | "lint/safety/banDropTable" + | "lint/safety/addSerialColumn" | "lint/safety/banTruncateCascade" | "lint/safety/changingColumnType" | "lint/safety/constraintMissingNotValid" @@ -122,7 +123,7 @@ export type DiagnosticTags = DiagnosticTag[]; /** * Serializable representation of a [Diagnostic](super::Diagnostic) advice -See the [Visitor] trait for additional documentation on all the supported advice types. +See the [Visitor] trait for additional documentation on all the supported advice types. */ export type Advice = | { log: [LogCategory, MarkupBuf] } @@ -227,7 +228,7 @@ export interface CompletionItem { /** * The text that the editor should fill in. If `None`, the `label` should be used. Tables, for example, might have different completion_texts: -label: "users", description: "Schema: auth", completion_text: "auth.users". +label: "users", description: "Schema: auth", completion_text: "auth.users". */ export interface CompletionText { is_snippet: boolean; @@ -411,7 +412,7 @@ export interface PartialVcsConfiguration { /** * The folder where we should check for VCS files. By default, we will use the same folder where `postgres-language-server.jsonc` was found. -If we can't find the configuration, it will attempt to use the current working directory. If no current working directory can't be found, we won't use the VCS integration, and a diagnostic will be emitted +If we can't find the configuration, it will attempt to use the current working directory. If no current working directory can't be found, we won't use the VCS integration, and a diagnostic will be emitted */ root?: string; /** @@ -435,6 +436,10 @@ export type VcsClientKind = "git"; * A list of rules that belong to this group */ export interface Safety { + /** + * Adding a column with a SERIAL type or GENERATED ALWAYS AS ... STORED causes a full table rewrite. + */ + addSerialColumn?: RuleConfiguration_for_Null; /** * Adding a column with a DEFAULT value may lead to a table rewrite while holding an ACCESS EXCLUSIVE lock. */ From 6cf5b45f6df929fb4bfa0e00d9d3e48cda1f2fc2 Mon Sep 17 00:00:00 2001 From: psteinroe Date: Sun, 19 Oct 2025 12:35:18 +0200 Subject: [PATCH 2/8] feat(analyser): mutiple alter table --- crates/pgt_analyser/src/lint/safety.rs | 3 +- .../src/lint/safety/multiple_alter_table.rs | 115 ++++++++++++++++++ crates/pgt_analyser/src/options.rs | 2 + .../specs/safety/multipleAlterTable/basic.sql | 4 + .../safety/multipleAlterTable/basic.sql.snap | 22 ++++ .../mixed_schema_notation.sql | 4 + .../mixed_schema_notation.sql.snap | 22 ++++ .../multipleAlterTable/three_alters.sql | 5 + .../multipleAlterTable/three_alters.sql.snap | 23 ++++ .../multipleAlterTable/valid_combined.sql | 5 + .../valid_combined.sql.snap | 14 +++ .../valid_different_schemas.sql | 4 + .../valid_different_schemas.sql.snap | 13 ++ .../valid_different_tables.sql | 4 + .../valid_different_tables.sql.snap | 13 ++ .../multipleAlterTable/valid_single_alter.sql | 3 + .../valid_single_alter.sql.snap | 12 ++ .../safety/multipleAlterTable/with_schema.sql | 4 + .../multipleAlterTable/with_schema.sql.snap | 22 ++++ .../src/analyser/linter/rules.rs | 73 +++++++---- .../src/categories.rs | 3 +- docs/reference/rule_sources.md | 1 + docs/reference/rules.md | 1 + docs/reference/rules/multiple-alter-table.md | 56 +++++++++ docs/schema.json | 11 ++ .../backend-jsonrpc/src/workspace.ts | 7 +- 26 files changed, 417 insertions(+), 29 deletions(-) create mode 100644 crates/pgt_analyser/src/lint/safety/multiple_alter_table.rs create mode 100644 crates/pgt_analyser/tests/specs/safety/multipleAlterTable/basic.sql create mode 100644 crates/pgt_analyser/tests/specs/safety/multipleAlterTable/basic.sql.snap create mode 100644 crates/pgt_analyser/tests/specs/safety/multipleAlterTable/mixed_schema_notation.sql create mode 100644 crates/pgt_analyser/tests/specs/safety/multipleAlterTable/mixed_schema_notation.sql.snap create mode 100644 crates/pgt_analyser/tests/specs/safety/multipleAlterTable/three_alters.sql create mode 100644 crates/pgt_analyser/tests/specs/safety/multipleAlterTable/three_alters.sql.snap create mode 100644 crates/pgt_analyser/tests/specs/safety/multipleAlterTable/valid_combined.sql create mode 100644 crates/pgt_analyser/tests/specs/safety/multipleAlterTable/valid_combined.sql.snap create mode 100644 crates/pgt_analyser/tests/specs/safety/multipleAlterTable/valid_different_schemas.sql create mode 100644 crates/pgt_analyser/tests/specs/safety/multipleAlterTable/valid_different_schemas.sql.snap create mode 100644 crates/pgt_analyser/tests/specs/safety/multipleAlterTable/valid_different_tables.sql create mode 100644 crates/pgt_analyser/tests/specs/safety/multipleAlterTable/valid_different_tables.sql.snap create mode 100644 crates/pgt_analyser/tests/specs/safety/multipleAlterTable/valid_single_alter.sql create mode 100644 crates/pgt_analyser/tests/specs/safety/multipleAlterTable/valid_single_alter.sql.snap create mode 100644 crates/pgt_analyser/tests/specs/safety/multipleAlterTable/with_schema.sql create mode 100644 crates/pgt_analyser/tests/specs/safety/multipleAlterTable/with_schema.sql.snap create mode 100644 docs/reference/rules/multiple-alter-table.md diff --git a/crates/pgt_analyser/src/lint/safety.rs b/crates/pgt_analyser/src/lint/safety.rs index 0bbb8c35c..3cdd5dd3e 100644 --- a/crates/pgt_analyser/src/lint/safety.rs +++ b/crates/pgt_analyser/src/lint/safety.rs @@ -17,6 +17,7 @@ pub mod ban_truncate_cascade; pub mod changing_column_type; pub mod constraint_missing_not_valid; pub mod disallow_unique_constraint; +pub mod multiple_alter_table; pub mod prefer_big_int; pub mod prefer_bigint_over_int; pub mod prefer_bigint_over_smallint; @@ -30,4 +31,4 @@ pub mod renaming_table; pub mod require_concurrent_index_creation; pub mod require_concurrent_index_deletion; pub mod transaction_nesting; -declare_lint_group! { pub Safety { name : "safety" , rules : [self :: add_serial_column :: AddSerialColumn , self :: adding_field_with_default :: AddingFieldWithDefault , self :: adding_foreign_key_constraint :: AddingForeignKeyConstraint , self :: adding_not_null_field :: AddingNotNullField , self :: adding_primary_key_constraint :: AddingPrimaryKeyConstraint , self :: adding_required_field :: AddingRequiredField , self :: ban_char_field :: BanCharField , self :: ban_concurrent_index_creation_in_transaction :: BanConcurrentIndexCreationInTransaction , self :: ban_drop_column :: BanDropColumn , self :: ban_drop_database :: BanDropDatabase , self :: ban_drop_not_null :: BanDropNotNull , self :: ban_drop_table :: BanDropTable , self :: ban_truncate_cascade :: BanTruncateCascade , self :: changing_column_type :: ChangingColumnType , self :: constraint_missing_not_valid :: ConstraintMissingNotValid , self :: disallow_unique_constraint :: DisallowUniqueConstraint , self :: prefer_big_int :: PreferBigInt , self :: prefer_bigint_over_int :: PreferBigintOverInt , self :: prefer_bigint_over_smallint :: PreferBigintOverSmallint , self :: prefer_identity :: PreferIdentity , self :: prefer_jsonb :: PreferJsonb , self :: prefer_robust_stmts :: PreferRobustStmts , self :: prefer_text_field :: PreferTextField , self :: prefer_timestamptz :: PreferTimestamptz , self :: renaming_column :: RenamingColumn , self :: renaming_table :: RenamingTable , self :: require_concurrent_index_creation :: RequireConcurrentIndexCreation , self :: require_concurrent_index_deletion :: RequireConcurrentIndexDeletion , self :: transaction_nesting :: TransactionNesting ,] } } +declare_lint_group! { pub Safety { name : "safety" , rules : [self :: add_serial_column :: AddSerialColumn , self :: adding_field_with_default :: AddingFieldWithDefault , self :: adding_foreign_key_constraint :: AddingForeignKeyConstraint , self :: adding_not_null_field :: AddingNotNullField , self :: adding_primary_key_constraint :: AddingPrimaryKeyConstraint , self :: adding_required_field :: AddingRequiredField , self :: ban_char_field :: BanCharField , self :: ban_concurrent_index_creation_in_transaction :: BanConcurrentIndexCreationInTransaction , self :: ban_drop_column :: BanDropColumn , self :: ban_drop_database :: BanDropDatabase , self :: ban_drop_not_null :: BanDropNotNull , self :: ban_drop_table :: BanDropTable , self :: ban_truncate_cascade :: BanTruncateCascade , self :: changing_column_type :: ChangingColumnType , self :: constraint_missing_not_valid :: ConstraintMissingNotValid , self :: disallow_unique_constraint :: DisallowUniqueConstraint , self :: multiple_alter_table :: MultipleAlterTable , self :: prefer_big_int :: PreferBigInt , self :: prefer_bigint_over_int :: PreferBigintOverInt , self :: prefer_bigint_over_smallint :: PreferBigintOverSmallint , self :: prefer_identity :: PreferIdentity , self :: prefer_jsonb :: PreferJsonb , self :: prefer_robust_stmts :: PreferRobustStmts , self :: prefer_text_field :: PreferTextField , self :: prefer_timestamptz :: PreferTimestamptz , self :: renaming_column :: RenamingColumn , self :: renaming_table :: RenamingTable , self :: require_concurrent_index_creation :: RequireConcurrentIndexCreation , self :: require_concurrent_index_deletion :: RequireConcurrentIndexDeletion , self :: transaction_nesting :: TransactionNesting ,] } } diff --git a/crates/pgt_analyser/src/lint/safety/multiple_alter_table.rs b/crates/pgt_analyser/src/lint/safety/multiple_alter_table.rs new file mode 100644 index 000000000..b1237794e --- /dev/null +++ b/crates/pgt_analyser/src/lint/safety/multiple_alter_table.rs @@ -0,0 +1,115 @@ +use pgt_analyse::{Rule, RuleDiagnostic, RuleSource, context::RuleContext, declare_lint_rule}; +use pgt_console::markup; +use pgt_diagnostics::Severity; + +declare_lint_rule! { + /// Multiple ALTER TABLE statements on the same table should be combined into a single statement. + /// + /// When you run multiple ALTER TABLE statements on the same table, PostgreSQL must scan and potentially + /// rewrite the table multiple times. Each ALTER TABLE command requires acquiring locks and performing + /// table operations that can be expensive, especially on large tables. + /// + /// Combining multiple ALTER TABLE operations into a single statement with comma-separated actions + /// allows PostgreSQL to scan and modify the table only once, improving performance and reducing + /// the time locks are held. + /// + /// ## Examples + /// + /// ### Invalid + /// + /// ```sql,expect_diagnostic + /// ALTER TABLE authors ALTER COLUMN name SET NOT NULL; + /// ALTER TABLE authors ALTER COLUMN email SET NOT NULL; + /// ``` + /// + /// ### Valid + /// + /// ```sql + /// ALTER TABLE authors + /// ALTER COLUMN name SET NOT NULL, + /// ALTER COLUMN email SET NOT NULL; + /// ``` + /// + pub MultipleAlterTable { + version: "next", + name: "multipleAlterTable", + severity: Severity::Warning, + recommended: true, + sources: &[RuleSource::Eugene("W12")], + } +} + +impl Rule for MultipleAlterTable { + type Options = (); + + fn run(ctx: &RuleContext) -> Vec { + let mut diagnostics = Vec::new(); + + // Check if current statement is ALTER TABLE + if let pgt_query::NodeEnum::AlterTableStmt(stmt) = &ctx.stmt() { + if let Some(relation) = &stmt.relation { + let current_schema = &relation.schemaname; + let current_table = &relation.relname; + + // Check previous statements for ALTER TABLE on the same table + let file_ctx = ctx.file_context(); + + // Normalize schema name: treat empty string as "public" + let current_schema_normalized = if current_schema.is_empty() { + "public" + } else { + current_schema.as_str() + }; + + let has_previous_alter = + file_ctx + .previous_stmts() + .iter() + .any(|prev_stmt| match prev_stmt { + pgt_query::NodeEnum::AlterTableStmt(prev_alter) => { + if let Some(prev_relation) = &prev_alter.relation { + let prev_schema_normalized = + if prev_relation.schemaname.is_empty() { + "public" + } else { + prev_relation.schemaname.as_str() + }; + + // Match if same table and schema (treating empty schema as "public") + prev_relation.relname == *current_table + && prev_schema_normalized == current_schema_normalized + } else { + false + } + } + _ => false, + }); + + if has_previous_alter { + let schema_display = if current_schema.is_empty() { + "public" + } else { + current_schema + }; + + diagnostics.push( + RuleDiagnostic::new( + rule_category!(), + None, + markup! { + "Multiple ""ALTER TABLE"" statements found for table "{schema_display}"."{{current_table}}"." + }, + ) + .detail( + None, + "Multiple ALTER TABLE statements on the same table require scanning and potentially rewriting the table multiple times.", + ) + .note("Combine the ALTER TABLE statements into a single statement with comma-separated actions to scan the table only once."), + ); + } + } + } + + diagnostics + } +} diff --git a/crates/pgt_analyser/src/options.rs b/crates/pgt_analyser/src/options.rs index a0f4d36d5..f5a16da3d 100644 --- a/crates/pgt_analyser/src/options.rs +++ b/crates/pgt_analyser/src/options.rs @@ -26,6 +26,8 @@ pub type ChangingColumnType = ::Options; pub type ConstraintMissingNotValid = < lint :: safety :: constraint_missing_not_valid :: ConstraintMissingNotValid as pgt_analyse :: Rule > :: Options ; pub type DisallowUniqueConstraint = < lint :: safety :: disallow_unique_constraint :: DisallowUniqueConstraint as pgt_analyse :: Rule > :: Options ; +pub type MultipleAlterTable = + ::Options; pub type PreferBigInt = ::Options; pub type PreferBigintOverInt = ::Options; diff --git a/crates/pgt_analyser/tests/specs/safety/multipleAlterTable/basic.sql b/crates/pgt_analyser/tests/specs/safety/multipleAlterTable/basic.sql new file mode 100644 index 000000000..0552ce05d --- /dev/null +++ b/crates/pgt_analyser/tests/specs/safety/multipleAlterTable/basic.sql @@ -0,0 +1,4 @@ +-- expect_lint/safety/multipleAlterTable +-- Test multiple ALTER TABLE statements on the same table +ALTER TABLE authors ALTER COLUMN name SET NOT NULL; +ALTER TABLE authors ALTER COLUMN email SET NOT NULL; diff --git a/crates/pgt_analyser/tests/specs/safety/multipleAlterTable/basic.sql.snap b/crates/pgt_analyser/tests/specs/safety/multipleAlterTable/basic.sql.snap new file mode 100644 index 000000000..c861dee32 --- /dev/null +++ b/crates/pgt_analyser/tests/specs/safety/multipleAlterTable/basic.sql.snap @@ -0,0 +1,22 @@ +--- +source: crates/pgt_analyser/tests/rules_tests.rs +expression: snapshot +snapshot_kind: text +--- +# Input +``` +-- expect_lint/safety/multipleAlterTable +-- Test multiple ALTER TABLE statements on the same table +ALTER TABLE authors ALTER COLUMN name SET NOT NULL; +ALTER TABLE authors ALTER COLUMN email SET NOT NULL; + +``` + +# Diagnostics +lint/safety/multipleAlterTable ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + × Multiple ALTER TABLE statements found for table public.authors. + + i Multiple ALTER TABLE statements on the same table require scanning and potentially rewriting the table multiple times. + + i Combine the ALTER TABLE statements into a single statement with comma-separated actions to scan the table only once. diff --git a/crates/pgt_analyser/tests/specs/safety/multipleAlterTable/mixed_schema_notation.sql b/crates/pgt_analyser/tests/specs/safety/multipleAlterTable/mixed_schema_notation.sql new file mode 100644 index 000000000..e01b586f0 --- /dev/null +++ b/crates/pgt_analyser/tests/specs/safety/multipleAlterTable/mixed_schema_notation.sql @@ -0,0 +1,4 @@ +-- expect_lint/safety/multipleAlterTable +-- Test mixing implicit and explicit public schema (should match) +ALTER TABLE authors ADD COLUMN name text; +ALTER TABLE public.authors ADD COLUMN email text; diff --git a/crates/pgt_analyser/tests/specs/safety/multipleAlterTable/mixed_schema_notation.sql.snap b/crates/pgt_analyser/tests/specs/safety/multipleAlterTable/mixed_schema_notation.sql.snap new file mode 100644 index 000000000..612b9272f --- /dev/null +++ b/crates/pgt_analyser/tests/specs/safety/multipleAlterTable/mixed_schema_notation.sql.snap @@ -0,0 +1,22 @@ +--- +source: crates/pgt_analyser/tests/rules_tests.rs +expression: snapshot +snapshot_kind: text +--- +# Input +``` +-- expect_lint/safety/multipleAlterTable +-- Test mixing implicit and explicit public schema (should match) +ALTER TABLE authors ADD COLUMN name text; +ALTER TABLE public.authors ADD COLUMN email text; + +``` + +# Diagnostics +lint/safety/multipleAlterTable ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + × Multiple ALTER TABLE statements found for table public.authors. + + i Multiple ALTER TABLE statements on the same table require scanning and potentially rewriting the table multiple times. + + i Combine the ALTER TABLE statements into a single statement with comma-separated actions to scan the table only once. diff --git a/crates/pgt_analyser/tests/specs/safety/multipleAlterTable/three_alters.sql b/crates/pgt_analyser/tests/specs/safety/multipleAlterTable/three_alters.sql new file mode 100644 index 000000000..0bfba2df1 --- /dev/null +++ b/crates/pgt_analyser/tests/specs/safety/multipleAlterTable/three_alters.sql @@ -0,0 +1,5 @@ +-- expect_lint/safety/multipleAlterTable +-- Test ALTER TABLE after other statements +CREATE TABLE products (id serial PRIMARY KEY); +ALTER TABLE products ADD COLUMN description text; +ALTER TABLE products ADD COLUMN price numeric; diff --git a/crates/pgt_analyser/tests/specs/safety/multipleAlterTable/three_alters.sql.snap b/crates/pgt_analyser/tests/specs/safety/multipleAlterTable/three_alters.sql.snap new file mode 100644 index 000000000..b46c580ea --- /dev/null +++ b/crates/pgt_analyser/tests/specs/safety/multipleAlterTable/three_alters.sql.snap @@ -0,0 +1,23 @@ +--- +source: crates/pgt_analyser/tests/rules_tests.rs +expression: snapshot +snapshot_kind: text +--- +# Input +``` +-- expect_lint/safety/multipleAlterTable +-- Test ALTER TABLE after other statements +CREATE TABLE products (id serial PRIMARY KEY); +ALTER TABLE products ADD COLUMN description text; +ALTER TABLE products ADD COLUMN price numeric; + +``` + +# Diagnostics +lint/safety/multipleAlterTable ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + × Multiple ALTER TABLE statements found for table public.products. + + i Multiple ALTER TABLE statements on the same table require scanning and potentially rewriting the table multiple times. + + i Combine the ALTER TABLE statements into a single statement with comma-separated actions to scan the table only once. diff --git a/crates/pgt_analyser/tests/specs/safety/multipleAlterTable/valid_combined.sql b/crates/pgt_analyser/tests/specs/safety/multipleAlterTable/valid_combined.sql new file mode 100644 index 000000000..1ef78464b --- /dev/null +++ b/crates/pgt_analyser/tests/specs/safety/multipleAlterTable/valid_combined.sql @@ -0,0 +1,5 @@ +-- Test single ALTER TABLE with multiple actions (should be safe) +-- expect_no_diagnostics +ALTER TABLE authors + ALTER COLUMN name SET NOT NULL, + ALTER COLUMN email SET NOT NULL; diff --git a/crates/pgt_analyser/tests/specs/safety/multipleAlterTable/valid_combined.sql.snap b/crates/pgt_analyser/tests/specs/safety/multipleAlterTable/valid_combined.sql.snap new file mode 100644 index 000000000..ea80b035d --- /dev/null +++ b/crates/pgt_analyser/tests/specs/safety/multipleAlterTable/valid_combined.sql.snap @@ -0,0 +1,14 @@ +--- +source: crates/pgt_analyser/tests/rules_tests.rs +expression: snapshot +snapshot_kind: text +--- +# Input +``` +-- Test single ALTER TABLE with multiple actions (should be safe) +-- expect_no_diagnostics +ALTER TABLE authors + ALTER COLUMN name SET NOT NULL, + ALTER COLUMN email SET NOT NULL; + +``` diff --git a/crates/pgt_analyser/tests/specs/safety/multipleAlterTable/valid_different_schemas.sql b/crates/pgt_analyser/tests/specs/safety/multipleAlterTable/valid_different_schemas.sql new file mode 100644 index 000000000..1866d16e2 --- /dev/null +++ b/crates/pgt_analyser/tests/specs/safety/multipleAlterTable/valid_different_schemas.sql @@ -0,0 +1,4 @@ +-- Test ALTER TABLE on tables with same name but different schemas (should be safe) +-- expect_no_diagnostics +ALTER TABLE public.users ADD COLUMN age integer; +ALTER TABLE admin.users ADD COLUMN age integer; diff --git a/crates/pgt_analyser/tests/specs/safety/multipleAlterTable/valid_different_schemas.sql.snap b/crates/pgt_analyser/tests/specs/safety/multipleAlterTable/valid_different_schemas.sql.snap new file mode 100644 index 000000000..24822cddb --- /dev/null +++ b/crates/pgt_analyser/tests/specs/safety/multipleAlterTable/valid_different_schemas.sql.snap @@ -0,0 +1,13 @@ +--- +source: crates/pgt_analyser/tests/rules_tests.rs +expression: snapshot +snapshot_kind: text +--- +# Input +``` +-- Test ALTER TABLE on tables with same name but different schemas (should be safe) +-- expect_no_diagnostics +ALTER TABLE public.users ADD COLUMN age integer; +ALTER TABLE admin.users ADD COLUMN age integer; + +``` diff --git a/crates/pgt_analyser/tests/specs/safety/multipleAlterTable/valid_different_tables.sql b/crates/pgt_analyser/tests/specs/safety/multipleAlterTable/valid_different_tables.sql new file mode 100644 index 000000000..af6e1b722 --- /dev/null +++ b/crates/pgt_analyser/tests/specs/safety/multipleAlterTable/valid_different_tables.sql @@ -0,0 +1,4 @@ +-- Test ALTER TABLE on different tables (should be safe) +-- expect_no_diagnostics +ALTER TABLE authors ADD COLUMN bio text; +ALTER TABLE posts ADD COLUMN published_at timestamp; diff --git a/crates/pgt_analyser/tests/specs/safety/multipleAlterTable/valid_different_tables.sql.snap b/crates/pgt_analyser/tests/specs/safety/multipleAlterTable/valid_different_tables.sql.snap new file mode 100644 index 000000000..e915a2054 --- /dev/null +++ b/crates/pgt_analyser/tests/specs/safety/multipleAlterTable/valid_different_tables.sql.snap @@ -0,0 +1,13 @@ +--- +source: crates/pgt_analyser/tests/rules_tests.rs +expression: snapshot +snapshot_kind: text +--- +# Input +``` +-- Test ALTER TABLE on different tables (should be safe) +-- expect_no_diagnostics +ALTER TABLE authors ADD COLUMN bio text; +ALTER TABLE posts ADD COLUMN published_at timestamp; + +``` diff --git a/crates/pgt_analyser/tests/specs/safety/multipleAlterTable/valid_single_alter.sql b/crates/pgt_analyser/tests/specs/safety/multipleAlterTable/valid_single_alter.sql new file mode 100644 index 000000000..2deb83503 --- /dev/null +++ b/crates/pgt_analyser/tests/specs/safety/multipleAlterTable/valid_single_alter.sql @@ -0,0 +1,3 @@ +-- Test single ALTER TABLE statement (should be safe) +-- expect_no_diagnostics +ALTER TABLE authors ALTER COLUMN name SET NOT NULL; diff --git a/crates/pgt_analyser/tests/specs/safety/multipleAlterTable/valid_single_alter.sql.snap b/crates/pgt_analyser/tests/specs/safety/multipleAlterTable/valid_single_alter.sql.snap new file mode 100644 index 000000000..5bb2374a3 --- /dev/null +++ b/crates/pgt_analyser/tests/specs/safety/multipleAlterTable/valid_single_alter.sql.snap @@ -0,0 +1,12 @@ +--- +source: crates/pgt_analyser/tests/rules_tests.rs +expression: snapshot +snapshot_kind: text +--- +# Input +``` +-- Test single ALTER TABLE statement (should be safe) +-- expect_no_diagnostics +ALTER TABLE authors ALTER COLUMN name SET NOT NULL; + +``` diff --git a/crates/pgt_analyser/tests/specs/safety/multipleAlterTable/with_schema.sql b/crates/pgt_analyser/tests/specs/safety/multipleAlterTable/with_schema.sql new file mode 100644 index 000000000..b8e30e61e --- /dev/null +++ b/crates/pgt_analyser/tests/specs/safety/multipleAlterTable/with_schema.sql @@ -0,0 +1,4 @@ +-- expect_lint/safety/multipleAlterTable +-- Test multiple ALTER TABLE statements with explicit schema +ALTER TABLE public.users ADD COLUMN age integer; +ALTER TABLE public.users ADD COLUMN country text; diff --git a/crates/pgt_analyser/tests/specs/safety/multipleAlterTable/with_schema.sql.snap b/crates/pgt_analyser/tests/specs/safety/multipleAlterTable/with_schema.sql.snap new file mode 100644 index 000000000..83fbcb689 --- /dev/null +++ b/crates/pgt_analyser/tests/specs/safety/multipleAlterTable/with_schema.sql.snap @@ -0,0 +1,22 @@ +--- +source: crates/pgt_analyser/tests/rules_tests.rs +expression: snapshot +snapshot_kind: text +--- +# Input +``` +-- expect_lint/safety/multipleAlterTable +-- Test multiple ALTER TABLE statements with explicit schema +ALTER TABLE public.users ADD COLUMN age integer; +ALTER TABLE public.users ADD COLUMN country text; + +``` + +# Diagnostics +lint/safety/multipleAlterTable ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + × Multiple ALTER TABLE statements found for table public.users. + + i Multiple ALTER TABLE statements on the same table require scanning and potentially rewriting the table multiple times. + + i Combine the ALTER TABLE statements into a single statement with comma-separated actions to scan the table only once. diff --git a/crates/pgt_configuration/src/analyser/linter/rules.rs b/crates/pgt_configuration/src/analyser/linter/rules.rs index 3788c81b4..a37e08ab4 100644 --- a/crates/pgt_configuration/src/analyser/linter/rules.rs +++ b/crates/pgt_configuration/src/analyser/linter/rules.rs @@ -196,6 +196,9 @@ pub struct Safety { #[serde(skip_serializing_if = "Option::is_none")] pub disallow_unique_constraint: Option>, + #[doc = "Multiple ALTER TABLE statements on the same table should be combined into a single statement."] + #[serde(skip_serializing_if = "Option::is_none")] + pub multiple_alter_table: Option>, #[doc = "Prefer BIGINT over smaller integer types."] #[serde(skip_serializing_if = "Option::is_none")] pub prefer_big_int: Option>, @@ -259,6 +262,7 @@ impl Safety { "changingColumnType", "constraintMissingNotValid", "disallowUniqueConstraint", + "multipleAlterTable", "preferBigInt", "preferBigintOverInt", "preferBigintOverSmallint", @@ -283,6 +287,7 @@ impl Safety { RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[8]), RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[10]), RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[11]), + RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[16]), ]; const ALL_RULES_AS_FILTERS: &'static [RuleFilter<'static>] = &[ RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[0]), @@ -314,6 +319,7 @@ impl Safety { RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[26]), RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[27]), RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[28]), + RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[29]), ]; #[doc = r" Retrieves the recommended rules"] pub(crate) fn is_recommended_true(&self) -> bool { @@ -410,71 +416,76 @@ impl Safety { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[15])); } } - if let Some(rule) = self.prefer_big_int.as_ref() { + if let Some(rule) = self.multiple_alter_table.as_ref() { if rule.is_enabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[16])); } } - if let Some(rule) = self.prefer_bigint_over_int.as_ref() { + if let Some(rule) = self.prefer_big_int.as_ref() { if rule.is_enabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[17])); } } - if let Some(rule) = self.prefer_bigint_over_smallint.as_ref() { + if let Some(rule) = self.prefer_bigint_over_int.as_ref() { if rule.is_enabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[18])); } } - if let Some(rule) = self.prefer_identity.as_ref() { + if let Some(rule) = self.prefer_bigint_over_smallint.as_ref() { if rule.is_enabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[19])); } } - if let Some(rule) = self.prefer_jsonb.as_ref() { + if let Some(rule) = self.prefer_identity.as_ref() { if rule.is_enabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[20])); } } - if let Some(rule) = self.prefer_robust_stmts.as_ref() { + if let Some(rule) = self.prefer_jsonb.as_ref() { if rule.is_enabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[21])); } } - if let Some(rule) = self.prefer_text_field.as_ref() { + if let Some(rule) = self.prefer_robust_stmts.as_ref() { if rule.is_enabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[22])); } } - if let Some(rule) = self.prefer_timestamptz.as_ref() { + if let Some(rule) = self.prefer_text_field.as_ref() { if rule.is_enabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[23])); } } - if let Some(rule) = self.renaming_column.as_ref() { + if let Some(rule) = self.prefer_timestamptz.as_ref() { if rule.is_enabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[24])); } } - if let Some(rule) = self.renaming_table.as_ref() { + if let Some(rule) = self.renaming_column.as_ref() { if rule.is_enabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[25])); } } - if let Some(rule) = self.require_concurrent_index_creation.as_ref() { + if let Some(rule) = self.renaming_table.as_ref() { if rule.is_enabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[26])); } } - if let Some(rule) = self.require_concurrent_index_deletion.as_ref() { + if let Some(rule) = self.require_concurrent_index_creation.as_ref() { if rule.is_enabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[27])); } } - if let Some(rule) = self.transaction_nesting.as_ref() { + if let Some(rule) = self.require_concurrent_index_deletion.as_ref() { if rule.is_enabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[28])); } } + if let Some(rule) = self.transaction_nesting.as_ref() { + if rule.is_enabled() { + index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[29])); + } + } index_set } pub(crate) fn get_disabled_rules(&self) -> FxHashSet> { @@ -559,71 +570,76 @@ impl Safety { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[15])); } } - if let Some(rule) = self.prefer_big_int.as_ref() { + if let Some(rule) = self.multiple_alter_table.as_ref() { if rule.is_disabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[16])); } } - if let Some(rule) = self.prefer_bigint_over_int.as_ref() { + if let Some(rule) = self.prefer_big_int.as_ref() { if rule.is_disabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[17])); } } - if let Some(rule) = self.prefer_bigint_over_smallint.as_ref() { + if let Some(rule) = self.prefer_bigint_over_int.as_ref() { if rule.is_disabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[18])); } } - if let Some(rule) = self.prefer_identity.as_ref() { + if let Some(rule) = self.prefer_bigint_over_smallint.as_ref() { if rule.is_disabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[19])); } } - if let Some(rule) = self.prefer_jsonb.as_ref() { + if let Some(rule) = self.prefer_identity.as_ref() { if rule.is_disabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[20])); } } - if let Some(rule) = self.prefer_robust_stmts.as_ref() { + if let Some(rule) = self.prefer_jsonb.as_ref() { if rule.is_disabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[21])); } } - if let Some(rule) = self.prefer_text_field.as_ref() { + if let Some(rule) = self.prefer_robust_stmts.as_ref() { if rule.is_disabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[22])); } } - if let Some(rule) = self.prefer_timestamptz.as_ref() { + if let Some(rule) = self.prefer_text_field.as_ref() { if rule.is_disabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[23])); } } - if let Some(rule) = self.renaming_column.as_ref() { + if let Some(rule) = self.prefer_timestamptz.as_ref() { if rule.is_disabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[24])); } } - if let Some(rule) = self.renaming_table.as_ref() { + if let Some(rule) = self.renaming_column.as_ref() { if rule.is_disabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[25])); } } - if let Some(rule) = self.require_concurrent_index_creation.as_ref() { + if let Some(rule) = self.renaming_table.as_ref() { if rule.is_disabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[26])); } } - if let Some(rule) = self.require_concurrent_index_deletion.as_ref() { + if let Some(rule) = self.require_concurrent_index_creation.as_ref() { if rule.is_disabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[27])); } } - if let Some(rule) = self.transaction_nesting.as_ref() { + if let Some(rule) = self.require_concurrent_index_deletion.as_ref() { if rule.is_disabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[28])); } } + if let Some(rule) = self.transaction_nesting.as_ref() { + if rule.is_disabled() { + index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[29])); + } + } index_set } #[doc = r" Checks if, given a rule name, matches one of the rules contained in this category"] @@ -669,6 +685,7 @@ impl Safety { "changingColumnType" => Severity::Warning, "constraintMissingNotValid" => Severity::Warning, "disallowUniqueConstraint" => Severity::Error, + "multipleAlterTable" => Severity::Warning, "preferBigInt" => Severity::Warning, "preferBigintOverInt" => Severity::Warning, "preferBigintOverSmallint" => Severity::Warning, @@ -754,6 +771,10 @@ impl Safety { .disallow_unique_constraint .as_ref() .map(|conf| (conf.level(), conf.get_options())), + "multipleAlterTable" => self + .multiple_alter_table + .as_ref() + .map(|conf| (conf.level(), conf.get_options())), "preferBigInt" => self .prefer_big_int .as_ref() diff --git a/crates/pgt_diagnostics_categories/src/categories.rs b/crates/pgt_diagnostics_categories/src/categories.rs index 856f9f983..1a507f1b7 100644 --- a/crates/pgt_diagnostics_categories/src/categories.rs +++ b/crates/pgt_diagnostics_categories/src/categories.rs @@ -13,6 +13,7 @@ // must be between `define_categories! {\n` and `\n ;\n`. define_categories! { + "lint/safety/addSerialColumn": "https://pgtools.dev/latest/rules/add-serial-column", "lint/safety/addingFieldWithDefault": "https://pgtools.dev/latest/rules/adding-field-with-default", "lint/safety/addingForeignKeyConstraint": "https://pgtools.dev/latest/rules/adding-foreign-key-constraint", "lint/safety/addingNotNullField": "https://pgtools.dev/latest/rules/adding-not-null-field", @@ -24,11 +25,11 @@ define_categories! { "lint/safety/banDropDatabase": "https://pgtools.dev/latest/rules/ban-drop-database", "lint/safety/banDropNotNull": "https://pgtools.dev/latest/rules/ban-drop-not-null", "lint/safety/banDropTable": "https://pgtools.dev/latest/rules/ban-drop-table", - "lint/safety/addSerialColumn": "https://pgtools.dev/latest/rules/add-serial-column", "lint/safety/banTruncateCascade": "https://pgtools.dev/latest/rules/ban-truncate-cascade", "lint/safety/changingColumnType": "https://pgtools.dev/latest/rules/changing-column-type", "lint/safety/constraintMissingNotValid": "https://pgtools.dev/latest/rules/constraint-missing-not-valid", "lint/safety/disallowUniqueConstraint": "https://pgtools.dev/latest/rules/disallow-unique-constraint", + "lint/safety/multipleAlterTable": "https://pgtools.dev/latest/rules/multiple-alter-table", "lint/safety/preferBigInt": "https://pgtools.dev/latest/rules/prefer-big-int", "lint/safety/preferBigintOverInt": "https://pgtools.dev/latest/rules/prefer-bigint-over-int", "lint/safety/preferBigintOverSmallint": "https://pgtools.dev/latest/rules/prefer-bigint-over-smallint", diff --git a/docs/reference/rule_sources.md b/docs/reference/rule_sources.md index 98f401c2f..ec056b7af 100644 --- a/docs/reference/rule_sources.md +++ b/docs/reference/rule_sources.md @@ -8,6 +8,7 @@ _No exclusive rules available._ | ---- | ---- | | [E11](https://kaveland.no/eugene/hints/E11/index.html) |[addSerialColumn](../rules/add-serial-column) | | [E3](https://kaveland.no/eugene/hints/E3/index.html) |[preferJsonb](../rules/prefer-jsonb) | +| [W12](https://kaveland.no/eugene/hints/W12/index.html) |[multipleAlterTable](../rules/multiple-alter-table) | ### Squawk | Squawk Rule Name | Rule Name | | ---- | ---- | diff --git a/docs/reference/rules.md b/docs/reference/rules.md index 836b2c982..2e988b932 100644 --- a/docs/reference/rules.md +++ b/docs/reference/rules.md @@ -28,6 +28,7 @@ Rules that detect potential safety issues in your code. | [changingColumnType](./changing-column-type) | Changing a column type may break existing clients. | | | [constraintMissingNotValid](./constraint-missing-not-valid) | Adding constraints without NOT VALID blocks all reads and writes. | | | [disallowUniqueConstraint](./disallow-unique-constraint) | Disallow adding a UNIQUE constraint without using an existing index. | | +| [multipleAlterTable](./multiple-alter-table) | Multiple ALTER TABLE statements on the same table should be combined into a single statement. | ✅ | | [preferBigInt](./prefer-big-int) | Prefer BIGINT over smaller integer types. | | | [preferBigintOverInt](./prefer-bigint-over-int) | Prefer BIGINT over INT/INTEGER types. | | | [preferBigintOverSmallint](./prefer-bigint-over-smallint) | Prefer BIGINT over SMALLINT types. | | diff --git a/docs/reference/rules/multiple-alter-table.md b/docs/reference/rules/multiple-alter-table.md new file mode 100644 index 000000000..e5b1e49f2 --- /dev/null +++ b/docs/reference/rules/multiple-alter-table.md @@ -0,0 +1,56 @@ +# multipleAlterTable +**Diagnostic Category: `lint/safety/multipleAlterTable`** + +**Since**: `vnext` + +> [!NOTE] +> This rule is recommended. A diagnostic error will appear when linting your code. + +**Sources**: +- Inspired from: eugene/W12 + +## Description +Multiple ALTER TABLE statements on the same table should be combined into a single statement. + +When you run multiple ALTER TABLE statements on the same table, PostgreSQL must scan and potentially +rewrite the table multiple times. Each ALTER TABLE command requires acquiring locks and performing +table operations that can be expensive, especially on large tables. + +Combining multiple ALTER TABLE operations into a single statement with comma-separated actions +allows PostgreSQL to scan and modify the table only once, improving performance and reducing +the time locks are held. + +## Examples + +### Invalid + +```sql +ALTER TABLE authors ALTER COLUMN name SET NOT NULL; +ALTER TABLE authors ALTER COLUMN email SET NOT NULL; +``` + +```sh +``` + +### Valid + +```sql +ALTER TABLE authors + ALTER COLUMN name SET NOT NULL, + ALTER COLUMN email SET NOT NULL; +``` + +## How to configure +```json + +{ + "linter": { + "rules": { + "safety": { + "multipleAlterTable": "error" + } + } + } +} + +``` diff --git a/docs/schema.json b/docs/schema.json index e3c4ab147..68c448365 100644 --- a/docs/schema.json +++ b/docs/schema.json @@ -539,6 +539,17 @@ } ] }, + "multipleAlterTable": { + "description": "Multiple ALTER TABLE statements on the same table should be combined into a single statement.", + "anyOf": [ + { + "$ref": "#/definitions/RuleConfiguration" + }, + { + "type": "null" + } + ] + }, "preferBigInt": { "description": "Prefer BIGINT over smaller integer types.", "anyOf": [ diff --git a/packages/@postgrestools/backend-jsonrpc/src/workspace.ts b/packages/@postgrestools/backend-jsonrpc/src/workspace.ts index 379eef876..b170e6838 100644 --- a/packages/@postgrestools/backend-jsonrpc/src/workspace.ts +++ b/packages/@postgrestools/backend-jsonrpc/src/workspace.ts @@ -63,6 +63,7 @@ export interface Advices { advices: Advice[]; } export type Category = + | "lint/safety/addSerialColumn" | "lint/safety/addingFieldWithDefault" | "lint/safety/addingForeignKeyConstraint" | "lint/safety/addingNotNullField" @@ -74,11 +75,11 @@ export type Category = | "lint/safety/banDropDatabase" | "lint/safety/banDropNotNull" | "lint/safety/banDropTable" - | "lint/safety/addSerialColumn" | "lint/safety/banTruncateCascade" | "lint/safety/changingColumnType" | "lint/safety/constraintMissingNotValid" | "lint/safety/disallowUniqueConstraint" + | "lint/safety/multipleAlterTable" | "lint/safety/preferBigInt" | "lint/safety/preferBigintOverInt" | "lint/safety/preferBigintOverSmallint" @@ -504,6 +505,10 @@ export interface Safety { * Disallow adding a UNIQUE constraint without using an existing index. */ disallowUniqueConstraint?: RuleConfiguration_for_Null; + /** + * Multiple ALTER TABLE statements on the same table should be combined into a single statement. + */ + multipleAlterTable?: RuleConfiguration_for_Null; /** * Prefer BIGINT over smaller integer types. */ From 6ed1eca0b7b206107306dcafceea9fd9503fea9e Mon Sep 17 00:00:00 2001 From: psteinroe Date: Sun, 19 Oct 2025 12:39:41 +0200 Subject: [PATCH 3/8] chore: add agentic document --- agentic/port_eugene_rules.md | 378 +++++++++++++++++++++++++++++++++++ 1 file changed, 378 insertions(+) create mode 100644 agentic/port_eugene_rules.md diff --git a/agentic/port_eugene_rules.md b/agentic/port_eugene_rules.md new file mode 100644 index 000000000..00479a911 --- /dev/null +++ b/agentic/port_eugene_rules.md @@ -0,0 +1,378 @@ +# Porting Rules from Eugene to postgres-language-server + +This document tracks the progress of porting lint rules from [Eugene](https://github.com/kaaveland/eugene) to the postgres-language-server analyzer. + +## Overview + +Eugene is a PostgreSQL migration linter that detects dangerous operations. We are porting its rules to the postgres-language-server to provide similar safety checks within the language server environment. + +**Eugene source location**: `eugene/eugene/src/lints/rules.rs` +**Hint metadata location**: `eugene/eugene/src/hint_data.rs` +**Example SQL files**: `eugene/eugene/examples/*/` + +## Step-by-Step Porting Process + +### 1. Understand the Rule + +1. **Read Eugene's implementation** in `eugene/eugene/src/lints/rules.rs` + - Find the rule function (e.g., `added_serial_column`) + - Understand the AST patterns it matches + - Note any special logic (e.g., checking previous statements) + +2. **Read the hint metadata** in `eugene/eugene/src/hint_data.rs` + - Get the ID (e.g., "E11") + - Get name, condition, effect, and workaround text + - This provides documentation content + +3. **Review example SQL** in `eugene/eugene/examples//` + - `bad.sql` - Invalid cases that should trigger the rule + - `good.sql` - Valid cases that should NOT trigger + +### 2. Create the Rule + +```bash +# Create rule with appropriate severity (error/warn) +just new-lintrule safety + +# Example: +just new-lintrule safety addSerialColumn error +``` + +This generates: +- `crates/pgt_analyser/src/lint/safety/.rs` +- `crates/pgt_analyser/tests/specs/safety//basic.sql` +- Updates to configuration files +- Diagnostic category registration + +### 3. Implement the Rule + +**File**: `crates/pgt_analyser/src/lint/safety/.rs` + +#### Key Components: + +```rust +use pgt_analyse::{context::RuleContext, declare_lint_rule, Rule, RuleDiagnostic, RuleSource}; +use pgt_console::markup; +use pgt_diagnostics::Severity; + +declare_lint_rule! { + /// Brief one-line description (shown in lists). + /// + /// Detailed explanation of what the rule detects and why it's problematic. + /// Explain the PostgreSQL behavior and performance/safety implications. + /// + /// ## Examples + /// + /// ### Invalid + /// + /// ```sql,expect_diagnostic + /// -- SQL that should trigger the rule + /// ALTER TABLE users ADD COLUMN id serial; + /// ``` + /// + /// ### Valid + /// + /// ```sql + /// -- SQL that should NOT trigger + /// CREATE TABLE users (id serial PRIMARY KEY); + /// ``` + /// + pub RuleName { + version: "next", + name: "ruleName", + severity: Severity::Error, // or Warning + recommended: true, // or false + sources: &[RuleSource::Eugene("")], // e.g., "E11" + } +} + +impl Rule for RuleName { + type Options = (); + + fn run(ctx: &RuleContext) -> Vec { + let mut diagnostics = Vec::new(); + + // Pattern match on the statement type + if let pgt_query::NodeEnum::AlterTableStmt(stmt) = &ctx.stmt() { + // Rule logic here + + if /* condition */ { + diagnostics.push( + RuleDiagnostic::new( + rule_category!(), + None, + markup! { + "Error message with ""formatting""." + }, + ) + .detail(None, "Additional context about the problem.") + .note("Suggested fix or workaround."), + ); + } + } + + diagnostics + } +} +``` + +#### Important Patterns: + +**Accessing previous statements** (for rules like `multipleAlterTable`): +```rust +let file_ctx = ctx.file_context(); +let previous = file_ctx.previous_stmts(); +``` + +**Schema normalization** (treating empty schema as "public"): +```rust +let schema_normalized = if schema.is_empty() { + "public" +} else { + schema.as_str() +}; +``` + +**Checking for specific ALTER TABLE actions**: +```rust +for cmd in &stmt.cmds { + if let Some(pgt_query::NodeEnum::AlterTableCmd(cmd)) = &cmd.node { + if cmd.subtype() == pgt_query::protobuf::AlterTableType::AtAddColumn { + // Handle ADD COLUMN + } + } +} +``` + +**Extracting column type**: +```rust +if let Some(pgt_query::NodeEnum::ColumnDef(col_def)) = &cmd.def.as_ref().and_then(|d| d.node.as_ref()) { + if let Some(type_name) = &col_def.type_name { + let type_str = get_type_name(type_name); + } +} + +fn get_type_name(type_name: &pgt_query::protobuf::TypeName) -> String { + type_name + .names + .iter() + .filter_map(|n| { + if let Some(pgt_query::NodeEnum::String(s)) = &n.node { + Some(s.sval.as_str()) + } else { + None + } + }) + .collect::>() + .join(".") +} +``` + +### 4. Create Comprehensive Tests + +**Directory**: `crates/pgt_analyser/tests/specs/safety//` + +Create multiple test files covering: + +#### Invalid Cases (should trigger): +```sql +-- expect_lint/safety/ +-- Description of what this tests + +``` + +#### Valid Cases (should NOT trigger): +```sql +-- Description of what this tests +-- expect_no_diagnostics + +``` + +#### Example Test Structure: +``` +tests/specs/safety/addSerialColumn/ +├── basic.sql # Basic case triggering the rule +├── bigserial.sql # Variant (bigserial type) +├── generated_stored.sql # Another variant (GENERATED) +├── valid_regular_column.sql # Valid: regular column +└── valid_create_table.sql # Valid: CREATE TABLE context +``` + +**Run tests and accept snapshots**: +```bash +cargo insta test -p pgt_analyser --accept +``` + +### 5. Verify and Generate Code + +```bash +# Check compilation +cargo check + +# Generate lint code and documentation +just gen-lint + +# Run all tests +cargo test -p pgt_analyser --test rules_tests + +# Final verification +just ready +``` + +### 6. Test the Rule Manually + +Create a test SQL file: +```sql +-- test.sql +ALTER TABLE users ADD COLUMN id serial; +``` + +Run the CLI: +```bash +cargo run -p pgt_cli -- check /path/to/test.sql +``` + +## Common Pitfalls and Solutions + +### 1. Schema Matching + +**Problem**: Need to match tables across statements with different schema notations. + +**Solution**: Normalize schema names: +```rust +let schema_normalized = if schema.is_empty() { "public" } else { schema.as_str() }; +``` + +### 2. AST Navigation + +**Problem**: Eugene uses a simplified AST (`StatementSummary`), but pgt uses the full PostgreSQL AST. + +**Solution**: Use pattern matching and helper functions. Look at existing rules for examples. + +### 3. File Context Rules + +**Problem**: Rules that need to track state across statements (like `multipleAlterTable`). + +**Solution**: Use `ctx.file_context()` to access `AnalysedFileContext`: +```rust +let file_ctx = ctx.file_context(); +let previous_stmts = file_ctx.previous_stmts(); +``` + +### 4. Transaction State + +**Problem**: Some Eugene rules check transaction state (e.g., `RUNNING_STATEMENT_WHILE_HOLDING_ACCESS_EXCLUSIVE`). + +**Solution**: This is more complex and may require extending the `AnalysedFileContext` to track transaction state. Consider implementing simpler rules first. + +### 5. Test Expectations + +**Problem**: `expect_lint` expects exactly ONE diagnostic, but rule generates multiple. + +**Solution**: Either: +- Split into separate test files (one diagnostic each) +- Adjust the test to only trigger once +- Use expect_diagnostic for each occurrence (check existing tests) + +## Rule Mapping Considerations + +### Overlapping Rules + +Some Eugene rules may overlap with existing pgt rules: + +| Eugene Rule | Potential PGT Overlap | Action | +|-------------|----------------------|--------| +| `SET_COLUMN_TYPE_TO_JSON` | `preferJsonb` | Review both, may enhance existing | +| `CREATE_INDEX_NONCONCURRENTLY` | `requireConcurrentIndexCreation` | Review both | +| `CHANGE_COLUMN_TYPE` | `changingColumnType` | Review both | +| `ADD_NEW_UNIQUE_CONSTRAINT_WITHOUT_USING_INDEX` | `disallowUniqueConstraint` | Review both | + +**When overlap exists**: +1. Compare implementations +2. If Eugene's is more comprehensive, consider updating the existing rule +3. If they cover different aspects, keep both +4. Document any differences + +### Transaction-Aware Rules + +These rules require tracking transaction state across multiple statements: + +- `RUNNING_STATEMENT_WHILE_HOLDING_ACCESS_EXCLUSIVE` (E4) +- `LOCKTIMEOUT_WARNING` (E9) + +**Approach**: +1. First implement simpler, statement-level rules +2. Design transaction state tracking in `AnalysedFileContext` +3. Add fields to track: + - Current transaction state (BEGIN/COMMIT/ROLLBACK) + - Locks held + - Lock timeout settings +4. Update state as statements are processed + +## Eugene AST vs PostgreSQL AST + +### Eugene's Simplified AST + +Eugene uses `StatementSummary` enum with simplified representations: +```rust +enum StatementSummary { + AlterTable { schema: String, name: String, actions: Vec }, + CreateIndex { schema: String, idxname: String, concurrently: bool, target: String }, + // ... +} + +enum AlterTableAction { + AddColumn { column: String, type_name: String, stored_generated: bool, ... }, + SetType { column: String, type_name: String }, + // ... +} +``` + +### PostgreSQL AST (pgt_query) + +We use the full PostgreSQL protobuf AST: +```rust +pgt_query::NodeEnum::AlterTableStmt(stmt) + -> stmt.cmds: Vec + -> NodeEnum::AlterTableCmd(cmd) + -> cmd.subtype: AlterTableType + -> cmd.def: Option + -> NodeEnum::ColumnDef(col_def) +``` + +**Translation Strategy**: +1. Look at Eugene's simplified logic +2. Map to corresponding PostgreSQL AST nodes +3. Use existing pgt rules as references +4. Check `pgt_query::protobuf` for available types/enums + +## Useful References + +- **Eugene source**: `eugene/eugene/src/lints/rules.rs` +- **Existing pgt rules**: `crates/pgt_analyser/src/lint/safety/` +- **Contributing guide**: `crates/pgt_analyser/CONTRIBUTING.md` +- **AST types**: `crates/pgt_query/src/lib.rs` +- **PostgreSQL protobuf**: `pgt_query::protobuf` module + +## Next Steps + +1. **Priority**: Port high-priority safety rules (E1, E2, E6, E7, E9) +2. **Review overlaps**: Check if E3, E5, E6, E7 overlap with existing rules +3. **Transaction tracking**: Design transaction state tracking for E4, E9 +4. **Documentation**: Update Eugene source attribution in ported rules +5. **Testing**: Ensure comprehensive test coverage for all ported rules + +## Template Checklist + +When porting a new rule, ensure: + +- [ ] Rule implementation in `src/lint/safety/.rs` +- [ ] Documentation with examples (invalid and valid cases) +- [ ] `sources: &[RuleSource::Eugene("")]` attribution +- [ ] At least 3-5 test files (mix of invalid and valid) +- [ ] Snapshot tests accepted with `cargo insta test --accept` +- [ ] All tests pass: `cargo test -p pgt_analyser --test rules_tests` +- [ ] Compilation clean: `cargo check` +- [ ] Code generation: `just gen-lint` +- [ ] Manual CLI test with sample SQL +- [ ] Update this document with completed rule From bc01ca13f21f7b0be97ecbafcdf1495630aa7b7d Mon Sep 17 00:00:00 2001 From: psteinroe Date: Sun, 19 Oct 2025 12:48:31 +0200 Subject: [PATCH 4/8] feat(analyser): creating enum --- crates/pgt_analyser/src/lint/safety.rs | 3 +- .../src/lint/safety/creating_enum.rs | 82 +++++++++++++++++++ crates/pgt_analyser/src/options.rs | 1 + .../tests/specs/safety/creatingEnum/basic.sql | 2 + .../specs/safety/creatingEnum/basic.sql.snap | 19 +++++ .../specs/safety/creatingEnum/simple_enum.sql | 3 + .../safety/creatingEnum/simple_enum.sql.snap | 21 +++++ .../creatingEnum/valid_create_table.sql | 6 ++ .../creatingEnum/valid_create_table.sql.snap | 15 ++++ .../creatingEnum/valid_lookup_table.sql | 12 +++ .../creatingEnum/valid_lookup_table.sql.snap | 21 +++++ .../specs/safety/creatingEnum/with_schema.sql | 3 + .../safety/creatingEnum/with_schema.sql.snap | 21 +++++ .../src/analyser/linter/rules.rs | 82 ++++++++++++------- .../src/categories.rs | 1 + docs/reference/rule_sources.md | 1 + docs/reference/rules.md | 1 + docs/reference/rules/creating-enum.md | 69 ++++++++++++++++ docs/schema.json | 11 +++ .../backend-jsonrpc/src/workspace.ts | 5 ++ 20 files changed, 347 insertions(+), 32 deletions(-) create mode 100644 crates/pgt_analyser/src/lint/safety/creating_enum.rs create mode 100644 crates/pgt_analyser/tests/specs/safety/creatingEnum/basic.sql create mode 100644 crates/pgt_analyser/tests/specs/safety/creatingEnum/basic.sql.snap create mode 100644 crates/pgt_analyser/tests/specs/safety/creatingEnum/simple_enum.sql create mode 100644 crates/pgt_analyser/tests/specs/safety/creatingEnum/simple_enum.sql.snap create mode 100644 crates/pgt_analyser/tests/specs/safety/creatingEnum/valid_create_table.sql create mode 100644 crates/pgt_analyser/tests/specs/safety/creatingEnum/valid_create_table.sql.snap create mode 100644 crates/pgt_analyser/tests/specs/safety/creatingEnum/valid_lookup_table.sql create mode 100644 crates/pgt_analyser/tests/specs/safety/creatingEnum/valid_lookup_table.sql.snap create mode 100644 crates/pgt_analyser/tests/specs/safety/creatingEnum/with_schema.sql create mode 100644 crates/pgt_analyser/tests/specs/safety/creatingEnum/with_schema.sql.snap create mode 100644 docs/reference/rules/creating-enum.md diff --git a/crates/pgt_analyser/src/lint/safety.rs b/crates/pgt_analyser/src/lint/safety.rs index 3cdd5dd3e..c20461b19 100644 --- a/crates/pgt_analyser/src/lint/safety.rs +++ b/crates/pgt_analyser/src/lint/safety.rs @@ -16,6 +16,7 @@ pub mod ban_drop_table; pub mod ban_truncate_cascade; pub mod changing_column_type; pub mod constraint_missing_not_valid; +pub mod creating_enum; pub mod disallow_unique_constraint; pub mod multiple_alter_table; pub mod prefer_big_int; @@ -31,4 +32,4 @@ pub mod renaming_table; pub mod require_concurrent_index_creation; pub mod require_concurrent_index_deletion; pub mod transaction_nesting; -declare_lint_group! { pub Safety { name : "safety" , rules : [self :: add_serial_column :: AddSerialColumn , self :: adding_field_with_default :: AddingFieldWithDefault , self :: adding_foreign_key_constraint :: AddingForeignKeyConstraint , self :: adding_not_null_field :: AddingNotNullField , self :: adding_primary_key_constraint :: AddingPrimaryKeyConstraint , self :: adding_required_field :: AddingRequiredField , self :: ban_char_field :: BanCharField , self :: ban_concurrent_index_creation_in_transaction :: BanConcurrentIndexCreationInTransaction , self :: ban_drop_column :: BanDropColumn , self :: ban_drop_database :: BanDropDatabase , self :: ban_drop_not_null :: BanDropNotNull , self :: ban_drop_table :: BanDropTable , self :: ban_truncate_cascade :: BanTruncateCascade , self :: changing_column_type :: ChangingColumnType , self :: constraint_missing_not_valid :: ConstraintMissingNotValid , self :: disallow_unique_constraint :: DisallowUniqueConstraint , self :: multiple_alter_table :: MultipleAlterTable , self :: prefer_big_int :: PreferBigInt , self :: prefer_bigint_over_int :: PreferBigintOverInt , self :: prefer_bigint_over_smallint :: PreferBigintOverSmallint , self :: prefer_identity :: PreferIdentity , self :: prefer_jsonb :: PreferJsonb , self :: prefer_robust_stmts :: PreferRobustStmts , self :: prefer_text_field :: PreferTextField , self :: prefer_timestamptz :: PreferTimestamptz , self :: renaming_column :: RenamingColumn , self :: renaming_table :: RenamingTable , self :: require_concurrent_index_creation :: RequireConcurrentIndexCreation , self :: require_concurrent_index_deletion :: RequireConcurrentIndexDeletion , self :: transaction_nesting :: TransactionNesting ,] } } +declare_lint_group! { pub Safety { name : "safety" , rules : [self :: add_serial_column :: AddSerialColumn , self :: adding_field_with_default :: AddingFieldWithDefault , self :: adding_foreign_key_constraint :: AddingForeignKeyConstraint , self :: adding_not_null_field :: AddingNotNullField , self :: adding_primary_key_constraint :: AddingPrimaryKeyConstraint , self :: adding_required_field :: AddingRequiredField , self :: ban_char_field :: BanCharField , self :: ban_concurrent_index_creation_in_transaction :: BanConcurrentIndexCreationInTransaction , self :: ban_drop_column :: BanDropColumn , self :: ban_drop_database :: BanDropDatabase , self :: ban_drop_not_null :: BanDropNotNull , self :: ban_drop_table :: BanDropTable , self :: ban_truncate_cascade :: BanTruncateCascade , self :: changing_column_type :: ChangingColumnType , self :: constraint_missing_not_valid :: ConstraintMissingNotValid , self :: creating_enum :: CreatingEnum , self :: disallow_unique_constraint :: DisallowUniqueConstraint , self :: multiple_alter_table :: MultipleAlterTable , self :: prefer_big_int :: PreferBigInt , self :: prefer_bigint_over_int :: PreferBigintOverInt , self :: prefer_bigint_over_smallint :: PreferBigintOverSmallint , self :: prefer_identity :: PreferIdentity , self :: prefer_jsonb :: PreferJsonb , self :: prefer_robust_stmts :: PreferRobustStmts , self :: prefer_text_field :: PreferTextField , self :: prefer_timestamptz :: PreferTimestamptz , self :: renaming_column :: RenamingColumn , self :: renaming_table :: RenamingTable , self :: require_concurrent_index_creation :: RequireConcurrentIndexCreation , self :: require_concurrent_index_deletion :: RequireConcurrentIndexDeletion , self :: transaction_nesting :: TransactionNesting ,] } } diff --git a/crates/pgt_analyser/src/lint/safety/creating_enum.rs b/crates/pgt_analyser/src/lint/safety/creating_enum.rs new file mode 100644 index 000000000..2743a3d67 --- /dev/null +++ b/crates/pgt_analyser/src/lint/safety/creating_enum.rs @@ -0,0 +1,82 @@ +use pgt_analyse::{Rule, RuleDiagnostic, RuleSource, context::RuleContext, declare_lint_rule}; +use pgt_console::markup; +use pgt_diagnostics::Severity; + +declare_lint_rule! { + /// Creating enum types is not recommended for new applications. + /// + /// Enumerated types have several limitations that make them difficult to work with in production: + /// + /// - Removing values from an enum requires complex migrations and is not supported directly + /// - Adding values to an enum requires an ACCESS EXCLUSIVE lock in some PostgreSQL versions + /// - Associating additional data with enum values is impossible without restructuring + /// - Renaming enum values requires careful migration planning + /// + /// A lookup table with a foreign key constraint provides more flexibility and is easier to maintain. + /// + /// ## Examples + /// + /// ### Invalid + /// + /// ```sql,expect_diagnostic + /// CREATE TYPE document_type AS ENUM ('invoice', 'receipt', 'other'); + /// ``` + /// + /// ### Valid + /// + /// ```sql + /// -- Use a lookup table instead + /// CREATE TABLE document_type ( + /// type_name TEXT PRIMARY KEY + /// ); + /// INSERT INTO document_type VALUES ('invoice'), ('receipt'), ('other'); + /// ``` + /// + pub CreatingEnum { + version: "next", + name: "creatingEnum", + severity: Severity::Warning, + recommended: false, + sources: &[RuleSource::Eugene("W13")], + } +} + +impl Rule for CreatingEnum { + type Options = (); + + fn run(ctx: &RuleContext) -> Vec { + let mut diagnostics = Vec::new(); + + if let pgt_query::NodeEnum::CreateEnumStmt(stmt) = &ctx.stmt() { + let type_name = get_type_name(&stmt.type_name); + + diagnostics.push( + RuleDiagnostic::new( + rule_category!(), + None, + markup! { + "Creating enum type "{type_name}" is not recommended." + }, + ) + .detail(None, "Enum types are difficult to modify: removing values requires complex migrations, and associating additional data with values is not possible.") + .note("Consider using a lookup table with a foreign key constraint instead, which provides more flexibility and easier maintenance."), + ); + } + + diagnostics + } +} + +fn get_type_name(type_name_nodes: &[pgt_query::protobuf::Node]) -> String { + type_name_nodes + .iter() + .filter_map(|n| { + if let Some(pgt_query::NodeEnum::String(s)) = &n.node { + Some(s.sval.as_str()) + } else { + None + } + }) + .collect::>() + .join(".") +} diff --git a/crates/pgt_analyser/src/options.rs b/crates/pgt_analyser/src/options.rs index f5a16da3d..7de988bdd 100644 --- a/crates/pgt_analyser/src/options.rs +++ b/crates/pgt_analyser/src/options.rs @@ -25,6 +25,7 @@ pub type BanTruncateCascade = pub type ChangingColumnType = ::Options; pub type ConstraintMissingNotValid = < lint :: safety :: constraint_missing_not_valid :: ConstraintMissingNotValid as pgt_analyse :: Rule > :: Options ; +pub type CreatingEnum = ::Options; pub type DisallowUniqueConstraint = < lint :: safety :: disallow_unique_constraint :: DisallowUniqueConstraint as pgt_analyse :: Rule > :: Options ; pub type MultipleAlterTable = ::Options; diff --git a/crates/pgt_analyser/tests/specs/safety/creatingEnum/basic.sql b/crates/pgt_analyser/tests/specs/safety/creatingEnum/basic.sql new file mode 100644 index 000000000..819a3ec47 --- /dev/null +++ b/crates/pgt_analyser/tests/specs/safety/creatingEnum/basic.sql @@ -0,0 +1,2 @@ +-- expect_only_lint/safety/creatingEnum +CREATE TYPE document_type AS ENUM ('invoice', 'receipt', 'other'); \ No newline at end of file diff --git a/crates/pgt_analyser/tests/specs/safety/creatingEnum/basic.sql.snap b/crates/pgt_analyser/tests/specs/safety/creatingEnum/basic.sql.snap new file mode 100644 index 000000000..8dd89c013 --- /dev/null +++ b/crates/pgt_analyser/tests/specs/safety/creatingEnum/basic.sql.snap @@ -0,0 +1,19 @@ +--- +source: crates/pgt_analyser/tests/rules_tests.rs +expression: snapshot +snapshot_kind: text +--- +# Input +``` +-- expect_only_lint/safety/creatingEnum +CREATE TYPE document_type AS ENUM ('invoice', 'receipt', 'other'); +``` + +# Diagnostics +lint/safety/creatingEnum ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + × Creating enum type document_type is not recommended. + + i Enum types are difficult to modify: removing values requires complex migrations, and associating additional data with values is not possible. + + i Consider using a lookup table with a foreign key constraint instead, which provides more flexibility and easier maintenance. diff --git a/crates/pgt_analyser/tests/specs/safety/creatingEnum/simple_enum.sql b/crates/pgt_analyser/tests/specs/safety/creatingEnum/simple_enum.sql new file mode 100644 index 000000000..a1ba1b884 --- /dev/null +++ b/crates/pgt_analyser/tests/specs/safety/creatingEnum/simple_enum.sql @@ -0,0 +1,3 @@ +-- expect_only_lint/safety/creatingEnum +-- Simple enum with two values +CREATE TYPE status AS ENUM ('active', 'inactive'); diff --git a/crates/pgt_analyser/tests/specs/safety/creatingEnum/simple_enum.sql.snap b/crates/pgt_analyser/tests/specs/safety/creatingEnum/simple_enum.sql.snap new file mode 100644 index 000000000..ac8437030 --- /dev/null +++ b/crates/pgt_analyser/tests/specs/safety/creatingEnum/simple_enum.sql.snap @@ -0,0 +1,21 @@ +--- +source: crates/pgt_analyser/tests/rules_tests.rs +expression: snapshot +snapshot_kind: text +--- +# Input +``` +-- expect_only_lint/safety/creatingEnum +-- Simple enum with two values +CREATE TYPE status AS ENUM ('active', 'inactive'); + +``` + +# Diagnostics +lint/safety/creatingEnum ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + × Creating enum type status is not recommended. + + i Enum types are difficult to modify: removing values requires complex migrations, and associating additional data with values is not possible. + + i Consider using a lookup table with a foreign key constraint instead, which provides more flexibility and easier maintenance. diff --git a/crates/pgt_analyser/tests/specs/safety/creatingEnum/valid_create_table.sql b/crates/pgt_analyser/tests/specs/safety/creatingEnum/valid_create_table.sql new file mode 100644 index 000000000..bfda6fc32 --- /dev/null +++ b/crates/pgt_analyser/tests/specs/safety/creatingEnum/valid_create_table.sql @@ -0,0 +1,6 @@ +-- Valid: Creating a regular table (not an enum) +-- expect_no_diagnostics +CREATE TABLE users ( + id INT PRIMARY KEY, + name TEXT NOT NULL +); diff --git a/crates/pgt_analyser/tests/specs/safety/creatingEnum/valid_create_table.sql.snap b/crates/pgt_analyser/tests/specs/safety/creatingEnum/valid_create_table.sql.snap new file mode 100644 index 000000000..9f5d30d89 --- /dev/null +++ b/crates/pgt_analyser/tests/specs/safety/creatingEnum/valid_create_table.sql.snap @@ -0,0 +1,15 @@ +--- +source: crates/pgt_analyser/tests/rules_tests.rs +expression: snapshot +snapshot_kind: text +--- +# Input +``` +-- Valid: Creating a regular table (not an enum) +-- expect_no_diagnostics +CREATE TABLE users ( + id INT PRIMARY KEY, + name TEXT NOT NULL +); + +``` diff --git a/crates/pgt_analyser/tests/specs/safety/creatingEnum/valid_lookup_table.sql b/crates/pgt_analyser/tests/specs/safety/creatingEnum/valid_lookup_table.sql new file mode 100644 index 000000000..44b0e51ba --- /dev/null +++ b/crates/pgt_analyser/tests/specs/safety/creatingEnum/valid_lookup_table.sql @@ -0,0 +1,12 @@ +-- Valid: Using a lookup table instead of enum +-- expect_no_diagnostics +CREATE TABLE document_type ( + type_name TEXT PRIMARY KEY +); + +INSERT INTO document_type VALUES ('invoice'), ('receipt'), ('other'); + +CREATE TABLE document ( + id INT GENERATED ALWAYS AS IDENTITY PRIMARY KEY, + type TEXT REFERENCES document_type(type_name) +); diff --git a/crates/pgt_analyser/tests/specs/safety/creatingEnum/valid_lookup_table.sql.snap b/crates/pgt_analyser/tests/specs/safety/creatingEnum/valid_lookup_table.sql.snap new file mode 100644 index 000000000..038fb90f0 --- /dev/null +++ b/crates/pgt_analyser/tests/specs/safety/creatingEnum/valid_lookup_table.sql.snap @@ -0,0 +1,21 @@ +--- +source: crates/pgt_analyser/tests/rules_tests.rs +expression: snapshot +snapshot_kind: text +--- +# Input +``` +-- Valid: Using a lookup table instead of enum +-- expect_no_diagnostics +CREATE TABLE document_type ( + type_name TEXT PRIMARY KEY +); + +INSERT INTO document_type VALUES ('invoice'), ('receipt'), ('other'); + +CREATE TABLE document ( + id INT GENERATED ALWAYS AS IDENTITY PRIMARY KEY, + type TEXT REFERENCES document_type(type_name) +); + +``` diff --git a/crates/pgt_analyser/tests/specs/safety/creatingEnum/with_schema.sql b/crates/pgt_analyser/tests/specs/safety/creatingEnum/with_schema.sql new file mode 100644 index 000000000..098b3e5b0 --- /dev/null +++ b/crates/pgt_analyser/tests/specs/safety/creatingEnum/with_schema.sql @@ -0,0 +1,3 @@ +-- expect_only_lint/safety/creatingEnum +-- Enum type with schema qualification +CREATE TYPE myschema.status AS ENUM ('active', 'inactive', 'pending'); diff --git a/crates/pgt_analyser/tests/specs/safety/creatingEnum/with_schema.sql.snap b/crates/pgt_analyser/tests/specs/safety/creatingEnum/with_schema.sql.snap new file mode 100644 index 000000000..33f5ae68f --- /dev/null +++ b/crates/pgt_analyser/tests/specs/safety/creatingEnum/with_schema.sql.snap @@ -0,0 +1,21 @@ +--- +source: crates/pgt_analyser/tests/rules_tests.rs +expression: snapshot +snapshot_kind: text +--- +# Input +``` +-- expect_only_lint/safety/creatingEnum +-- Enum type with schema qualification +CREATE TYPE myschema.status AS ENUM ('active', 'inactive', 'pending'); + +``` + +# Diagnostics +lint/safety/creatingEnum ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + × Creating enum type myschema.status is not recommended. + + i Enum types are difficult to modify: removing values requires complex migrations, and associating additional data with values is not possible. + + i Consider using a lookup table with a foreign key constraint instead, which provides more flexibility and easier maintenance. diff --git a/crates/pgt_configuration/src/analyser/linter/rules.rs b/crates/pgt_configuration/src/analyser/linter/rules.rs index a37e08ab4..92268deb3 100644 --- a/crates/pgt_configuration/src/analyser/linter/rules.rs +++ b/crates/pgt_configuration/src/analyser/linter/rules.rs @@ -192,6 +192,9 @@ pub struct Safety { #[serde(skip_serializing_if = "Option::is_none")] pub constraint_missing_not_valid: Option>, + #[doc = "Creating enum types is not recommended for new applications."] + #[serde(skip_serializing_if = "Option::is_none")] + pub creating_enum: Option>, #[doc = "Disallow adding a UNIQUE constraint without using an existing index."] #[serde(skip_serializing_if = "Option::is_none")] pub disallow_unique_constraint: @@ -261,6 +264,7 @@ impl Safety { "banTruncateCascade", "changingColumnType", "constraintMissingNotValid", + "creatingEnum", "disallowUniqueConstraint", "multipleAlterTable", "preferBigInt", @@ -287,7 +291,7 @@ impl Safety { RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[8]), RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[10]), RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[11]), - RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[16]), + RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[17]), ]; const ALL_RULES_AS_FILTERS: &'static [RuleFilter<'static>] = &[ RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[0]), @@ -320,6 +324,7 @@ impl Safety { RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[27]), RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[28]), RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[29]), + RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[30]), ]; #[doc = r" Retrieves the recommended rules"] pub(crate) fn is_recommended_true(&self) -> bool { @@ -411,81 +416,86 @@ impl Safety { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[14])); } } - if let Some(rule) = self.disallow_unique_constraint.as_ref() { + if let Some(rule) = self.creating_enum.as_ref() { if rule.is_enabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[15])); } } - if let Some(rule) = self.multiple_alter_table.as_ref() { + if let Some(rule) = self.disallow_unique_constraint.as_ref() { if rule.is_enabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[16])); } } - if let Some(rule) = self.prefer_big_int.as_ref() { + if let Some(rule) = self.multiple_alter_table.as_ref() { if rule.is_enabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[17])); } } - if let Some(rule) = self.prefer_bigint_over_int.as_ref() { + if let Some(rule) = self.prefer_big_int.as_ref() { if rule.is_enabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[18])); } } - if let Some(rule) = self.prefer_bigint_over_smallint.as_ref() { + if let Some(rule) = self.prefer_bigint_over_int.as_ref() { if rule.is_enabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[19])); } } - if let Some(rule) = self.prefer_identity.as_ref() { + if let Some(rule) = self.prefer_bigint_over_smallint.as_ref() { if rule.is_enabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[20])); } } - if let Some(rule) = self.prefer_jsonb.as_ref() { + if let Some(rule) = self.prefer_identity.as_ref() { if rule.is_enabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[21])); } } - if let Some(rule) = self.prefer_robust_stmts.as_ref() { + if let Some(rule) = self.prefer_jsonb.as_ref() { if rule.is_enabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[22])); } } - if let Some(rule) = self.prefer_text_field.as_ref() { + if let Some(rule) = self.prefer_robust_stmts.as_ref() { if rule.is_enabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[23])); } } - if let Some(rule) = self.prefer_timestamptz.as_ref() { + if let Some(rule) = self.prefer_text_field.as_ref() { if rule.is_enabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[24])); } } - if let Some(rule) = self.renaming_column.as_ref() { + if let Some(rule) = self.prefer_timestamptz.as_ref() { if rule.is_enabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[25])); } } - if let Some(rule) = self.renaming_table.as_ref() { + if let Some(rule) = self.renaming_column.as_ref() { if rule.is_enabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[26])); } } - if let Some(rule) = self.require_concurrent_index_creation.as_ref() { + if let Some(rule) = self.renaming_table.as_ref() { if rule.is_enabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[27])); } } - if let Some(rule) = self.require_concurrent_index_deletion.as_ref() { + if let Some(rule) = self.require_concurrent_index_creation.as_ref() { if rule.is_enabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[28])); } } - if let Some(rule) = self.transaction_nesting.as_ref() { + if let Some(rule) = self.require_concurrent_index_deletion.as_ref() { if rule.is_enabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[29])); } } + if let Some(rule) = self.transaction_nesting.as_ref() { + if rule.is_enabled() { + index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[30])); + } + } index_set } pub(crate) fn get_disabled_rules(&self) -> FxHashSet> { @@ -565,81 +575,86 @@ impl Safety { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[14])); } } - if let Some(rule) = self.disallow_unique_constraint.as_ref() { + if let Some(rule) = self.creating_enum.as_ref() { if rule.is_disabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[15])); } } - if let Some(rule) = self.multiple_alter_table.as_ref() { + if let Some(rule) = self.disallow_unique_constraint.as_ref() { if rule.is_disabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[16])); } } - if let Some(rule) = self.prefer_big_int.as_ref() { + if let Some(rule) = self.multiple_alter_table.as_ref() { if rule.is_disabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[17])); } } - if let Some(rule) = self.prefer_bigint_over_int.as_ref() { + if let Some(rule) = self.prefer_big_int.as_ref() { if rule.is_disabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[18])); } } - if let Some(rule) = self.prefer_bigint_over_smallint.as_ref() { + if let Some(rule) = self.prefer_bigint_over_int.as_ref() { if rule.is_disabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[19])); } } - if let Some(rule) = self.prefer_identity.as_ref() { + if let Some(rule) = self.prefer_bigint_over_smallint.as_ref() { if rule.is_disabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[20])); } } - if let Some(rule) = self.prefer_jsonb.as_ref() { + if let Some(rule) = self.prefer_identity.as_ref() { if rule.is_disabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[21])); } } - if let Some(rule) = self.prefer_robust_stmts.as_ref() { + if let Some(rule) = self.prefer_jsonb.as_ref() { if rule.is_disabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[22])); } } - if let Some(rule) = self.prefer_text_field.as_ref() { + if let Some(rule) = self.prefer_robust_stmts.as_ref() { if rule.is_disabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[23])); } } - if let Some(rule) = self.prefer_timestamptz.as_ref() { + if let Some(rule) = self.prefer_text_field.as_ref() { if rule.is_disabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[24])); } } - if let Some(rule) = self.renaming_column.as_ref() { + if let Some(rule) = self.prefer_timestamptz.as_ref() { if rule.is_disabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[25])); } } - if let Some(rule) = self.renaming_table.as_ref() { + if let Some(rule) = self.renaming_column.as_ref() { if rule.is_disabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[26])); } } - if let Some(rule) = self.require_concurrent_index_creation.as_ref() { + if let Some(rule) = self.renaming_table.as_ref() { if rule.is_disabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[27])); } } - if let Some(rule) = self.require_concurrent_index_deletion.as_ref() { + if let Some(rule) = self.require_concurrent_index_creation.as_ref() { if rule.is_disabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[28])); } } - if let Some(rule) = self.transaction_nesting.as_ref() { + if let Some(rule) = self.require_concurrent_index_deletion.as_ref() { if rule.is_disabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[29])); } } + if let Some(rule) = self.transaction_nesting.as_ref() { + if rule.is_disabled() { + index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[30])); + } + } index_set } #[doc = r" Checks if, given a rule name, matches one of the rules contained in this category"] @@ -684,6 +699,7 @@ impl Safety { "banTruncateCascade" => Severity::Error, "changingColumnType" => Severity::Warning, "constraintMissingNotValid" => Severity::Warning, + "creatingEnum" => Severity::Warning, "disallowUniqueConstraint" => Severity::Error, "multipleAlterTable" => Severity::Warning, "preferBigInt" => Severity::Warning, @@ -767,6 +783,10 @@ impl Safety { .constraint_missing_not_valid .as_ref() .map(|conf| (conf.level(), conf.get_options())), + "creatingEnum" => self + .creating_enum + .as_ref() + .map(|conf| (conf.level(), conf.get_options())), "disallowUniqueConstraint" => self .disallow_unique_constraint .as_ref() diff --git a/crates/pgt_diagnostics_categories/src/categories.rs b/crates/pgt_diagnostics_categories/src/categories.rs index 1a507f1b7..08a4f4cd2 100644 --- a/crates/pgt_diagnostics_categories/src/categories.rs +++ b/crates/pgt_diagnostics_categories/src/categories.rs @@ -28,6 +28,7 @@ define_categories! { "lint/safety/banTruncateCascade": "https://pgtools.dev/latest/rules/ban-truncate-cascade", "lint/safety/changingColumnType": "https://pgtools.dev/latest/rules/changing-column-type", "lint/safety/constraintMissingNotValid": "https://pgtools.dev/latest/rules/constraint-missing-not-valid", + "lint/safety/creatingEnum": "https://pgtools.dev/latest/rules/creating-enum", "lint/safety/disallowUniqueConstraint": "https://pgtools.dev/latest/rules/disallow-unique-constraint", "lint/safety/multipleAlterTable": "https://pgtools.dev/latest/rules/multiple-alter-table", "lint/safety/preferBigInt": "https://pgtools.dev/latest/rules/prefer-big-int", diff --git a/docs/reference/rule_sources.md b/docs/reference/rule_sources.md index ec056b7af..8d4d2872b 100644 --- a/docs/reference/rule_sources.md +++ b/docs/reference/rule_sources.md @@ -9,6 +9,7 @@ _No exclusive rules available._ | [E11](https://kaveland.no/eugene/hints/E11/index.html) |[addSerialColumn](../rules/add-serial-column) | | [E3](https://kaveland.no/eugene/hints/E3/index.html) |[preferJsonb](../rules/prefer-jsonb) | | [W12](https://kaveland.no/eugene/hints/W12/index.html) |[multipleAlterTable](../rules/multiple-alter-table) | +| [W13](https://kaveland.no/eugene/hints/W13/index.html) |[creatingEnum](../rules/creating-enum) | ### Squawk | Squawk Rule Name | Rule Name | | ---- | ---- | diff --git a/docs/reference/rules.md b/docs/reference/rules.md index 2e988b932..35f7bd7c6 100644 --- a/docs/reference/rules.md +++ b/docs/reference/rules.md @@ -27,6 +27,7 @@ Rules that detect potential safety issues in your code. | [banTruncateCascade](./ban-truncate-cascade) | Using `TRUNCATE`'s `CASCADE` option will truncate any tables that are also foreign-keyed to the specified tables. | | | [changingColumnType](./changing-column-type) | Changing a column type may break existing clients. | | | [constraintMissingNotValid](./constraint-missing-not-valid) | Adding constraints without NOT VALID blocks all reads and writes. | | +| [creatingEnum](./creating-enum) | Creating enum types is not recommended for new applications. | | | [disallowUniqueConstraint](./disallow-unique-constraint) | Disallow adding a UNIQUE constraint without using an existing index. | | | [multipleAlterTable](./multiple-alter-table) | Multiple ALTER TABLE statements on the same table should be combined into a single statement. | ✅ | | [preferBigInt](./prefer-big-int) | Prefer BIGINT over smaller integer types. | | diff --git a/docs/reference/rules/creating-enum.md b/docs/reference/rules/creating-enum.md new file mode 100644 index 000000000..54c45494e --- /dev/null +++ b/docs/reference/rules/creating-enum.md @@ -0,0 +1,69 @@ +# creatingEnum +**Diagnostic Category: `lint/safety/creatingEnum`** + +**Since**: `vnext` + + +**Sources**: +- Inspired from: eugene/W13 + +## Description +Creating enum types is not recommended for new applications. + +Enumerated types have several limitations that make them difficult to work with in production: + +- Removing values from an enum requires complex migrations and is not supported directly +- Adding values to an enum requires an ACCESS EXCLUSIVE lock in some PostgreSQL versions +- Associating additional data with enum values is impossible without restructuring +- Renaming enum values requires careful migration planning + +A lookup table with a foreign key constraint provides more flexibility and is easier to maintain. + +## Examples + +### Invalid + +```sql +CREATE TYPE document_type AS ENUM ('invoice', 'receipt', 'other'); +``` + +```sh +code-block.sql:1:1 lint/safety/creatingEnum ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! Creating enum type document_type is not recommended. + + > 1 │ CREATE TYPE document_type AS ENUM ('invoice', 'receipt', 'other'); + │ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + 2 │ + + i Enum types are difficult to modify: removing values requires complex migrations, and associating additional data with values is not possible. + + i Consider using a lookup table with a foreign key constraint instead, which provides more flexibility and easier maintenance. + + +``` + +### Valid + +```sql +-- Use a lookup table instead +CREATE TABLE document_type ( + type_name TEXT PRIMARY KEY +); +INSERT INTO document_type VALUES ('invoice'), ('receipt'), ('other'); +``` + +## How to configure +```json + +{ + "linter": { + "rules": { + "safety": { + "creatingEnum": "error" + } + } + } +} + +``` diff --git a/docs/schema.json b/docs/schema.json index 68c448365..90dddbddf 100644 --- a/docs/schema.json +++ b/docs/schema.json @@ -528,6 +528,17 @@ } ] }, + "creatingEnum": { + "description": "Creating enum types is not recommended for new applications.", + "anyOf": [ + { + "$ref": "#/definitions/RuleConfiguration" + }, + { + "type": "null" + } + ] + }, "disallowUniqueConstraint": { "description": "Disallow adding a UNIQUE constraint without using an existing index.", "anyOf": [ diff --git a/packages/@postgrestools/backend-jsonrpc/src/workspace.ts b/packages/@postgrestools/backend-jsonrpc/src/workspace.ts index b170e6838..dc30b74b0 100644 --- a/packages/@postgrestools/backend-jsonrpc/src/workspace.ts +++ b/packages/@postgrestools/backend-jsonrpc/src/workspace.ts @@ -78,6 +78,7 @@ export type Category = | "lint/safety/banTruncateCascade" | "lint/safety/changingColumnType" | "lint/safety/constraintMissingNotValid" + | "lint/safety/creatingEnum" | "lint/safety/disallowUniqueConstraint" | "lint/safety/multipleAlterTable" | "lint/safety/preferBigInt" @@ -501,6 +502,10 @@ export interface Safety { * Adding constraints without NOT VALID blocks all reads and writes. */ constraintMissingNotValid?: RuleConfiguration_for_Null; + /** + * Creating enum types is not recommended for new applications. + */ + creatingEnum?: RuleConfiguration_for_Null; /** * Disallow adding a UNIQUE constraint without using an existing index. */ From 5939553c5ed235e3b3437692276edd2e6758269f Mon Sep 17 00:00:00 2001 From: psteinroe Date: Sun, 19 Oct 2025 13:06:55 +0200 Subject: [PATCH 5/8] feat(analyser): lock timout warning --- .../pgt_analyse/src/analysed_file_context.rs | 101 +++++++++++++- crates/pgt_analyser/src/lint/safety.rs | 3 +- .../src/lint/safety/lock_timeout_warning.rs | 124 ++++++++++++++++++ crates/pgt_analyser/src/options.rs | 2 + .../specs/safety/lockTimeoutWarning/basic.sql | 3 + .../safety/lockTimeoutWarning/basic.sql.snap | 20 +++ .../create_index_without_timeout.sql | 3 + .../create_index_without_timeout.sql.snap | 21 +++ .../multiple_statements_without_timeout.sql | 6 + ...ltiple_statements_without_timeout.sql.snap | 34 +++++ .../timeout_set_variant.sql | 4 + .../timeout_set_variant.sql.snap | 13 ++ .../valid_concurrent_index.sql | 3 + .../valid_concurrent_index.sql.snap | 12 ++ .../valid_index_on_new_table.sql | 8 ++ .../valid_index_on_new_table.sql.snap | 17 +++ .../lockTimeoutWarning/valid_new_table.sql | 8 ++ .../valid_new_table.sql.snap | 17 +++ .../lockTimeoutWarning/valid_with_timeout.sql | 4 + .../valid_with_timeout.sql.snap | 13 ++ .../src/analyser/linter/rules.rs | 78 +++++++---- .../src/categories.rs | 1 + docs/reference/rule_sources.md | 1 + docs/reference/rules.md | 1 + docs/reference/rules/lock-timeout-warning.md | 87 ++++++++++++ docs/schema.json | 11 ++ .../backend-jsonrpc/src/workspace.ts | 5 + 27 files changed, 568 insertions(+), 32 deletions(-) create mode 100644 crates/pgt_analyser/src/lint/safety/lock_timeout_warning.rs create mode 100644 crates/pgt_analyser/tests/specs/safety/lockTimeoutWarning/basic.sql create mode 100644 crates/pgt_analyser/tests/specs/safety/lockTimeoutWarning/basic.sql.snap create mode 100644 crates/pgt_analyser/tests/specs/safety/lockTimeoutWarning/create_index_without_timeout.sql create mode 100644 crates/pgt_analyser/tests/specs/safety/lockTimeoutWarning/create_index_without_timeout.sql.snap create mode 100644 crates/pgt_analyser/tests/specs/safety/lockTimeoutWarning/multiple_statements_without_timeout.sql create mode 100644 crates/pgt_analyser/tests/specs/safety/lockTimeoutWarning/multiple_statements_without_timeout.sql.snap create mode 100644 crates/pgt_analyser/tests/specs/safety/lockTimeoutWarning/timeout_set_variant.sql create mode 100644 crates/pgt_analyser/tests/specs/safety/lockTimeoutWarning/timeout_set_variant.sql.snap create mode 100644 crates/pgt_analyser/tests/specs/safety/lockTimeoutWarning/valid_concurrent_index.sql create mode 100644 crates/pgt_analyser/tests/specs/safety/lockTimeoutWarning/valid_concurrent_index.sql.snap create mode 100644 crates/pgt_analyser/tests/specs/safety/lockTimeoutWarning/valid_index_on_new_table.sql create mode 100644 crates/pgt_analyser/tests/specs/safety/lockTimeoutWarning/valid_index_on_new_table.sql.snap create mode 100644 crates/pgt_analyser/tests/specs/safety/lockTimeoutWarning/valid_new_table.sql create mode 100644 crates/pgt_analyser/tests/specs/safety/lockTimeoutWarning/valid_new_table.sql.snap create mode 100644 crates/pgt_analyser/tests/specs/safety/lockTimeoutWarning/valid_with_timeout.sql create mode 100644 crates/pgt_analyser/tests/specs/safety/lockTimeoutWarning/valid_with_timeout.sql.snap create mode 100644 docs/reference/rules/lock-timeout-warning.md diff --git a/crates/pgt_analyse/src/analysed_file_context.rs b/crates/pgt_analyse/src/analysed_file_context.rs index 8bf21d99c..a04ccb36e 100644 --- a/crates/pgt_analyse/src/analysed_file_context.rs +++ b/crates/pgt_analyse/src/analysed_file_context.rs @@ -1,12 +1,16 @@ pub struct AnalysedFileContext<'a> { pub stmts: &'a Vec, - pos: usize, + transaction_state: TransactionState, } impl<'a> AnalysedFileContext<'a> { pub fn new(stmts: &'a Vec) -> Self { - Self { stmts, pos: 0 } + Self { + stmts, + pos: 0, + transaction_state: TransactionState::default(), + } } pub fn previous_stmts(&self) -> &[pgt_query::NodeEnum] { @@ -17,7 +21,100 @@ impl<'a> AnalysedFileContext<'a> { self.stmts.len() } + /// Move to the next statement and update transaction state with the current statement pub fn next(&mut self) { + if self.pos < self.stmts.len() { + self.transaction_state + .update_from_stmt(&self.stmts[self.pos]); + } self.pos += 1; } + + /// Get a reference to the transaction state + pub fn transaction_state(&self) -> &TransactionState { + &self.transaction_state + } +} + +/// Represents the state of a transaction as we analyze statements in a file. +/// +/// This tracks properties that span multiple statements, such as: +/// - Whether a lock timeout has been set +/// - Which objects have been created in this transaction +#[derive(Debug, Default)] +pub struct TransactionState { + /// Whether `SET lock_timeout` has been called in this transaction + lock_timeout_set: bool, + /// Objects (schema, name) created in this transaction + /// Schema names are normalized: empty string is stored as "public" + created_objects: Vec<(String, String)>, +} + +impl TransactionState { + /// Returns true if a lock timeout has been set in this transaction + pub fn has_lock_timeout(&self) -> bool { + self.lock_timeout_set + } + + /// Returns true if an object with the given schema and name was created in this transaction + pub fn has_created_object(&self, schema: &str, name: &str) -> bool { + // Normalize schema: treat empty string as "public" + let normalized_schema = if schema.is_empty() { "public" } else { schema }; + + self.created_objects + .iter() + .any(|(s, n)| normalized_schema.eq_ignore_ascii_case(s) && name.eq_ignore_ascii_case(n)) + } + + /// Record that an object was created, normalizing the schema name + fn add_created_object(&mut self, schema: String, name: String) { + // Normalize schema: store "public" instead of empty string + let normalized_schema = if schema.is_empty() { + "public".to_string() + } else { + schema + }; + self.created_objects.push((normalized_schema, name)); + } + + /// Update transaction state based on a statement + pub(crate) fn update_from_stmt(&mut self, stmt: &pgt_query::NodeEnum) { + // Track SET lock_timeout + if let pgt_query::NodeEnum::VariableSetStmt(set_stmt) = stmt { + if set_stmt.name.eq_ignore_ascii_case("lock_timeout") { + self.lock_timeout_set = true; + } + } + + // Track created objects + match stmt { + pgt_query::NodeEnum::CreateStmt(create_stmt) => { + if let Some(relation) = &create_stmt.relation { + let schema = relation.schemaname.clone(); + let name = relation.relname.clone(); + self.add_created_object(schema, name); + } + } + pgt_query::NodeEnum::IndexStmt(index_stmt) => { + if !index_stmt.idxname.is_empty() { + let schema = index_stmt + .relation + .as_ref() + .map(|r| r.schemaname.clone()) + .unwrap_or_default(); + self.add_created_object(schema, index_stmt.idxname.clone()); + } + } + pgt_query::NodeEnum::CreateTableAsStmt(ctas) => { + if let Some(into) = &ctas.into { + if let Some(rel) = &into.rel { + let schema = rel.schemaname.clone(); + let name = rel.relname.clone(); + self.add_created_object(schema, name); + } + } + } + _ => {} + } + } } diff --git a/crates/pgt_analyser/src/lint/safety.rs b/crates/pgt_analyser/src/lint/safety.rs index c20461b19..cb9063cc1 100644 --- a/crates/pgt_analyser/src/lint/safety.rs +++ b/crates/pgt_analyser/src/lint/safety.rs @@ -18,6 +18,7 @@ pub mod changing_column_type; pub mod constraint_missing_not_valid; pub mod creating_enum; pub mod disallow_unique_constraint; +pub mod lock_timeout_warning; pub mod multiple_alter_table; pub mod prefer_big_int; pub mod prefer_bigint_over_int; @@ -32,4 +33,4 @@ pub mod renaming_table; pub mod require_concurrent_index_creation; pub mod require_concurrent_index_deletion; pub mod transaction_nesting; -declare_lint_group! { pub Safety { name : "safety" , rules : [self :: add_serial_column :: AddSerialColumn , self :: adding_field_with_default :: AddingFieldWithDefault , self :: adding_foreign_key_constraint :: AddingForeignKeyConstraint , self :: adding_not_null_field :: AddingNotNullField , self :: adding_primary_key_constraint :: AddingPrimaryKeyConstraint , self :: adding_required_field :: AddingRequiredField , self :: ban_char_field :: BanCharField , self :: ban_concurrent_index_creation_in_transaction :: BanConcurrentIndexCreationInTransaction , self :: ban_drop_column :: BanDropColumn , self :: ban_drop_database :: BanDropDatabase , self :: ban_drop_not_null :: BanDropNotNull , self :: ban_drop_table :: BanDropTable , self :: ban_truncate_cascade :: BanTruncateCascade , self :: changing_column_type :: ChangingColumnType , self :: constraint_missing_not_valid :: ConstraintMissingNotValid , self :: creating_enum :: CreatingEnum , self :: disallow_unique_constraint :: DisallowUniqueConstraint , self :: multiple_alter_table :: MultipleAlterTable , self :: prefer_big_int :: PreferBigInt , self :: prefer_bigint_over_int :: PreferBigintOverInt , self :: prefer_bigint_over_smallint :: PreferBigintOverSmallint , self :: prefer_identity :: PreferIdentity , self :: prefer_jsonb :: PreferJsonb , self :: prefer_robust_stmts :: PreferRobustStmts , self :: prefer_text_field :: PreferTextField , self :: prefer_timestamptz :: PreferTimestamptz , self :: renaming_column :: RenamingColumn , self :: renaming_table :: RenamingTable , self :: require_concurrent_index_creation :: RequireConcurrentIndexCreation , self :: require_concurrent_index_deletion :: RequireConcurrentIndexDeletion , self :: transaction_nesting :: TransactionNesting ,] } } +declare_lint_group! { pub Safety { name : "safety" , rules : [self :: add_serial_column :: AddSerialColumn , self :: adding_field_with_default :: AddingFieldWithDefault , self :: adding_foreign_key_constraint :: AddingForeignKeyConstraint , self :: adding_not_null_field :: AddingNotNullField , self :: adding_primary_key_constraint :: AddingPrimaryKeyConstraint , self :: adding_required_field :: AddingRequiredField , self :: ban_char_field :: BanCharField , self :: ban_concurrent_index_creation_in_transaction :: BanConcurrentIndexCreationInTransaction , self :: ban_drop_column :: BanDropColumn , self :: ban_drop_database :: BanDropDatabase , self :: ban_drop_not_null :: BanDropNotNull , self :: ban_drop_table :: BanDropTable , self :: ban_truncate_cascade :: BanTruncateCascade , self :: changing_column_type :: ChangingColumnType , self :: constraint_missing_not_valid :: ConstraintMissingNotValid , self :: creating_enum :: CreatingEnum , self :: disallow_unique_constraint :: DisallowUniqueConstraint , self :: lock_timeout_warning :: LockTimeoutWarning , self :: multiple_alter_table :: MultipleAlterTable , self :: prefer_big_int :: PreferBigInt , self :: prefer_bigint_over_int :: PreferBigintOverInt , self :: prefer_bigint_over_smallint :: PreferBigintOverSmallint , self :: prefer_identity :: PreferIdentity , self :: prefer_jsonb :: PreferJsonb , self :: prefer_robust_stmts :: PreferRobustStmts , self :: prefer_text_field :: PreferTextField , self :: prefer_timestamptz :: PreferTimestamptz , self :: renaming_column :: RenamingColumn , self :: renaming_table :: RenamingTable , self :: require_concurrent_index_creation :: RequireConcurrentIndexCreation , self :: require_concurrent_index_deletion :: RequireConcurrentIndexDeletion , self :: transaction_nesting :: TransactionNesting ,] } } diff --git a/crates/pgt_analyser/src/lint/safety/lock_timeout_warning.rs b/crates/pgt_analyser/src/lint/safety/lock_timeout_warning.rs new file mode 100644 index 000000000..6fe982cba --- /dev/null +++ b/crates/pgt_analyser/src/lint/safety/lock_timeout_warning.rs @@ -0,0 +1,124 @@ +use pgt_analyse::{Rule, RuleDiagnostic, RuleSource, context::RuleContext, declare_lint_rule}; +use pgt_console::markup; +use pgt_diagnostics::Severity; + +declare_lint_rule! { + /// Taking a dangerous lock without setting a lock timeout can cause indefinite blocking. + /// + /// When a statement acquires a lock that would block common operations (like SELECT, INSERT, UPDATE, DELETE), + /// it can cause the database to become unresponsive if another transaction is holding a conflicting lock + /// while idle in transaction or active. This is particularly dangerous for: + /// + /// - ALTER TABLE statements (acquire ACCESS EXCLUSIVE lock) + /// - CREATE INDEX without CONCURRENTLY (acquires SHARE lock) + /// + /// Setting a lock timeout ensures that if the lock cannot be acquired within a reasonable time, + /// the statement will fail rather than blocking indefinitely. + /// + /// ## Examples + /// + /// ### Invalid + /// + /// ```sql,expect_diagnostic + /// ALTER TABLE users ADD COLUMN email TEXT; + /// ``` + /// + /// ```sql,expect_diagnostic + /// CREATE INDEX users_email_idx ON users(email); + /// ``` + /// + /// ### Valid + /// + /// ```sql + /// -- Use CONCURRENTLY to avoid taking dangerous locks + /// CREATE INDEX CONCURRENTLY users_email_idx ON users(email); + /// ``` + /// + pub LockTimeoutWarning { + version: "next", + name: "lockTimeoutWarning", + severity: Severity::Error, + recommended: false, + sources: &[RuleSource::Eugene("E9")], + } +} + +impl Rule for LockTimeoutWarning { + type Options = (); + + fn run(ctx: &RuleContext) -> Vec { + let mut diagnostics = Vec::new(); + + // Check if lock timeout has been set in the transaction + let tx_state = ctx.file_context().transaction_state(); + if tx_state.has_lock_timeout() { + return diagnostics; + } + + // Check if this statement takes a dangerous lock on an existing object + match ctx.stmt() { + // ALTER TABLE takes ACCESS EXCLUSIVE lock + pgt_query::NodeEnum::AlterTableStmt(stmt) => { + if let Some(relation) = &stmt.relation { + let schema = if relation.schemaname.is_empty() { + "public" + } else { + &relation.schemaname + }; + let table = &relation.relname; + + // Only warn if the table wasn't created in this transaction + if !tx_state.has_created_object(schema, table) { + let full_name = format!("{}.{}", schema, table); + diagnostics.push( + RuleDiagnostic::new( + rule_category!(), + None, + markup! { + "Statement takes ACCESS EXCLUSIVE lock on "{full_name}" without lock timeout set." + }, + ) + .detail(None, "This can block all operations on the table indefinitely if another transaction holds a conflicting lock.") + .note("Run 'SET LOCAL lock_timeout = '2s';' before this statement and retry the migration if it times out."), + ); + } + } + } + + // CREATE INDEX without CONCURRENTLY takes SHARE lock + pgt_query::NodeEnum::IndexStmt(stmt) => { + if !stmt.concurrent { + if let Some(relation) = &stmt.relation { + let schema = if relation.schemaname.is_empty() { + "public" + } else { + &relation.schemaname + }; + let table = &relation.relname; + + // Only warn if the table wasn't created in this transaction + if !tx_state.has_created_object(schema, table) { + let full_name = format!("{}.{}", schema, table); + let index_name = &stmt.idxname; + diagnostics.push( + RuleDiagnostic::new( + rule_category!(), + None, + markup! { + "Statement takes SHARE lock on "{full_name}" while creating index "{index_name}" without lock timeout set." + }, + ) + .detail(None, "This blocks writes to the table indefinitely if another transaction holds a conflicting lock.") + .note("Run 'SET LOCAL lock_timeout = '2s';' before this statement, or use CREATE INDEX CONCURRENTLY to avoid blocking writes."), + ); + } + } + } + } + + _ => {} + } + + diagnostics + } +} diff --git a/crates/pgt_analyser/src/options.rs b/crates/pgt_analyser/src/options.rs index 7de988bdd..340beabb2 100644 --- a/crates/pgt_analyser/src/options.rs +++ b/crates/pgt_analyser/src/options.rs @@ -27,6 +27,8 @@ pub type ChangingColumnType = pub type ConstraintMissingNotValid = < lint :: safety :: constraint_missing_not_valid :: ConstraintMissingNotValid as pgt_analyse :: Rule > :: Options ; pub type CreatingEnum = ::Options; pub type DisallowUniqueConstraint = < lint :: safety :: disallow_unique_constraint :: DisallowUniqueConstraint as pgt_analyse :: Rule > :: Options ; +pub type LockTimeoutWarning = + ::Options; pub type MultipleAlterTable = ::Options; pub type PreferBigInt = ::Options; diff --git a/crates/pgt_analyser/tests/specs/safety/lockTimeoutWarning/basic.sql b/crates/pgt_analyser/tests/specs/safety/lockTimeoutWarning/basic.sql new file mode 100644 index 000000000..ef197ae14 --- /dev/null +++ b/crates/pgt_analyser/tests/specs/safety/lockTimeoutWarning/basic.sql @@ -0,0 +1,3 @@ +-- expect_only_lint/safety/lockTimeoutWarning +-- ALTER TABLE without lock timeout should trigger the rule +ALTER TABLE authors ADD COLUMN email TEXT; \ No newline at end of file diff --git a/crates/pgt_analyser/tests/specs/safety/lockTimeoutWarning/basic.sql.snap b/crates/pgt_analyser/tests/specs/safety/lockTimeoutWarning/basic.sql.snap new file mode 100644 index 000000000..a6c2e0ba2 --- /dev/null +++ b/crates/pgt_analyser/tests/specs/safety/lockTimeoutWarning/basic.sql.snap @@ -0,0 +1,20 @@ +--- +source: crates/pgt_analyser/tests/rules_tests.rs +expression: snapshot +snapshot_kind: text +--- +# Input +``` +-- expect_only_lint/safety/lockTimeoutWarning +-- ALTER TABLE without lock timeout should trigger the rule +ALTER TABLE authors ADD COLUMN email TEXT; +``` + +# Diagnostics +lint/safety/lockTimeoutWarning ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + × Statement takes ACCESS EXCLUSIVE lock on public.authors without lock timeout set. + + i This can block all operations on the table indefinitely if another transaction holds a conflicting lock. + + i Run 'SET LOCAL lock_timeout = '2s';' before this statement and retry the migration if it times out. diff --git a/crates/pgt_analyser/tests/specs/safety/lockTimeoutWarning/create_index_without_timeout.sql b/crates/pgt_analyser/tests/specs/safety/lockTimeoutWarning/create_index_without_timeout.sql new file mode 100644 index 000000000..fd96af806 --- /dev/null +++ b/crates/pgt_analyser/tests/specs/safety/lockTimeoutWarning/create_index_without_timeout.sql @@ -0,0 +1,3 @@ +-- expect_only_lint/safety/lockTimeoutWarning +-- CREATE INDEX without CONCURRENTLY or lock timeout should trigger the rule +CREATE INDEX books_title_idx ON books(title); diff --git a/crates/pgt_analyser/tests/specs/safety/lockTimeoutWarning/create_index_without_timeout.sql.snap b/crates/pgt_analyser/tests/specs/safety/lockTimeoutWarning/create_index_without_timeout.sql.snap new file mode 100644 index 000000000..07d0dfd7b --- /dev/null +++ b/crates/pgt_analyser/tests/specs/safety/lockTimeoutWarning/create_index_without_timeout.sql.snap @@ -0,0 +1,21 @@ +--- +source: crates/pgt_analyser/tests/rules_tests.rs +expression: snapshot +snapshot_kind: text +--- +# Input +``` +-- expect_only_lint/safety/lockTimeoutWarning +-- CREATE INDEX without CONCURRENTLY or lock timeout should trigger the rule +CREATE INDEX books_title_idx ON books(title); + +``` + +# Diagnostics +lint/safety/lockTimeoutWarning ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + × Statement takes SHARE lock on public.books while creating index books_title_idx without lock timeout set. + + i This blocks writes to the table indefinitely if another transaction holds a conflicting lock. + + i Run 'SET LOCAL lock_timeout = '2s';' before this statement, or use CREATE INDEX CONCURRENTLY to avoid blocking writes. diff --git a/crates/pgt_analyser/tests/specs/safety/lockTimeoutWarning/multiple_statements_without_timeout.sql b/crates/pgt_analyser/tests/specs/safety/lockTimeoutWarning/multiple_statements_without_timeout.sql new file mode 100644 index 000000000..5400a06d2 --- /dev/null +++ b/crates/pgt_analyser/tests/specs/safety/lockTimeoutWarning/multiple_statements_without_timeout.sql @@ -0,0 +1,6 @@ +-- Both statements should trigger the rule +-- expect_lint/safety/lockTimeoutWarning +CREATE INDEX orders_user_idx ON orders(user_id); + +-- expect_lint/safety/lockTimeoutWarning +ALTER TABLE orders ADD COLUMN total DECIMAL(10, 2); diff --git a/crates/pgt_analyser/tests/specs/safety/lockTimeoutWarning/multiple_statements_without_timeout.sql.snap b/crates/pgt_analyser/tests/specs/safety/lockTimeoutWarning/multiple_statements_without_timeout.sql.snap new file mode 100644 index 000000000..cf1868df8 --- /dev/null +++ b/crates/pgt_analyser/tests/specs/safety/lockTimeoutWarning/multiple_statements_without_timeout.sql.snap @@ -0,0 +1,34 @@ +--- +source: crates/pgt_analyser/tests/rules_tests.rs +expression: snapshot +snapshot_kind: text +--- +# Input +``` +-- Both statements should trigger the rule +-- expect_lint/safety/lockTimeoutWarning +CREATE INDEX orders_user_idx ON orders(user_id); + +-- expect_lint/safety/lockTimeoutWarning +ALTER TABLE orders ADD COLUMN total DECIMAL(10, 2); + +``` + +# Diagnostics +lint/safety/lockTimeoutWarning ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + × Statement takes SHARE lock on public.orders while creating index orders_user_idx without lock timeout set. + + i This blocks writes to the table indefinitely if another transaction holds a conflicting lock. + + i Run 'SET LOCAL lock_timeout = '2s';' before this statement, or use CREATE INDEX CONCURRENTLY to avoid blocking writes. + + + +lint/safety/lockTimeoutWarning ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + × Statement takes ACCESS EXCLUSIVE lock on public.orders without lock timeout set. + + i This can block all operations on the table indefinitely if another transaction holds a conflicting lock. + + i Run 'SET LOCAL lock_timeout = '2s';' before this statement and retry the migration if it times out. diff --git a/crates/pgt_analyser/tests/specs/safety/lockTimeoutWarning/timeout_set_variant.sql b/crates/pgt_analyser/tests/specs/safety/lockTimeoutWarning/timeout_set_variant.sql new file mode 100644 index 000000000..c57e04bb8 --- /dev/null +++ b/crates/pgt_analyser/tests/specs/safety/lockTimeoutWarning/timeout_set_variant.sql @@ -0,0 +1,4 @@ +-- Valid: SET lock_timeout (without LOCAL) also works +-- expect_no_diagnostics +SET lock_timeout = '1s'; +ALTER TABLE books ADD COLUMN author_id INT; diff --git a/crates/pgt_analyser/tests/specs/safety/lockTimeoutWarning/timeout_set_variant.sql.snap b/crates/pgt_analyser/tests/specs/safety/lockTimeoutWarning/timeout_set_variant.sql.snap new file mode 100644 index 000000000..13e990601 --- /dev/null +++ b/crates/pgt_analyser/tests/specs/safety/lockTimeoutWarning/timeout_set_variant.sql.snap @@ -0,0 +1,13 @@ +--- +source: crates/pgt_analyser/tests/rules_tests.rs +expression: snapshot +snapshot_kind: text +--- +# Input +``` +-- Valid: SET lock_timeout (without LOCAL) also works +-- expect_no_diagnostics +SET lock_timeout = '1s'; +ALTER TABLE books ADD COLUMN author_id INT; + +``` diff --git a/crates/pgt_analyser/tests/specs/safety/lockTimeoutWarning/valid_concurrent_index.sql b/crates/pgt_analyser/tests/specs/safety/lockTimeoutWarning/valid_concurrent_index.sql new file mode 100644 index 000000000..5aece53b1 --- /dev/null +++ b/crates/pgt_analyser/tests/specs/safety/lockTimeoutWarning/valid_concurrent_index.sql @@ -0,0 +1,3 @@ +-- Valid: CREATE INDEX CONCURRENTLY doesn't take dangerous locks +-- expect_no_diagnostics +CREATE INDEX CONCURRENTLY books_title_idx ON books(title); diff --git a/crates/pgt_analyser/tests/specs/safety/lockTimeoutWarning/valid_concurrent_index.sql.snap b/crates/pgt_analyser/tests/specs/safety/lockTimeoutWarning/valid_concurrent_index.sql.snap new file mode 100644 index 000000000..1284ce728 --- /dev/null +++ b/crates/pgt_analyser/tests/specs/safety/lockTimeoutWarning/valid_concurrent_index.sql.snap @@ -0,0 +1,12 @@ +--- +source: crates/pgt_analyser/tests/rules_tests.rs +expression: snapshot +snapshot_kind: text +--- +# Input +``` +-- Valid: CREATE INDEX CONCURRENTLY doesn't take dangerous locks +-- expect_no_diagnostics +CREATE INDEX CONCURRENTLY books_title_idx ON books(title); + +``` diff --git a/crates/pgt_analyser/tests/specs/safety/lockTimeoutWarning/valid_index_on_new_table.sql b/crates/pgt_analyser/tests/specs/safety/lockTimeoutWarning/valid_index_on_new_table.sql new file mode 100644 index 000000000..b918e7804 --- /dev/null +++ b/crates/pgt_analyser/tests/specs/safety/lockTimeoutWarning/valid_index_on_new_table.sql @@ -0,0 +1,8 @@ +-- Valid: CREATE INDEX on a table created in the same transaction doesn't need timeout +-- expect_no_diagnostics +CREATE TABLE products ( + id INT PRIMARY KEY, + name TEXT +); + +CREATE INDEX products_name_idx ON products(name); diff --git a/crates/pgt_analyser/tests/specs/safety/lockTimeoutWarning/valid_index_on_new_table.sql.snap b/crates/pgt_analyser/tests/specs/safety/lockTimeoutWarning/valid_index_on_new_table.sql.snap new file mode 100644 index 000000000..3e5e4ab45 --- /dev/null +++ b/crates/pgt_analyser/tests/specs/safety/lockTimeoutWarning/valid_index_on_new_table.sql.snap @@ -0,0 +1,17 @@ +--- +source: crates/pgt_analyser/tests/rules_tests.rs +expression: snapshot +snapshot_kind: text +--- +# Input +``` +-- Valid: CREATE INDEX on a table created in the same transaction doesn't need timeout +-- expect_no_diagnostics +CREATE TABLE products ( + id INT PRIMARY KEY, + name TEXT +); + +CREATE INDEX products_name_idx ON products(name); + +``` diff --git a/crates/pgt_analyser/tests/specs/safety/lockTimeoutWarning/valid_new_table.sql b/crates/pgt_analyser/tests/specs/safety/lockTimeoutWarning/valid_new_table.sql new file mode 100644 index 000000000..45c2b5e72 --- /dev/null +++ b/crates/pgt_analyser/tests/specs/safety/lockTimeoutWarning/valid_new_table.sql @@ -0,0 +1,8 @@ +-- Valid: ALTER TABLE on a table created in the same transaction doesn't need timeout +-- expect_no_diagnostics +CREATE TABLE users ( + id INT PRIMARY KEY, + name TEXT +); + +ALTER TABLE users ADD COLUMN email TEXT; diff --git a/crates/pgt_analyser/tests/specs/safety/lockTimeoutWarning/valid_new_table.sql.snap b/crates/pgt_analyser/tests/specs/safety/lockTimeoutWarning/valid_new_table.sql.snap new file mode 100644 index 000000000..5ca50762d --- /dev/null +++ b/crates/pgt_analyser/tests/specs/safety/lockTimeoutWarning/valid_new_table.sql.snap @@ -0,0 +1,17 @@ +--- +source: crates/pgt_analyser/tests/rules_tests.rs +expression: snapshot +snapshot_kind: text +--- +# Input +``` +-- Valid: ALTER TABLE on a table created in the same transaction doesn't need timeout +-- expect_no_diagnostics +CREATE TABLE users ( + id INT PRIMARY KEY, + name TEXT +); + +ALTER TABLE users ADD COLUMN email TEXT; + +``` diff --git a/crates/pgt_analyser/tests/specs/safety/lockTimeoutWarning/valid_with_timeout.sql b/crates/pgt_analyser/tests/specs/safety/lockTimeoutWarning/valid_with_timeout.sql new file mode 100644 index 000000000..3114205da --- /dev/null +++ b/crates/pgt_analyser/tests/specs/safety/lockTimeoutWarning/valid_with_timeout.sql @@ -0,0 +1,4 @@ +-- Valid: Lock timeout is set before ALTER TABLE +-- expect_no_diagnostics +SET LOCAL lock_timeout = '2s'; +ALTER TABLE authors ADD COLUMN email TEXT; diff --git a/crates/pgt_analyser/tests/specs/safety/lockTimeoutWarning/valid_with_timeout.sql.snap b/crates/pgt_analyser/tests/specs/safety/lockTimeoutWarning/valid_with_timeout.sql.snap new file mode 100644 index 000000000..b8880c935 --- /dev/null +++ b/crates/pgt_analyser/tests/specs/safety/lockTimeoutWarning/valid_with_timeout.sql.snap @@ -0,0 +1,13 @@ +--- +source: crates/pgt_analyser/tests/rules_tests.rs +expression: snapshot +snapshot_kind: text +--- +# Input +``` +-- Valid: Lock timeout is set before ALTER TABLE +-- expect_no_diagnostics +SET LOCAL lock_timeout = '2s'; +ALTER TABLE authors ADD COLUMN email TEXT; + +``` diff --git a/crates/pgt_configuration/src/analyser/linter/rules.rs b/crates/pgt_configuration/src/analyser/linter/rules.rs index 92268deb3..0b39403e6 100644 --- a/crates/pgt_configuration/src/analyser/linter/rules.rs +++ b/crates/pgt_configuration/src/analyser/linter/rules.rs @@ -199,6 +199,9 @@ pub struct Safety { #[serde(skip_serializing_if = "Option::is_none")] pub disallow_unique_constraint: Option>, + #[doc = "Taking a dangerous lock without setting a lock timeout can cause indefinite blocking."] + #[serde(skip_serializing_if = "Option::is_none")] + pub lock_timeout_warning: Option>, #[doc = "Multiple ALTER TABLE statements on the same table should be combined into a single statement."] #[serde(skip_serializing_if = "Option::is_none")] pub multiple_alter_table: Option>, @@ -266,6 +269,7 @@ impl Safety { "constraintMissingNotValid", "creatingEnum", "disallowUniqueConstraint", + "lockTimeoutWarning", "multipleAlterTable", "preferBigInt", "preferBigintOverInt", @@ -291,7 +295,7 @@ impl Safety { RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[8]), RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[10]), RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[11]), - RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[17]), + RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[18]), ]; const ALL_RULES_AS_FILTERS: &'static [RuleFilter<'static>] = &[ RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[0]), @@ -325,6 +329,7 @@ impl Safety { RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[28]), RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[29]), RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[30]), + RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[31]), ]; #[doc = r" Retrieves the recommended rules"] pub(crate) fn is_recommended_true(&self) -> bool { @@ -426,76 +431,81 @@ impl Safety { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[16])); } } - if let Some(rule) = self.multiple_alter_table.as_ref() { + if let Some(rule) = self.lock_timeout_warning.as_ref() { if rule.is_enabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[17])); } } - if let Some(rule) = self.prefer_big_int.as_ref() { + if let Some(rule) = self.multiple_alter_table.as_ref() { if rule.is_enabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[18])); } } - if let Some(rule) = self.prefer_bigint_over_int.as_ref() { + if let Some(rule) = self.prefer_big_int.as_ref() { if rule.is_enabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[19])); } } - if let Some(rule) = self.prefer_bigint_over_smallint.as_ref() { + if let Some(rule) = self.prefer_bigint_over_int.as_ref() { if rule.is_enabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[20])); } } - if let Some(rule) = self.prefer_identity.as_ref() { + if let Some(rule) = self.prefer_bigint_over_smallint.as_ref() { if rule.is_enabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[21])); } } - if let Some(rule) = self.prefer_jsonb.as_ref() { + if let Some(rule) = self.prefer_identity.as_ref() { if rule.is_enabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[22])); } } - if let Some(rule) = self.prefer_robust_stmts.as_ref() { + if let Some(rule) = self.prefer_jsonb.as_ref() { if rule.is_enabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[23])); } } - if let Some(rule) = self.prefer_text_field.as_ref() { + if let Some(rule) = self.prefer_robust_stmts.as_ref() { if rule.is_enabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[24])); } } - if let Some(rule) = self.prefer_timestamptz.as_ref() { + if let Some(rule) = self.prefer_text_field.as_ref() { if rule.is_enabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[25])); } } - if let Some(rule) = self.renaming_column.as_ref() { + if let Some(rule) = self.prefer_timestamptz.as_ref() { if rule.is_enabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[26])); } } - if let Some(rule) = self.renaming_table.as_ref() { + if let Some(rule) = self.renaming_column.as_ref() { if rule.is_enabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[27])); } } - if let Some(rule) = self.require_concurrent_index_creation.as_ref() { + if let Some(rule) = self.renaming_table.as_ref() { if rule.is_enabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[28])); } } - if let Some(rule) = self.require_concurrent_index_deletion.as_ref() { + if let Some(rule) = self.require_concurrent_index_creation.as_ref() { if rule.is_enabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[29])); } } - if let Some(rule) = self.transaction_nesting.as_ref() { + if let Some(rule) = self.require_concurrent_index_deletion.as_ref() { if rule.is_enabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[30])); } } + if let Some(rule) = self.transaction_nesting.as_ref() { + if rule.is_enabled() { + index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[31])); + } + } index_set } pub(crate) fn get_disabled_rules(&self) -> FxHashSet> { @@ -585,76 +595,81 @@ impl Safety { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[16])); } } - if let Some(rule) = self.multiple_alter_table.as_ref() { + if let Some(rule) = self.lock_timeout_warning.as_ref() { if rule.is_disabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[17])); } } - if let Some(rule) = self.prefer_big_int.as_ref() { + if let Some(rule) = self.multiple_alter_table.as_ref() { if rule.is_disabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[18])); } } - if let Some(rule) = self.prefer_bigint_over_int.as_ref() { + if let Some(rule) = self.prefer_big_int.as_ref() { if rule.is_disabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[19])); } } - if let Some(rule) = self.prefer_bigint_over_smallint.as_ref() { + if let Some(rule) = self.prefer_bigint_over_int.as_ref() { if rule.is_disabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[20])); } } - if let Some(rule) = self.prefer_identity.as_ref() { + if let Some(rule) = self.prefer_bigint_over_smallint.as_ref() { if rule.is_disabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[21])); } } - if let Some(rule) = self.prefer_jsonb.as_ref() { + if let Some(rule) = self.prefer_identity.as_ref() { if rule.is_disabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[22])); } } - if let Some(rule) = self.prefer_robust_stmts.as_ref() { + if let Some(rule) = self.prefer_jsonb.as_ref() { if rule.is_disabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[23])); } } - if let Some(rule) = self.prefer_text_field.as_ref() { + if let Some(rule) = self.prefer_robust_stmts.as_ref() { if rule.is_disabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[24])); } } - if let Some(rule) = self.prefer_timestamptz.as_ref() { + if let Some(rule) = self.prefer_text_field.as_ref() { if rule.is_disabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[25])); } } - if let Some(rule) = self.renaming_column.as_ref() { + if let Some(rule) = self.prefer_timestamptz.as_ref() { if rule.is_disabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[26])); } } - if let Some(rule) = self.renaming_table.as_ref() { + if let Some(rule) = self.renaming_column.as_ref() { if rule.is_disabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[27])); } } - if let Some(rule) = self.require_concurrent_index_creation.as_ref() { + if let Some(rule) = self.renaming_table.as_ref() { if rule.is_disabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[28])); } } - if let Some(rule) = self.require_concurrent_index_deletion.as_ref() { + if let Some(rule) = self.require_concurrent_index_creation.as_ref() { if rule.is_disabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[29])); } } - if let Some(rule) = self.transaction_nesting.as_ref() { + if let Some(rule) = self.require_concurrent_index_deletion.as_ref() { if rule.is_disabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[30])); } } + if let Some(rule) = self.transaction_nesting.as_ref() { + if rule.is_disabled() { + index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[31])); + } + } index_set } #[doc = r" Checks if, given a rule name, matches one of the rules contained in this category"] @@ -701,6 +716,7 @@ impl Safety { "constraintMissingNotValid" => Severity::Warning, "creatingEnum" => Severity::Warning, "disallowUniqueConstraint" => Severity::Error, + "lockTimeoutWarning" => Severity::Error, "multipleAlterTable" => Severity::Warning, "preferBigInt" => Severity::Warning, "preferBigintOverInt" => Severity::Warning, @@ -791,6 +807,10 @@ impl Safety { .disallow_unique_constraint .as_ref() .map(|conf| (conf.level(), conf.get_options())), + "lockTimeoutWarning" => self + .lock_timeout_warning + .as_ref() + .map(|conf| (conf.level(), conf.get_options())), "multipleAlterTable" => self .multiple_alter_table .as_ref() diff --git a/crates/pgt_diagnostics_categories/src/categories.rs b/crates/pgt_diagnostics_categories/src/categories.rs index 08a4f4cd2..b6c2881c8 100644 --- a/crates/pgt_diagnostics_categories/src/categories.rs +++ b/crates/pgt_diagnostics_categories/src/categories.rs @@ -30,6 +30,7 @@ define_categories! { "lint/safety/constraintMissingNotValid": "https://pgtools.dev/latest/rules/constraint-missing-not-valid", "lint/safety/creatingEnum": "https://pgtools.dev/latest/rules/creating-enum", "lint/safety/disallowUniqueConstraint": "https://pgtools.dev/latest/rules/disallow-unique-constraint", + "lint/safety/lockTimeoutWarning": "https://pgtools.dev/latest/rules/lock-timeout-warning", "lint/safety/multipleAlterTable": "https://pgtools.dev/latest/rules/multiple-alter-table", "lint/safety/preferBigInt": "https://pgtools.dev/latest/rules/prefer-big-int", "lint/safety/preferBigintOverInt": "https://pgtools.dev/latest/rules/prefer-bigint-over-int", diff --git a/docs/reference/rule_sources.md b/docs/reference/rule_sources.md index 8d4d2872b..cf43e484c 100644 --- a/docs/reference/rule_sources.md +++ b/docs/reference/rule_sources.md @@ -8,6 +8,7 @@ _No exclusive rules available._ | ---- | ---- | | [E11](https://kaveland.no/eugene/hints/E11/index.html) |[addSerialColumn](../rules/add-serial-column) | | [E3](https://kaveland.no/eugene/hints/E3/index.html) |[preferJsonb](../rules/prefer-jsonb) | +| [E9](https://kaveland.no/eugene/hints/E9/index.html) |[lockTimeoutWarning](../rules/lock-timeout-warning) | | [W12](https://kaveland.no/eugene/hints/W12/index.html) |[multipleAlterTable](../rules/multiple-alter-table) | | [W13](https://kaveland.no/eugene/hints/W13/index.html) |[creatingEnum](../rules/creating-enum) | ### Squawk diff --git a/docs/reference/rules.md b/docs/reference/rules.md index 35f7bd7c6..8c7294e21 100644 --- a/docs/reference/rules.md +++ b/docs/reference/rules.md @@ -29,6 +29,7 @@ Rules that detect potential safety issues in your code. | [constraintMissingNotValid](./constraint-missing-not-valid) | Adding constraints without NOT VALID blocks all reads and writes. | | | [creatingEnum](./creating-enum) | Creating enum types is not recommended for new applications. | | | [disallowUniqueConstraint](./disallow-unique-constraint) | Disallow adding a UNIQUE constraint without using an existing index. | | +| [lockTimeoutWarning](./lock-timeout-warning) | Taking a dangerous lock without setting a lock timeout can cause indefinite blocking. | | | [multipleAlterTable](./multiple-alter-table) | Multiple ALTER TABLE statements on the same table should be combined into a single statement. | ✅ | | [preferBigInt](./prefer-big-int) | Prefer BIGINT over smaller integer types. | | | [preferBigintOverInt](./prefer-bigint-over-int) | Prefer BIGINT over INT/INTEGER types. | | diff --git a/docs/reference/rules/lock-timeout-warning.md b/docs/reference/rules/lock-timeout-warning.md new file mode 100644 index 000000000..e27c6156a --- /dev/null +++ b/docs/reference/rules/lock-timeout-warning.md @@ -0,0 +1,87 @@ +# lockTimeoutWarning +**Diagnostic Category: `lint/safety/lockTimeoutWarning`** + +**Since**: `vnext` + + +**Sources**: +- Inspired from: eugene/E9 + +## Description +Taking a dangerous lock without setting a lock timeout can cause indefinite blocking. + +When a statement acquires a lock that would block common operations (like SELECT, INSERT, UPDATE, DELETE), +it can cause the database to become unresponsive if another transaction is holding a conflicting lock +while idle in transaction or active. This is particularly dangerous for: + +- ALTER TABLE statements (acquire ACCESS EXCLUSIVE lock) +- CREATE INDEX without CONCURRENTLY (acquires SHARE lock) + +Setting a lock timeout ensures that if the lock cannot be acquired within a reasonable time, +the statement will fail rather than blocking indefinitely. + +## Examples + +### Invalid + +```sql +ALTER TABLE users ADD COLUMN email TEXT; +``` + +```sh +code-block.sql:1:1 lint/safety/lockTimeoutWarning ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + × Statement takes ACCESS EXCLUSIVE lock on public.users without lock timeout set. + + > 1 │ ALTER TABLE users ADD COLUMN email TEXT; + │ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + 2 │ + + i This can block all operations on the table indefinitely if another transaction holds a conflicting lock. + + i Run 'SET LOCAL lock_timeout = '2s';' before this statement and retry the migration if it times out. + + +``` + +```sql +CREATE INDEX users_email_idx ON users(email); +``` + +```sh +code-block.sql:1:1 lint/safety/lockTimeoutWarning ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + × Statement takes SHARE lock on public.users while creating index users_email_idx without lock timeout set. + + > 1 │ CREATE INDEX users_email_idx ON users(email); + │ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + 2 │ + + i This blocks writes to the table indefinitely if another transaction holds a conflicting lock. + + i Run 'SET LOCAL lock_timeout = '2s';' before this statement, or use CREATE INDEX CONCURRENTLY to avoid blocking writes. + + +``` + +### Valid + +```sql +-- Use CONCURRENTLY to avoid taking dangerous locks +CREATE INDEX CONCURRENTLY users_email_idx ON users(email); +``` + +## How to configure +```json + +{ + "linter": { + "rules": { + "safety": { + "lockTimeoutWarning": "error" + } + } + } +} + +``` diff --git a/docs/schema.json b/docs/schema.json index 90dddbddf..7efacd795 100644 --- a/docs/schema.json +++ b/docs/schema.json @@ -550,6 +550,17 @@ } ] }, + "lockTimeoutWarning": { + "description": "Taking a dangerous lock without setting a lock timeout can cause indefinite blocking.", + "anyOf": [ + { + "$ref": "#/definitions/RuleConfiguration" + }, + { + "type": "null" + } + ] + }, "multipleAlterTable": { "description": "Multiple ALTER TABLE statements on the same table should be combined into a single statement.", "anyOf": [ diff --git a/packages/@postgrestools/backend-jsonrpc/src/workspace.ts b/packages/@postgrestools/backend-jsonrpc/src/workspace.ts index dc30b74b0..a7e544c06 100644 --- a/packages/@postgrestools/backend-jsonrpc/src/workspace.ts +++ b/packages/@postgrestools/backend-jsonrpc/src/workspace.ts @@ -80,6 +80,7 @@ export type Category = | "lint/safety/constraintMissingNotValid" | "lint/safety/creatingEnum" | "lint/safety/disallowUniqueConstraint" + | "lint/safety/lockTimeoutWarning" | "lint/safety/multipleAlterTable" | "lint/safety/preferBigInt" | "lint/safety/preferBigintOverInt" @@ -510,6 +511,10 @@ export interface Safety { * Disallow adding a UNIQUE constraint without using an existing index. */ disallowUniqueConstraint?: RuleConfiguration_for_Null; + /** + * Taking a dangerous lock without setting a lock timeout can cause indefinite blocking. + */ + lockTimeoutWarning?: RuleConfiguration_for_Null; /** * Multiple ALTER TABLE statements on the same table should be combined into a single statement. */ From 3edca2db264545d6eec02d430c4e9b4021d59816 Mon Sep 17 00:00:00 2001 From: psteinroe Date: Mon, 20 Oct 2025 08:29:36 +0200 Subject: [PATCH 6/8] feat(analyser): access exclusive --- .../pgt_analyse/src/analysed_file_context.rs | 22 ++++++ crates/pgt_analyser/src/lint/safety.rs | 3 +- .../src/lint/safety/lock_timeout_warning.rs | 2 +- ...tatement_while_holding_access_exclusive.rs | 72 +++++++++++++++++++ crates/pgt_analyser/src/options.rs | 1 + .../basic.sql | 4 ++ .../basic.sql.snap | 21 ++++++ .../create_index_after_alter.sql | 4 ++ .../create_index_after_alter.sql.snap | 22 ++++++ .../insert_after_alter.sql | 4 ++ .../insert_after_alter.sql.snap | 22 ++++++ .../multiple_statements.sql | 8 +++ .../multiple_statements.sql.snap | 36 ++++++++++ .../valid_before_alter.sql | 4 ++ .../valid_before_alter.sql.snap | 13 ++++ .../valid_new_table.sql | 9 +++ .../valid_new_table.sql.snap | 18 +++++ .../valid_single_alter.sql | 3 + .../valid_single_alter.sql.snap | 12 ++++ .../src/analyser/linter/rules.rs | 34 ++++++++- .../src/categories.rs | 1 + docs/reference/rule_sources.md | 1 + docs/reference/rules.md | 3 +- docs/reference/rules/lock-timeout-warning.md | 2 + ...tatement-while-holding-access-exclusive.md | 60 ++++++++++++++++ docs/schema.json | 11 +++ .../backend-jsonrpc/src/workspace.ts | 5 ++ 27 files changed, 392 insertions(+), 5 deletions(-) create mode 100644 crates/pgt_analyser/src/lint/safety/running_statement_while_holding_access_exclusive.rs create mode 100644 crates/pgt_analyser/tests/specs/safety/runningStatementWhileHoldingAccessExclusive/basic.sql create mode 100644 crates/pgt_analyser/tests/specs/safety/runningStatementWhileHoldingAccessExclusive/basic.sql.snap create mode 100644 crates/pgt_analyser/tests/specs/safety/runningStatementWhileHoldingAccessExclusive/create_index_after_alter.sql create mode 100644 crates/pgt_analyser/tests/specs/safety/runningStatementWhileHoldingAccessExclusive/create_index_after_alter.sql.snap create mode 100644 crates/pgt_analyser/tests/specs/safety/runningStatementWhileHoldingAccessExclusive/insert_after_alter.sql create mode 100644 crates/pgt_analyser/tests/specs/safety/runningStatementWhileHoldingAccessExclusive/insert_after_alter.sql.snap create mode 100644 crates/pgt_analyser/tests/specs/safety/runningStatementWhileHoldingAccessExclusive/multiple_statements.sql create mode 100644 crates/pgt_analyser/tests/specs/safety/runningStatementWhileHoldingAccessExclusive/multiple_statements.sql.snap create mode 100644 crates/pgt_analyser/tests/specs/safety/runningStatementWhileHoldingAccessExclusive/valid_before_alter.sql create mode 100644 crates/pgt_analyser/tests/specs/safety/runningStatementWhileHoldingAccessExclusive/valid_before_alter.sql.snap create mode 100644 crates/pgt_analyser/tests/specs/safety/runningStatementWhileHoldingAccessExclusive/valid_new_table.sql create mode 100644 crates/pgt_analyser/tests/specs/safety/runningStatementWhileHoldingAccessExclusive/valid_new_table.sql.snap create mode 100644 crates/pgt_analyser/tests/specs/safety/runningStatementWhileHoldingAccessExclusive/valid_single_alter.sql create mode 100644 crates/pgt_analyser/tests/specs/safety/runningStatementWhileHoldingAccessExclusive/valid_single_alter.sql.snap create mode 100644 docs/reference/rules/running-statement-while-holding-access-exclusive.md diff --git a/crates/pgt_analyse/src/analysed_file_context.rs b/crates/pgt_analyse/src/analysed_file_context.rs index a04ccb36e..020b8afb7 100644 --- a/crates/pgt_analyse/src/analysed_file_context.rs +++ b/crates/pgt_analyse/src/analysed_file_context.rs @@ -41,6 +41,7 @@ impl<'a> AnalysedFileContext<'a> { /// This tracks properties that span multiple statements, such as: /// - Whether a lock timeout has been set /// - Which objects have been created in this transaction +/// - Whether an ACCESS EXCLUSIVE lock is currently being held #[derive(Debug, Default)] pub struct TransactionState { /// Whether `SET lock_timeout` has been called in this transaction @@ -48,6 +49,9 @@ pub struct TransactionState { /// Objects (schema, name) created in this transaction /// Schema names are normalized: empty string is stored as "public" created_objects: Vec<(String, String)>, + /// Whether an ACCESS EXCLUSIVE lock is currently being held + /// This is set when an ALTER TABLE is executed on an existing table + holding_access_exclusive: bool, } impl TransactionState { @@ -66,6 +70,11 @@ impl TransactionState { .any(|(s, n)| normalized_schema.eq_ignore_ascii_case(s) && name.eq_ignore_ascii_case(n)) } + /// Returns true if the transaction is currently holding an ACCESS EXCLUSIVE lock + pub fn is_holding_access_exclusive(&self) -> bool { + self.holding_access_exclusive + } + /// Record that an object was created, normalizing the schema name fn add_created_object(&mut self, schema: String, name: String) { // Normalize schema: store "public" instead of empty string @@ -116,5 +125,18 @@ impl TransactionState { } _ => {} } + + // Track ACCESS EXCLUSIVE lock acquisition + // ALTER TABLE on an existing table acquires ACCESS EXCLUSIVE lock + if let pgt_query::NodeEnum::AlterTableStmt(alter_stmt) = stmt { + if let Some(relation) = &alter_stmt.relation { + let schema = &relation.schemaname; + let name = &relation.relname; + // Only set the flag if altering an existing table (not one created in this transaction) + if !self.has_created_object(schema, name) { + self.holding_access_exclusive = true; + } + } + } } } diff --git a/crates/pgt_analyser/src/lint/safety.rs b/crates/pgt_analyser/src/lint/safety.rs index cb9063cc1..32a050fc1 100644 --- a/crates/pgt_analyser/src/lint/safety.rs +++ b/crates/pgt_analyser/src/lint/safety.rs @@ -32,5 +32,6 @@ pub mod renaming_column; pub mod renaming_table; pub mod require_concurrent_index_creation; pub mod require_concurrent_index_deletion; +pub mod running_statement_while_holding_access_exclusive; pub mod transaction_nesting; -declare_lint_group! { pub Safety { name : "safety" , rules : [self :: add_serial_column :: AddSerialColumn , self :: adding_field_with_default :: AddingFieldWithDefault , self :: adding_foreign_key_constraint :: AddingForeignKeyConstraint , self :: adding_not_null_field :: AddingNotNullField , self :: adding_primary_key_constraint :: AddingPrimaryKeyConstraint , self :: adding_required_field :: AddingRequiredField , self :: ban_char_field :: BanCharField , self :: ban_concurrent_index_creation_in_transaction :: BanConcurrentIndexCreationInTransaction , self :: ban_drop_column :: BanDropColumn , self :: ban_drop_database :: BanDropDatabase , self :: ban_drop_not_null :: BanDropNotNull , self :: ban_drop_table :: BanDropTable , self :: ban_truncate_cascade :: BanTruncateCascade , self :: changing_column_type :: ChangingColumnType , self :: constraint_missing_not_valid :: ConstraintMissingNotValid , self :: creating_enum :: CreatingEnum , self :: disallow_unique_constraint :: DisallowUniqueConstraint , self :: lock_timeout_warning :: LockTimeoutWarning , self :: multiple_alter_table :: MultipleAlterTable , self :: prefer_big_int :: PreferBigInt , self :: prefer_bigint_over_int :: PreferBigintOverInt , self :: prefer_bigint_over_smallint :: PreferBigintOverSmallint , self :: prefer_identity :: PreferIdentity , self :: prefer_jsonb :: PreferJsonb , self :: prefer_robust_stmts :: PreferRobustStmts , self :: prefer_text_field :: PreferTextField , self :: prefer_timestamptz :: PreferTimestamptz , self :: renaming_column :: RenamingColumn , self :: renaming_table :: RenamingTable , self :: require_concurrent_index_creation :: RequireConcurrentIndexCreation , self :: require_concurrent_index_deletion :: RequireConcurrentIndexDeletion , self :: transaction_nesting :: TransactionNesting ,] } } +declare_lint_group! { pub Safety { name : "safety" , rules : [self :: add_serial_column :: AddSerialColumn , self :: adding_field_with_default :: AddingFieldWithDefault , self :: adding_foreign_key_constraint :: AddingForeignKeyConstraint , self :: adding_not_null_field :: AddingNotNullField , self :: adding_primary_key_constraint :: AddingPrimaryKeyConstraint , self :: adding_required_field :: AddingRequiredField , self :: ban_char_field :: BanCharField , self :: ban_concurrent_index_creation_in_transaction :: BanConcurrentIndexCreationInTransaction , self :: ban_drop_column :: BanDropColumn , self :: ban_drop_database :: BanDropDatabase , self :: ban_drop_not_null :: BanDropNotNull , self :: ban_drop_table :: BanDropTable , self :: ban_truncate_cascade :: BanTruncateCascade , self :: changing_column_type :: ChangingColumnType , self :: constraint_missing_not_valid :: ConstraintMissingNotValid , self :: creating_enum :: CreatingEnum , self :: disallow_unique_constraint :: DisallowUniqueConstraint , self :: lock_timeout_warning :: LockTimeoutWarning , self :: multiple_alter_table :: MultipleAlterTable , self :: prefer_big_int :: PreferBigInt , self :: prefer_bigint_over_int :: PreferBigintOverInt , self :: prefer_bigint_over_smallint :: PreferBigintOverSmallint , self :: prefer_identity :: PreferIdentity , self :: prefer_jsonb :: PreferJsonb , self :: prefer_robust_stmts :: PreferRobustStmts , self :: prefer_text_field :: PreferTextField , self :: prefer_timestamptz :: PreferTimestamptz , self :: renaming_column :: RenamingColumn , self :: renaming_table :: RenamingTable , self :: require_concurrent_index_creation :: RequireConcurrentIndexCreation , self :: require_concurrent_index_deletion :: RequireConcurrentIndexDeletion , self :: running_statement_while_holding_access_exclusive :: RunningStatementWhileHoldingAccessExclusive , self :: transaction_nesting :: TransactionNesting ,] } } diff --git a/crates/pgt_analyser/src/lint/safety/lock_timeout_warning.rs b/crates/pgt_analyser/src/lint/safety/lock_timeout_warning.rs index 6fe982cba..443a4ba53 100644 --- a/crates/pgt_analyser/src/lint/safety/lock_timeout_warning.rs +++ b/crates/pgt_analyser/src/lint/safety/lock_timeout_warning.rs @@ -38,7 +38,7 @@ declare_lint_rule! { version: "next", name: "lockTimeoutWarning", severity: Severity::Error, - recommended: false, + recommended: true, sources: &[RuleSource::Eugene("E9")], } } diff --git a/crates/pgt_analyser/src/lint/safety/running_statement_while_holding_access_exclusive.rs b/crates/pgt_analyser/src/lint/safety/running_statement_while_holding_access_exclusive.rs new file mode 100644 index 000000000..8c2917631 --- /dev/null +++ b/crates/pgt_analyser/src/lint/safety/running_statement_while_holding_access_exclusive.rs @@ -0,0 +1,72 @@ +use pgt_analyse::{Rule, RuleDiagnostic, RuleSource, context::RuleContext, declare_lint_rule}; +use pgt_console::markup; +use pgt_diagnostics::Severity; + +declare_lint_rule! { + /// Running additional statements while holding an ACCESS EXCLUSIVE lock blocks all table access. + /// + /// When a transaction acquires an ACCESS EXCLUSIVE lock (e.g., via ALTER TABLE), it blocks all other + /// operations on that table, including reads. Running additional statements in the same transaction + /// extends the duration the lock is held, potentially blocking all database access to that table. + /// + /// This is particularly problematic because: + /// - The lock blocks all SELECT, INSERT, UPDATE, DELETE operations + /// - The lock is held for the entire duration of all subsequent statements + /// - Even simple queries like SELECT COUNT(*) can significantly extend lock time + /// + /// To minimize blocking, run the ALTER TABLE in its own transaction and execute + /// other operations in separate transactions. + /// + /// ## Examples + /// + /// ### Invalid + /// + /// ```sql,expect_diagnostic + /// ALTER TABLE authors ADD COLUMN email TEXT; + /// SELECT COUNT(*) FROM authors; + /// ``` + /// + /// ### Valid + /// + /// ```sql + /// -- Run ALTER TABLE alone, other queries in separate transactions + /// ALTER TABLE authors ADD COLUMN email TEXT; + /// ``` + /// + pub RunningStatementWhileHoldingAccessExclusive { + version: "next", + name: "runningStatementWhileHoldingAccessExclusive", + severity: Severity::Warning, + recommended: true, + sources: &[RuleSource::Eugene("E4")], + } +} + +impl Rule for RunningStatementWhileHoldingAccessExclusive { + type Options = (); + + fn run(ctx: &RuleContext) -> Vec { + let mut diagnostics = Vec::new(); + + // Check if we're currently holding an ACCESS EXCLUSIVE lock + let tx_state = ctx.file_context().transaction_state(); + if tx_state.is_holding_access_exclusive() { + diagnostics.push( + RuleDiagnostic::new( + rule_category!(), + None, + markup! { + "Running statement while holding ACCESS EXCLUSIVE lock." + }, + ) + .detail( + None, + "This blocks all access to the table for the duration of this statement.", + ) + .note("Run this statement in a separate transaction to minimize lock duration."), + ); + } + + diagnostics + } +} diff --git a/crates/pgt_analyser/src/options.rs b/crates/pgt_analyser/src/options.rs index 340beabb2..598a6920d 100644 --- a/crates/pgt_analyser/src/options.rs +++ b/crates/pgt_analyser/src/options.rs @@ -50,5 +50,6 @@ pub type RenamingTable = ::Options; pub type RequireConcurrentIndexCreation = < lint :: safety :: require_concurrent_index_creation :: RequireConcurrentIndexCreation as pgt_analyse :: Rule > :: Options ; pub type RequireConcurrentIndexDeletion = < lint :: safety :: require_concurrent_index_deletion :: RequireConcurrentIndexDeletion as pgt_analyse :: Rule > :: Options ; +pub type RunningStatementWhileHoldingAccessExclusive = < lint :: safety :: running_statement_while_holding_access_exclusive :: RunningStatementWhileHoldingAccessExclusive as pgt_analyse :: Rule > :: Options ; pub type TransactionNesting = ::Options; diff --git a/crates/pgt_analyser/tests/specs/safety/runningStatementWhileHoldingAccessExclusive/basic.sql b/crates/pgt_analyser/tests/specs/safety/runningStatementWhileHoldingAccessExclusive/basic.sql new file mode 100644 index 000000000..cc3d2573b --- /dev/null +++ b/crates/pgt_analyser/tests/specs/safety/runningStatementWhileHoldingAccessExclusive/basic.sql @@ -0,0 +1,4 @@ +-- expect_only_lint/safety/runningStatementWhileHoldingAccessExclusive +-- Running SELECT after ALTER TABLE should trigger the rule +ALTER TABLE authors ADD COLUMN email TEXT NOT NULL; +SELECT COUNT(*) FROM authors; \ No newline at end of file diff --git a/crates/pgt_analyser/tests/specs/safety/runningStatementWhileHoldingAccessExclusive/basic.sql.snap b/crates/pgt_analyser/tests/specs/safety/runningStatementWhileHoldingAccessExclusive/basic.sql.snap new file mode 100644 index 000000000..5356d0058 --- /dev/null +++ b/crates/pgt_analyser/tests/specs/safety/runningStatementWhileHoldingAccessExclusive/basic.sql.snap @@ -0,0 +1,21 @@ +--- +source: crates/pgt_analyser/tests/rules_tests.rs +expression: snapshot +snapshot_kind: text +--- +# Input +``` +-- expect_only_lint/safety/runningStatementWhileHoldingAccessExclusive +-- Running SELECT after ALTER TABLE should trigger the rule +ALTER TABLE authors ADD COLUMN email TEXT NOT NULL; +SELECT COUNT(*) FROM authors; +``` + +# Diagnostics +lint/safety/runningStatementWhileHoldingAccessExclusive ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + × Running statement while holding ACCESS EXCLUSIVE lock. + + i This blocks all access to the table for the duration of this statement. + + i Run this statement in a separate transaction to minimize lock duration. diff --git a/crates/pgt_analyser/tests/specs/safety/runningStatementWhileHoldingAccessExclusive/create_index_after_alter.sql b/crates/pgt_analyser/tests/specs/safety/runningStatementWhileHoldingAccessExclusive/create_index_after_alter.sql new file mode 100644 index 000000000..fa986a281 --- /dev/null +++ b/crates/pgt_analyser/tests/specs/safety/runningStatementWhileHoldingAccessExclusive/create_index_after_alter.sql @@ -0,0 +1,4 @@ +-- expect_only_lint/safety/runningStatementWhileHoldingAccessExclusive +-- CREATE INDEX after ALTER TABLE should trigger +ALTER TABLE orders ADD COLUMN total DECIMAL(10, 2); +CREATE INDEX orders_total_idx ON orders(total); diff --git a/crates/pgt_analyser/tests/specs/safety/runningStatementWhileHoldingAccessExclusive/create_index_after_alter.sql.snap b/crates/pgt_analyser/tests/specs/safety/runningStatementWhileHoldingAccessExclusive/create_index_after_alter.sql.snap new file mode 100644 index 000000000..1b96e67f5 --- /dev/null +++ b/crates/pgt_analyser/tests/specs/safety/runningStatementWhileHoldingAccessExclusive/create_index_after_alter.sql.snap @@ -0,0 +1,22 @@ +--- +source: crates/pgt_analyser/tests/rules_tests.rs +expression: snapshot +snapshot_kind: text +--- +# Input +``` +-- expect_only_lint/safety/runningStatementWhileHoldingAccessExclusive +-- CREATE INDEX after ALTER TABLE should trigger +ALTER TABLE orders ADD COLUMN total DECIMAL(10, 2); +CREATE INDEX orders_total_idx ON orders(total); + +``` + +# Diagnostics +lint/safety/runningStatementWhileHoldingAccessExclusive ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + × Running statement while holding ACCESS EXCLUSIVE lock. + + i This blocks all access to the table for the duration of this statement. + + i Run this statement in a separate transaction to minimize lock duration. diff --git a/crates/pgt_analyser/tests/specs/safety/runningStatementWhileHoldingAccessExclusive/insert_after_alter.sql b/crates/pgt_analyser/tests/specs/safety/runningStatementWhileHoldingAccessExclusive/insert_after_alter.sql new file mode 100644 index 000000000..3d7dbf749 --- /dev/null +++ b/crates/pgt_analyser/tests/specs/safety/runningStatementWhileHoldingAccessExclusive/insert_after_alter.sql @@ -0,0 +1,4 @@ +-- expect_only_lint/safety/runningStatementWhileHoldingAccessExclusive +-- INSERT after ALTER TABLE should trigger +ALTER TABLE books ADD COLUMN isbn TEXT; +INSERT INTO books (title, isbn) VALUES ('Database Systems', '978-0-1234567-8-9'); diff --git a/crates/pgt_analyser/tests/specs/safety/runningStatementWhileHoldingAccessExclusive/insert_after_alter.sql.snap b/crates/pgt_analyser/tests/specs/safety/runningStatementWhileHoldingAccessExclusive/insert_after_alter.sql.snap new file mode 100644 index 000000000..54cd32d3b --- /dev/null +++ b/crates/pgt_analyser/tests/specs/safety/runningStatementWhileHoldingAccessExclusive/insert_after_alter.sql.snap @@ -0,0 +1,22 @@ +--- +source: crates/pgt_analyser/tests/rules_tests.rs +expression: snapshot +snapshot_kind: text +--- +# Input +``` +-- expect_only_lint/safety/runningStatementWhileHoldingAccessExclusive +-- INSERT after ALTER TABLE should trigger +ALTER TABLE books ADD COLUMN isbn TEXT; +INSERT INTO books (title, isbn) VALUES ('Database Systems', '978-0-1234567-8-9'); + +``` + +# Diagnostics +lint/safety/runningStatementWhileHoldingAccessExclusive ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + × Running statement while holding ACCESS EXCLUSIVE lock. + + i This blocks all access to the table for the duration of this statement. + + i Run this statement in a separate transaction to minimize lock duration. diff --git a/crates/pgt_analyser/tests/specs/safety/runningStatementWhileHoldingAccessExclusive/multiple_statements.sql b/crates/pgt_analyser/tests/specs/safety/runningStatementWhileHoldingAccessExclusive/multiple_statements.sql new file mode 100644 index 000000000..404127fa9 --- /dev/null +++ b/crates/pgt_analyser/tests/specs/safety/runningStatementWhileHoldingAccessExclusive/multiple_statements.sql @@ -0,0 +1,8 @@ +-- All statements after ALTER TABLE should trigger +ALTER TABLE users ADD COLUMN age INT; + +-- expect_lint/safety/runningStatementWhileHoldingAccessExclusive +UPDATE users SET age = 30; + +-- expect_lint/safety/runningStatementWhileHoldingAccessExclusive +SELECT * FROM users; diff --git a/crates/pgt_analyser/tests/specs/safety/runningStatementWhileHoldingAccessExclusive/multiple_statements.sql.snap b/crates/pgt_analyser/tests/specs/safety/runningStatementWhileHoldingAccessExclusive/multiple_statements.sql.snap new file mode 100644 index 000000000..eb748ecb2 --- /dev/null +++ b/crates/pgt_analyser/tests/specs/safety/runningStatementWhileHoldingAccessExclusive/multiple_statements.sql.snap @@ -0,0 +1,36 @@ +--- +source: crates/pgt_analyser/tests/rules_tests.rs +expression: snapshot +snapshot_kind: text +--- +# Input +``` +-- All statements after ALTER TABLE should trigger +ALTER TABLE users ADD COLUMN age INT; + +-- expect_lint/safety/runningStatementWhileHoldingAccessExclusive +UPDATE users SET age = 30; + +-- expect_lint/safety/runningStatementWhileHoldingAccessExclusive +SELECT * FROM users; + +``` + +# Diagnostics +lint/safety/runningStatementWhileHoldingAccessExclusive ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + × Running statement while holding ACCESS EXCLUSIVE lock. + + i This blocks all access to the table for the duration of this statement. + + i Run this statement in a separate transaction to minimize lock duration. + + + +lint/safety/runningStatementWhileHoldingAccessExclusive ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + × Running statement while holding ACCESS EXCLUSIVE lock. + + i This blocks all access to the table for the duration of this statement. + + i Run this statement in a separate transaction to minimize lock duration. diff --git a/crates/pgt_analyser/tests/specs/safety/runningStatementWhileHoldingAccessExclusive/valid_before_alter.sql b/crates/pgt_analyser/tests/specs/safety/runningStatementWhileHoldingAccessExclusive/valid_before_alter.sql new file mode 100644 index 000000000..68490455c --- /dev/null +++ b/crates/pgt_analyser/tests/specs/safety/runningStatementWhileHoldingAccessExclusive/valid_before_alter.sql @@ -0,0 +1,4 @@ +-- Valid: Statements before ALTER TABLE are fine +-- expect_no_diagnostics +SELECT COUNT(*) FROM authors; +CREATE INDEX authors_name_idx ON authors(name); diff --git a/crates/pgt_analyser/tests/specs/safety/runningStatementWhileHoldingAccessExclusive/valid_before_alter.sql.snap b/crates/pgt_analyser/tests/specs/safety/runningStatementWhileHoldingAccessExclusive/valid_before_alter.sql.snap new file mode 100644 index 000000000..60451f6e7 --- /dev/null +++ b/crates/pgt_analyser/tests/specs/safety/runningStatementWhileHoldingAccessExclusive/valid_before_alter.sql.snap @@ -0,0 +1,13 @@ +--- +source: crates/pgt_analyser/tests/rules_tests.rs +expression: snapshot +snapshot_kind: text +--- +# Input +``` +-- Valid: Statements before ALTER TABLE are fine +-- expect_no_diagnostics +SELECT COUNT(*) FROM authors; +CREATE INDEX authors_name_idx ON authors(name); + +``` diff --git a/crates/pgt_analyser/tests/specs/safety/runningStatementWhileHoldingAccessExclusive/valid_new_table.sql b/crates/pgt_analyser/tests/specs/safety/runningStatementWhileHoldingAccessExclusive/valid_new_table.sql new file mode 100644 index 000000000..5f6d0a7e5 --- /dev/null +++ b/crates/pgt_analyser/tests/specs/safety/runningStatementWhileHoldingAccessExclusive/valid_new_table.sql @@ -0,0 +1,9 @@ +-- Valid: ALTER TABLE on newly created table doesn't hold ACCESS EXCLUSIVE +-- expect_no_diagnostics +CREATE TABLE products ( + id INT PRIMARY KEY, + name TEXT +); + +ALTER TABLE products ADD COLUMN price DECIMAL(10, 2); +SELECT COUNT(*) FROM products; diff --git a/crates/pgt_analyser/tests/specs/safety/runningStatementWhileHoldingAccessExclusive/valid_new_table.sql.snap b/crates/pgt_analyser/tests/specs/safety/runningStatementWhileHoldingAccessExclusive/valid_new_table.sql.snap new file mode 100644 index 000000000..0a824452e --- /dev/null +++ b/crates/pgt_analyser/tests/specs/safety/runningStatementWhileHoldingAccessExclusive/valid_new_table.sql.snap @@ -0,0 +1,18 @@ +--- +source: crates/pgt_analyser/tests/rules_tests.rs +expression: snapshot +snapshot_kind: text +--- +# Input +``` +-- Valid: ALTER TABLE on newly created table doesn't hold ACCESS EXCLUSIVE +-- expect_no_diagnostics +CREATE TABLE products ( + id INT PRIMARY KEY, + name TEXT +); + +ALTER TABLE products ADD COLUMN price DECIMAL(10, 2); +SELECT COUNT(*) FROM products; + +``` diff --git a/crates/pgt_analyser/tests/specs/safety/runningStatementWhileHoldingAccessExclusive/valid_single_alter.sql b/crates/pgt_analyser/tests/specs/safety/runningStatementWhileHoldingAccessExclusive/valid_single_alter.sql new file mode 100644 index 000000000..4b5d85408 --- /dev/null +++ b/crates/pgt_analyser/tests/specs/safety/runningStatementWhileHoldingAccessExclusive/valid_single_alter.sql @@ -0,0 +1,3 @@ +-- Valid: ALTER TABLE alone in transaction (no subsequent statements) +-- expect_no_diagnostics +ALTER TABLE authors ADD COLUMN email TEXT; diff --git a/crates/pgt_analyser/tests/specs/safety/runningStatementWhileHoldingAccessExclusive/valid_single_alter.sql.snap b/crates/pgt_analyser/tests/specs/safety/runningStatementWhileHoldingAccessExclusive/valid_single_alter.sql.snap new file mode 100644 index 000000000..2ebd08c5c --- /dev/null +++ b/crates/pgt_analyser/tests/specs/safety/runningStatementWhileHoldingAccessExclusive/valid_single_alter.sql.snap @@ -0,0 +1,12 @@ +--- +source: crates/pgt_analyser/tests/rules_tests.rs +expression: snapshot +snapshot_kind: text +--- +# Input +``` +-- Valid: ALTER TABLE alone in transaction (no subsequent statements) +-- expect_no_diagnostics +ALTER TABLE authors ADD COLUMN email TEXT; + +``` diff --git a/crates/pgt_configuration/src/analyser/linter/rules.rs b/crates/pgt_configuration/src/analyser/linter/rules.rs index 0b39403e6..a5c7b8181 100644 --- a/crates/pgt_configuration/src/analyser/linter/rules.rs +++ b/crates/pgt_configuration/src/analyser/linter/rules.rs @@ -245,6 +245,11 @@ pub struct Safety { #[serde(skip_serializing_if = "Option::is_none")] pub require_concurrent_index_deletion: Option>, + #[doc = "Running additional statements while holding an ACCESS EXCLUSIVE lock blocks all table access."] + #[serde(skip_serializing_if = "Option::is_none")] + pub running_statement_while_holding_access_exclusive: Option< + RuleConfiguration, + >, #[doc = "Detects problematic transaction nesting that could lead to unexpected behavior."] #[serde(skip_serializing_if = "Option::is_none")] pub transaction_nesting: Option>, @@ -283,6 +288,7 @@ impl Safety { "renamingTable", "requireConcurrentIndexCreation", "requireConcurrentIndexDeletion", + "runningStatementWhileHoldingAccessExclusive", "transactionNesting", ]; const RECOMMENDED_RULES_AS_FILTERS: &'static [RuleFilter<'static>] = &[ @@ -295,7 +301,9 @@ impl Safety { RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[8]), RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[10]), RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[11]), + RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[17]), RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[18]), + RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[31]), ]; const ALL_RULES_AS_FILTERS: &'static [RuleFilter<'static>] = &[ RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[0]), @@ -330,6 +338,7 @@ impl Safety { RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[29]), RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[30]), RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[31]), + RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[32]), ]; #[doc = r" Retrieves the recommended rules"] pub(crate) fn is_recommended_true(&self) -> bool { @@ -501,11 +510,19 @@ impl Safety { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[30])); } } - if let Some(rule) = self.transaction_nesting.as_ref() { + if let Some(rule) = self + .running_statement_while_holding_access_exclusive + .as_ref() + { if rule.is_enabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[31])); } } + if let Some(rule) = self.transaction_nesting.as_ref() { + if rule.is_enabled() { + index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[32])); + } + } index_set } pub(crate) fn get_disabled_rules(&self) -> FxHashSet> { @@ -665,11 +682,19 @@ impl Safety { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[30])); } } - if let Some(rule) = self.transaction_nesting.as_ref() { + if let Some(rule) = self + .running_statement_while_holding_access_exclusive + .as_ref() + { if rule.is_disabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[31])); } } + if let Some(rule) = self.transaction_nesting.as_ref() { + if rule.is_disabled() { + index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[32])); + } + } index_set } #[doc = r" Checks if, given a rule name, matches one of the rules contained in this category"] @@ -730,6 +755,7 @@ impl Safety { "renamingTable" => Severity::Warning, "requireConcurrentIndexCreation" => Severity::Warning, "requireConcurrentIndexDeletion" => Severity::Warning, + "runningStatementWhileHoldingAccessExclusive" => Severity::Error, "transactionNesting" => Severity::Warning, _ => unreachable!(), } @@ -863,6 +889,10 @@ impl Safety { .require_concurrent_index_deletion .as_ref() .map(|conf| (conf.level(), conf.get_options())), + "runningStatementWhileHoldingAccessExclusive" => self + .running_statement_while_holding_access_exclusive + .as_ref() + .map(|conf| (conf.level(), conf.get_options())), "transactionNesting" => self .transaction_nesting .as_ref() diff --git a/crates/pgt_diagnostics_categories/src/categories.rs b/crates/pgt_diagnostics_categories/src/categories.rs index b6c2881c8..cbe9af24e 100644 --- a/crates/pgt_diagnostics_categories/src/categories.rs +++ b/crates/pgt_diagnostics_categories/src/categories.rs @@ -44,6 +44,7 @@ define_categories! { "lint/safety/renamingTable": "https://pgtools.dev/latest/rules/renaming-table", "lint/safety/requireConcurrentIndexCreation": "https://pgtools.dev/latest/rules/require-concurrent-index-creation", "lint/safety/requireConcurrentIndexDeletion": "https://pgtools.dev/latest/rules/require-concurrent-index-deletion", + "lint/safety/runningStatementWhileHoldingAccessExclusive": "https://pgtools.dev/latest/rules/running-statement-while-holding-access-exclusive", "lint/safety/transactionNesting": "https://pgtools.dev/latest/rules/transaction-nesting", // end lint rules ; diff --git a/docs/reference/rule_sources.md b/docs/reference/rule_sources.md index cf43e484c..06dfa5db8 100644 --- a/docs/reference/rule_sources.md +++ b/docs/reference/rule_sources.md @@ -8,6 +8,7 @@ _No exclusive rules available._ | ---- | ---- | | [E11](https://kaveland.no/eugene/hints/E11/index.html) |[addSerialColumn](../rules/add-serial-column) | | [E3](https://kaveland.no/eugene/hints/E3/index.html) |[preferJsonb](../rules/prefer-jsonb) | +| [E4](https://kaveland.no/eugene/hints/E4/index.html) |[runningStatementWhileHoldingAccessExclusive](../rules/running-statement-while-holding-access-exclusive) | | [E9](https://kaveland.no/eugene/hints/E9/index.html) |[lockTimeoutWarning](../rules/lock-timeout-warning) | | [W12](https://kaveland.no/eugene/hints/W12/index.html) |[multipleAlterTable](../rules/multiple-alter-table) | | [W13](https://kaveland.no/eugene/hints/W13/index.html) |[creatingEnum](../rules/creating-enum) | diff --git a/docs/reference/rules.md b/docs/reference/rules.md index 8c7294e21..9d798987c 100644 --- a/docs/reference/rules.md +++ b/docs/reference/rules.md @@ -29,7 +29,7 @@ Rules that detect potential safety issues in your code. | [constraintMissingNotValid](./constraint-missing-not-valid) | Adding constraints without NOT VALID blocks all reads and writes. | | | [creatingEnum](./creating-enum) | Creating enum types is not recommended for new applications. | | | [disallowUniqueConstraint](./disallow-unique-constraint) | Disallow adding a UNIQUE constraint without using an existing index. | | -| [lockTimeoutWarning](./lock-timeout-warning) | Taking a dangerous lock without setting a lock timeout can cause indefinite blocking. | | +| [lockTimeoutWarning](./lock-timeout-warning) | Taking a dangerous lock without setting a lock timeout can cause indefinite blocking. | ✅ | | [multipleAlterTable](./multiple-alter-table) | Multiple ALTER TABLE statements on the same table should be combined into a single statement. | ✅ | | [preferBigInt](./prefer-big-int) | Prefer BIGINT over smaller integer types. | | | [preferBigintOverInt](./prefer-bigint-over-int) | Prefer BIGINT over INT/INTEGER types. | | @@ -43,6 +43,7 @@ Rules that detect potential safety issues in your code. | [renamingTable](./renaming-table) | Renaming tables may break existing queries and application code. | | | [requireConcurrentIndexCreation](./require-concurrent-index-creation) | Creating indexes non-concurrently can lock the table for writes. | | | [requireConcurrentIndexDeletion](./require-concurrent-index-deletion) | Dropping indexes non-concurrently can lock the table for reads. | | +| [runningStatementWhileHoldingAccessExclusive](./running-statement-while-holding-access-exclusive) | Running additional statements while holding an ACCESS EXCLUSIVE lock blocks all table access. | ✅ | | [transactionNesting](./transaction-nesting) | Detects problematic transaction nesting that could lead to unexpected behavior. | | [//]: # (END RULES_INDEX) diff --git a/docs/reference/rules/lock-timeout-warning.md b/docs/reference/rules/lock-timeout-warning.md index e27c6156a..cd1e36a32 100644 --- a/docs/reference/rules/lock-timeout-warning.md +++ b/docs/reference/rules/lock-timeout-warning.md @@ -3,6 +3,8 @@ **Since**: `vnext` +> [!NOTE] +> This rule is recommended. A diagnostic error will appear when linting your code. **Sources**: - Inspired from: eugene/E9 diff --git a/docs/reference/rules/running-statement-while-holding-access-exclusive.md b/docs/reference/rules/running-statement-while-holding-access-exclusive.md new file mode 100644 index 000000000..30392cbaa --- /dev/null +++ b/docs/reference/rules/running-statement-while-holding-access-exclusive.md @@ -0,0 +1,60 @@ +# runningStatementWhileHoldingAccessExclusive +**Diagnostic Category: `lint/safety/runningStatementWhileHoldingAccessExclusive`** + +**Since**: `vnext` + +> [!NOTE] +> This rule is recommended. A diagnostic error will appear when linting your code. + +**Sources**: +- Inspired from: eugene/E4 + +## Description +Running additional statements while holding an ACCESS EXCLUSIVE lock blocks all table access. + +When a transaction acquires an ACCESS EXCLUSIVE lock (e.g., via ALTER TABLE), it blocks all other +operations on that table, including reads. Running additional statements in the same transaction +extends the duration the lock is held, potentially blocking all database access to that table. + +This is particularly problematic because: + +- The lock blocks all SELECT, INSERT, UPDATE, DELETE operations +- The lock is held for the entire duration of all subsequent statements +- Even simple queries like SELECT COUNT(\*) can significantly extend lock time + +To minimize blocking, run the ALTER TABLE in its own transaction and execute +other operations in separate transactions. + +## Examples + +### Invalid + +```sql +ALTER TABLE authors ADD COLUMN email TEXT; +SELECT COUNT(*) FROM authors; +``` + +```sh +``` + +### Valid + +```sql +-- Run ALTER TABLE alone, other queries in separate transactions +ALTER TABLE authors ADD COLUMN email TEXT; +``` + +## How to configure +```json + +{ + "linter": { + "rules": { + "safety": { + "runningStatementWhileHoldingAccessExclusive": "error" + } + } + } +} + +``` diff --git a/docs/schema.json b/docs/schema.json index 7efacd795..959c00193 100644 --- a/docs/schema.json +++ b/docs/schema.json @@ -711,6 +711,17 @@ } ] }, + "runningStatementWhileHoldingAccessExclusive": { + "description": "Running additional statements while holding an ACCESS EXCLUSIVE lock blocks all table access.", + "anyOf": [ + { + "$ref": "#/definitions/RuleConfiguration" + }, + { + "type": "null" + } + ] + }, "transactionNesting": { "description": "Detects problematic transaction nesting that could lead to unexpected behavior.", "anyOf": [ diff --git a/packages/@postgrestools/backend-jsonrpc/src/workspace.ts b/packages/@postgrestools/backend-jsonrpc/src/workspace.ts index a7e544c06..6e2f6a881 100644 --- a/packages/@postgrestools/backend-jsonrpc/src/workspace.ts +++ b/packages/@postgrestools/backend-jsonrpc/src/workspace.ts @@ -94,6 +94,7 @@ export type Category = | "lint/safety/renamingTable" | "lint/safety/requireConcurrentIndexCreation" | "lint/safety/requireConcurrentIndexDeletion" + | "lint/safety/runningStatementWhileHoldingAccessExclusive" | "lint/safety/transactionNesting" | "stdin" | "check" @@ -571,6 +572,10 @@ export interface Safety { * Dropping indexes non-concurrently can lock the table for reads. */ requireConcurrentIndexDeletion?: RuleConfiguration_for_Null; + /** + * Running additional statements while holding an ACCESS EXCLUSIVE lock blocks all table access. + */ + runningStatementWhileHoldingAccessExclusive?: RuleConfiguration_for_Null; /** * Detects problematic transaction nesting that could lead to unexpected behavior. */ From 77def76b55c85590999316e634fbc49651c2d3b3 Mon Sep 17 00:00:00 2001 From: psteinroe Date: Mon, 20 Oct 2025 08:32:22 +0200 Subject: [PATCH 7/8] make them warn only --- crates/pgt_analyser/src/lint/safety/add_serial_column.rs | 2 +- crates/pgt_analyser/src/lint/safety/lock_timeout_warning.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/pgt_analyser/src/lint/safety/add_serial_column.rs b/crates/pgt_analyser/src/lint/safety/add_serial_column.rs index b907f1ac1..9278509bc 100644 --- a/crates/pgt_analyser/src/lint/safety/add_serial_column.rs +++ b/crates/pgt_analyser/src/lint/safety/add_serial_column.rs @@ -31,7 +31,7 @@ declare_lint_rule! { pub AddSerialColumn { version: "next", name: "addSerialColumn", - severity: Severity::Error, + severity: Severity::Warning, recommended: true, sources: &[RuleSource::Eugene("E11")], } diff --git a/crates/pgt_analyser/src/lint/safety/lock_timeout_warning.rs b/crates/pgt_analyser/src/lint/safety/lock_timeout_warning.rs index 443a4ba53..7c0275a7e 100644 --- a/crates/pgt_analyser/src/lint/safety/lock_timeout_warning.rs +++ b/crates/pgt_analyser/src/lint/safety/lock_timeout_warning.rs @@ -37,7 +37,7 @@ declare_lint_rule! { pub LockTimeoutWarning { version: "next", name: "lockTimeoutWarning", - severity: Severity::Error, + severity: Severity::Warning, recommended: true, sources: &[RuleSource::Eugene("E9")], } From 5eef98196a28e57fd8f03cc47266cf640b7d0544 Mon Sep 17 00:00:00 2001 From: psteinroe Date: Mon, 20 Oct 2025 09:36:16 +0200 Subject: [PATCH 8/8] progress --- crates/pgt_configuration/src/analyser/linter/rules.rs | 6 +++--- crates/pgt_lsp/tests/server.rs | 4 +--- docs/reference/rules/add-serial-column.md | 6 +++--- docs/reference/rules/lock-timeout-warning.md | 4 ++-- 4 files changed, 9 insertions(+), 11 deletions(-) diff --git a/crates/pgt_configuration/src/analyser/linter/rules.rs b/crates/pgt_configuration/src/analyser/linter/rules.rs index a5c7b8181..b0b44dcc9 100644 --- a/crates/pgt_configuration/src/analyser/linter/rules.rs +++ b/crates/pgt_configuration/src/analyser/linter/rules.rs @@ -724,7 +724,7 @@ impl Safety { } pub(crate) fn severity(rule_name: &str) -> Severity { match rule_name { - "addSerialColumn" => Severity::Error, + "addSerialColumn" => Severity::Warning, "addingFieldWithDefault" => Severity::Warning, "addingForeignKeyConstraint" => Severity::Warning, "addingNotNullField" => Severity::Warning, @@ -741,7 +741,7 @@ impl Safety { "constraintMissingNotValid" => Severity::Warning, "creatingEnum" => Severity::Warning, "disallowUniqueConstraint" => Severity::Error, - "lockTimeoutWarning" => Severity::Error, + "lockTimeoutWarning" => Severity::Warning, "multipleAlterTable" => Severity::Warning, "preferBigInt" => Severity::Warning, "preferBigintOverInt" => Severity::Warning, @@ -755,7 +755,7 @@ impl Safety { "renamingTable" => Severity::Warning, "requireConcurrentIndexCreation" => Severity::Warning, "requireConcurrentIndexDeletion" => Severity::Warning, - "runningStatementWhileHoldingAccessExclusive" => Severity::Error, + "runningStatementWhileHoldingAccessExclusive" => Severity::Warning, "transactionNesting" => Severity::Warning, _ => unreachable!(), } diff --git a/crates/pgt_lsp/tests/server.rs b/crates/pgt_lsp/tests/server.rs index ac2e8f936..8b44af01e 100644 --- a/crates/pgt_lsp/tests/server.rs +++ b/crates/pgt_lsp/tests/server.rs @@ -1690,9 +1690,7 @@ ALTER TABLE ONLY "public"."campaign_contact_list" .filter(|d| { d.code.as_ref().is_none_or(|c| match c { lsp::NumberOrString::Number(_) => true, - lsp::NumberOrString::String(s) => { - s != "lint/safety/addingForeignKeyConstraint" - } + lsp::NumberOrString::String(s) => !s.starts_with("lint/safety"), }) }) .count() diff --git a/docs/reference/rules/add-serial-column.md b/docs/reference/rules/add-serial-column.md index 70fab7601..6047cb43e 100644 --- a/docs/reference/rules/add-serial-column.md +++ b/docs/reference/rules/add-serial-column.md @@ -30,7 +30,7 @@ ALTER TABLE prices ADD COLUMN id serial; ```sh code-block.sql:1:1 lint/safety/addSerialColumn ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - × Adding a column with type serial requires a table rewrite. + ! Adding a column with type serial requires a table rewrite. > 1 │ ALTER TABLE prices ADD COLUMN id serial; │ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -50,7 +50,7 @@ ALTER TABLE prices ADD COLUMN id bigserial; ```sh code-block.sql:1:1 lint/safety/addSerialColumn ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - × Adding a column with type bigserial requires a table rewrite. + ! Adding a column with type bigserial requires a table rewrite. > 1 │ ALTER TABLE prices ADD COLUMN id bigserial; │ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -70,7 +70,7 @@ ALTER TABLE prices ADD COLUMN total int GENERATED ALWAYS AS (price * quantity) S ```sh code-block.sql:1:1 lint/safety/addSerialColumn ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - × Adding a column with GENERATED ALWAYS AS ... STORED requires a table rewrite. + ! Adding a column with GENERATED ALWAYS AS ... STORED requires a table rewrite. > 1 │ ALTER TABLE prices ADD COLUMN total int GENERATED ALWAYS AS (price * quantity) STORED; │ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ diff --git a/docs/reference/rules/lock-timeout-warning.md b/docs/reference/rules/lock-timeout-warning.md index cd1e36a32..fb4a79a9f 100644 --- a/docs/reference/rules/lock-timeout-warning.md +++ b/docs/reference/rules/lock-timeout-warning.md @@ -33,7 +33,7 @@ ALTER TABLE users ADD COLUMN email TEXT; ```sh code-block.sql:1:1 lint/safety/lockTimeoutWarning ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - × Statement takes ACCESS EXCLUSIVE lock on public.users without lock timeout set. + ! Statement takes ACCESS EXCLUSIVE lock on public.users without lock timeout set. > 1 │ ALTER TABLE users ADD COLUMN email TEXT; │ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -53,7 +53,7 @@ CREATE INDEX users_email_idx ON users(email); ```sh code-block.sql:1:1 lint/safety/lockTimeoutWarning ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ - × Statement takes SHARE lock on public.users while creating index users_email_idx without lock timeout set. + ! Statement takes SHARE lock on public.users while creating index users_email_idx without lock timeout set. > 1 │ CREATE INDEX users_email_idx ON users(email); │ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^