From 4bd4e667aa0d4cf7ec1ce0c793166956263c1502 Mon Sep 17 00:00:00 2001 From: Steve Dignam Date: Tue, 8 Sep 2020 23:26:03 -0400 Subject: [PATCH] add(rules): adding_primary_key rule to check for blocking pk creation (#64) As outlined in the citusdata post, adding primary keys to existing tables is a blocking operation. As such, Squawk should warn about them. https://www.citusdata.com/blog/2018/02/22/seven-tips-for-dealing-with-postgres-locks/ rel: https://github.com/sbdchd/squawk/issues/60 --- README.md | 23 ++++++ linter/src/lib.rs | 21 ++++- .../rules/adding_primary_key_constraint.rs | 77 +++++++++++++++++++ linter/src/rules/mod.rs | 2 + ...aint__test_rules__plain_primary_key-2.snap | 25 ++++++ ...traint__test_rules__plain_primary_key.snap | 23 ++++++ ...raint__test_rules__serial_primary_key.snap | 23 ++++++ ...est_rules__adding_unique_constraint-2.snap | 20 ++++- linter/src/violations.rs | 3 + 9 files changed, 212 insertions(+), 5 deletions(-) create mode 100644 linter/src/rules/adding_primary_key_constraint.rs create mode 100644 linter/src/rules/snapshots/squawk_linter__rules__adding_primary_key_constraint__test_rules__plain_primary_key-2.snap create mode 100644 linter/src/rules/snapshots/squawk_linter__rules__adding_primary_key_constraint__test_rules__plain_primary_key.snap create mode 100644 linter/src/rules/snapshots/squawk_linter__rules__adding_primary_key_constraint__test_rules__serial_primary_key.snap diff --git a/README.md b/README.md index 918794ac..51deee0b 100644 --- a/README.md +++ b/README.md @@ -226,6 +226,29 @@ From the postgres docs: See the `prefer-text-field` rule for info on the advantages of `text` over `varchar`. +### `adding-primary-key-field` + +Outlined in [Citus' 2018 post on tips for Postgres +locking](https://www.citusdata.com/blog/2018/02/22/seven-tips-for-dealing-with-postgres-locks/) +as well as the Postgres docs, adding a primary key constraint is a blocking +operation. + +Instead of creating the constraint directly, consider creating the +`CONSTRAINT` `USING` an index. + +From the Postgres docs: + +> To recreate a primary key constraint, without blocking updates while the +> index is rebuilt: + +```sql +CREATE UNIQUE INDEX CONCURRENTLY dist_id_temp_idx ON distributors (dist_id); +ALTER TABLE distributors DROP CONSTRAINT distributors_pkey, + ADD CONSTRAINT distributors_pkey PRIMARY KEY USING INDEX dist_id_temp_idx; +``` + + + ## Bot Setup Squawk works as a CLI tool but can also create comments on GitHub Pull diff --git a/linter/src/lib.rs b/linter/src/lib.rs index f48c510b..86f56237 100644 --- a/linter/src/lib.rs +++ b/linter/src/lib.rs @@ -8,10 +8,10 @@ extern crate lazy_static; use crate::errors::CheckSQLError; use crate::rules::{ - adding_field_with_default, adding_not_nullable_field, ban_char_type, ban_drop_database, - changing_column_type, constraint_missing_not_valid, disallow_unique_constraint, - prefer_robust_stmts, prefer_text_field, renaming_column, renaming_table, - require_concurrent_index_creation, + adding_field_with_default, adding_not_nullable_field, adding_primary_key_constraint, + ban_char_type, ban_drop_database, changing_column_type, constraint_missing_not_valid, + disallow_unique_constraint, prefer_robust_stmts, prefer_text_field, renaming_column, + renaming_table, require_concurrent_index_creation, }; use crate::violations::{RuleViolation, RuleViolationKind, ViolationMessage}; use squawk_parser::ast::RootStmt; @@ -95,6 +95,19 @@ lazy_static! { ViolationMessage::Help("Make the field nullable.".into()) ], }, + SquawkRule { + name: RuleViolationKind::AddingSerialPrimaryKeyField, + func: adding_primary_key_constraint, + messages: vec![ + ViolationMessage::Note( + "Adding a PRIMARY KEY constraint results in locks and table rewrites".into(), + ), + ViolationMessage::Help( + "Add the PRIMARY KEY constraint USING an index.".into(), + ), + + ], + }, // see ChangingColumnType SquawkRule { name: RuleViolationKind::AddingFieldWithDefault, diff --git a/linter/src/rules/adding_primary_key_constraint.rs b/linter/src/rules/adding_primary_key_constraint.rs new file mode 100644 index 00000000..16b84896 --- /dev/null +++ b/linter/src/rules/adding_primary_key_constraint.rs @@ -0,0 +1,77 @@ +use crate::violations::{RuleViolation, RuleViolationKind}; +use squawk_parser::ast::{ + AlterTableCmds, AlterTableDef, ColumnDefConstraint, ConstrType, RootStmt, Stmt, +}; + +#[must_use] +pub fn adding_primary_key_constraint(tree: &[RootStmt]) -> Vec { + let mut errs = vec![]; + for RootStmt::RawStmt(raw_stmt) in tree { + match &raw_stmt.stmt { + Stmt::AlterTableStmt(stmt) => { + for AlterTableCmds::AlterTableCmd(cmd) in &stmt.cmds { + match &cmd.def { + Some(AlterTableDef::ColumnDef(def)) => { + for ColumnDefConstraint::Constraint(constraint) in &def.constraints { + if constraint.contype == ConstrType::Primary { + errs.push(RuleViolation::new( + RuleViolationKind::AddingSerialPrimaryKeyField, + raw_stmt, + None, + )); + } + } + } + Some(AlterTableDef::Constraint(constraint)) => { + if constraint.contype == ConstrType::Primary { + errs.push(RuleViolation::new( + RuleViolationKind::AddingSerialPrimaryKeyField, + raw_stmt, + None, + )); + } + } + _ => continue, + } + } + } + _ => continue, + } + } + errs +} + +#[cfg(test)] +mod test_rules { + use crate::check_sql; + use insta::assert_debug_snapshot; + + #[test] + fn test_serial_primary_key() { + let bad_sql = r#" +ALTER TABLE a ADD COLUMN b SERIAL PRIMARY KEY; +"#; + + let expected_bad_res = + check_sql(bad_sql, &["prefer-robust-stmts".into()]).unwrap_or_default(); + assert_ne!(expected_bad_res, vec![]); + assert_debug_snapshot!(expected_bad_res); + } + + #[test] + fn test_plain_primary_key() { + let bad_sql = r#" +ALTER TABLE items ADD PRIMARY KEY (id); +"#; + + let expected_bad_res = + check_sql(bad_sql, &["prefer-robust-stmts".into()]).unwrap_or_default(); + assert_ne!(expected_bad_res, vec![]); + assert_debug_snapshot!(expected_bad_res); + + let ok_sql = r#" +ALTER TABLE items ADD CONSTRAINT items_pk PRIMARY KEY USING INDEX items_pk; +"#; + assert_debug_snapshot!(check_sql(ok_sql, &["prefer-robust-stmts".into()])); + } +} diff --git a/linter/src/rules/mod.rs b/linter/src/rules/mod.rs index 90478969..ee5f8f44 100644 --- a/linter/src/rules/mod.rs +++ b/linter/src/rules/mod.rs @@ -2,6 +2,8 @@ pub mod adding_field_with_default; pub use adding_field_with_default::*; pub mod adding_not_null_field; pub use adding_not_null_field::*; +pub mod adding_primary_key_constraint; +pub use adding_primary_key_constraint::*; pub mod bad_drop_database; pub use bad_drop_database::*; pub mod changing_column_type; diff --git a/linter/src/rules/snapshots/squawk_linter__rules__adding_primary_key_constraint__test_rules__plain_primary_key-2.snap b/linter/src/rules/snapshots/squawk_linter__rules__adding_primary_key_constraint__test_rules__plain_primary_key-2.snap new file mode 100644 index 00000000..36805d26 --- /dev/null +++ b/linter/src/rules/snapshots/squawk_linter__rules__adding_primary_key_constraint__test_rules__plain_primary_key-2.snap @@ -0,0 +1,25 @@ +--- +source: linter/src/rules/adding_primary_key_constraint.rs +expression: "check_sql(ok_sql, &[\"prefer-robust-stmts\".into()])" +--- +Ok( + [ + RuleViolation { + kind: AddingSerialPrimaryKeyField, + span: Span { + start: 0, + len: Some( + 75, + ), + }, + messages: [ + Note( + "Adding a PRIMARY KEY constraint results in locks and table rewrites", + ), + Help( + "Add the PRIMARY KEY constraint USING an index.", + ), + ], + }, + ], +) diff --git a/linter/src/rules/snapshots/squawk_linter__rules__adding_primary_key_constraint__test_rules__plain_primary_key.snap b/linter/src/rules/snapshots/squawk_linter__rules__adding_primary_key_constraint__test_rules__plain_primary_key.snap new file mode 100644 index 00000000..37d7d79d --- /dev/null +++ b/linter/src/rules/snapshots/squawk_linter__rules__adding_primary_key_constraint__test_rules__plain_primary_key.snap @@ -0,0 +1,23 @@ +--- +source: linter/src/rules/adding_primary_key_constraint.rs +expression: expected_bad_res +--- +[ + RuleViolation { + kind: AddingSerialPrimaryKeyField, + span: Span { + start: 0, + len: Some( + 39, + ), + }, + messages: [ + Note( + "Adding a PRIMARY KEY constraint results in locks and table rewrites", + ), + Help( + "Add the PRIMARY KEY constraint USING an index.", + ), + ], + }, +] diff --git a/linter/src/rules/snapshots/squawk_linter__rules__adding_primary_key_constraint__test_rules__serial_primary_key.snap b/linter/src/rules/snapshots/squawk_linter__rules__adding_primary_key_constraint__test_rules__serial_primary_key.snap new file mode 100644 index 00000000..12c03b74 --- /dev/null +++ b/linter/src/rules/snapshots/squawk_linter__rules__adding_primary_key_constraint__test_rules__serial_primary_key.snap @@ -0,0 +1,23 @@ +--- +source: linter/src/rules/adding_primary_key_constraint.rs +expression: expected_bad_res +--- +[ + RuleViolation { + kind: AddingSerialPrimaryKeyField, + span: Span { + start: 0, + len: Some( + 46, + ), + }, + messages: [ + Note( + "Adding a PRIMARY KEY constraint results in locks and table rewrites", + ), + Help( + "Add the PRIMARY KEY constraint USING an index.", + ), + ], + }, +] diff --git a/linter/src/rules/snapshots/squawk_linter__rules__disallow_unique_constraint__test_rules__adding_unique_constraint-2.snap b/linter/src/rules/snapshots/squawk_linter__rules__disallow_unique_constraint__test_rules__adding_unique_constraint-2.snap index aa66adc3..b241fe4a 100644 --- a/linter/src/rules/snapshots/squawk_linter__rules__disallow_unique_constraint__test_rules__adding_unique_constraint-2.snap +++ b/linter/src/rules/snapshots/squawk_linter__rules__disallow_unique_constraint__test_rules__adding_unique_constraint-2.snap @@ -3,5 +3,23 @@ source: linter/src/rules/disallow_unique_constraint.rs expression: "check_sql(ok_sql, &[\"prefer-robust-stmts\".into()])" --- Ok( - [], + [ + RuleViolation { + kind: AddingSerialPrimaryKeyField, + span: Span { + start: 77, + len: Some( + 134, + ), + }, + messages: [ + Note( + "Adding a PRIMARY KEY constraint results in locks and table rewrites", + ), + Help( + "Add the PRIMARY KEY constraint USING an index.", + ), + ], + }, + ], ) diff --git a/linter/src/violations.rs b/linter/src/violations.rs index 64aacfdd..53e10fdf 100644 --- a/linter/src/violations.rs +++ b/linter/src/violations.rs @@ -9,6 +9,7 @@ pub enum RuleViolationKind { AddingFieldWithDefault, ChangingColumnType, AddingNotNullableField, + AddingSerialPrimaryKeyField, RenamingColumn, RenamingTable, DisallowedUniqueConstraint, @@ -29,6 +30,7 @@ impl std::fmt::Display for RuleViolationKind { Self::RenamingColumn => "renaming-column", Self::RenamingTable => "renaming-table", Self::DisallowedUniqueConstraint => "disallowed-unique-constraint", + Self::AddingSerialPrimaryKeyField => "adding-serial-primary-key-field", Self::BanDropDatabase => "ban-drop-database", Self::PreferTextField => "prefer-text-field", Self::PreferRobustStmts => "prefer-robust-stmts", @@ -47,6 +49,7 @@ impl std::convert::TryFrom<&str> for RuleViolationKind { "adding-field-with-default" => Ok(Self::AddingFieldWithDefault), "changing-column-type" => Ok(Self::ChangingColumnType), "adding-not-nullable-field" => Ok(Self::AddingNotNullableField), + "adding-serial-primary-key-field" => Ok(Self::AddingSerialPrimaryKeyField), "renaming-column" => Ok(Self::RenamingColumn), "renaming-table" => Ok(Self::RenamingTable), "disallowed-unique-constraint" => Ok(Self::DisallowedUniqueConstraint),