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: #60

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;
+```
+
+<https://www.postgresql.org/docs/current/sql-altertable.html>
+
 ## Bot Setup
 
 Squawk works as a CLI tool but can also create comments on GitHub Pull

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,

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<RuleViolation> {
+    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()]));
+    }
+}

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;

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.",
+                ),
+            ],
+        },
+    ],
+)

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.",
+            ),
+        ],
+    },
+]

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.",
+            ),
+        ],
+    },
+]

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.",
+                ),
+            ],
+        },
+    ],
 )

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),