feat(rules): add PGM021 (ADD UNIQUE without USING INDEX) and PGM022 (DROP TABLE)

- PGM021 (Critical): flags ADD UNIQUE on existing table without prior unique index
- PGM022 (Minor): flags DROP TABLE on existing table for human review
- Add matrix interaction tests for PGM012/PGM021 interplay
- Update README with new rules, notes, and rule count (25 → 28)

Author: sj-robert

README.md

@@ -7,7 +7,7 @@ Static analyzer for PostgreSQL migration files.
 
 ## What it does
 
-pg-migration-lint replays your full migration history to build an internal table catalog, then lints only new or changed migration files against 25 safety and correctness rules. It catches dangerous operations -- missing `CONCURRENTLY`, table rewrites, missing indexes on foreign keys, unsafe constraint additions, silent constraint removal, risky renames, type anti-patterns -- before they reach production.
+pg-migration-lint replays your full migration history to build an internal table catalog, then lints only new or changed migration files against 28 safety and correctness rules. It catches dangerous operations -- missing `CONCURRENTLY`, table rewrites, missing indexes on foreign keys, unsafe constraint additions, silent constraint removal, risky renames, type anti-patterns -- before they reach production.
 
 Output formats include SARIF (for GitHub Code Scanning inline PR annotations), SonarQube Generic Issue Import JSON, and human-readable text.
 
@@ -36,7 +36,7 @@ chmod +x pg-migration-lint
 
 ## Rules
 
-pg-migration-lint ships with 25 rules across two categories: migration safety rules (PGM001-PGM020) and PostgreSQL type anti-pattern rules (PGM101-PGM105, PGM108).
+pg-migration-lint ships with 28 rules across two categories: migration safety rules (PGM001-PGM022) and PostgreSQL type anti-pattern rules (PGM101-PGM105, PGM108).
 
 ### Migration Safety Rules
 
@@ -62,6 +62,8 @@ pg-migration-lint ships with 25 rules across two categories: migration safety ru
 | PGM018 | Critical | `ADD CHECK` without `NOT VALID` | `ALTER TABLE orders ADD CONSTRAINT chk CHECK (amount > 0);` |
 | PGM019 | Info | `RENAME TABLE` on existing table | `ALTER TABLE orders RENAME TO orders_old;` |
 | PGM020 | Info | `RENAME COLUMN` on existing table | `ALTER TABLE orders RENAME COLUMN status TO order_status;` |
+| PGM021 | Critical | `ADD UNIQUE` without `USING INDEX` | `ALTER TABLE users ADD CONSTRAINT uq_email UNIQUE (email);` |
+| PGM022 | Minor | `DROP TABLE` on existing table | `DROP TABLE legacy_orders;` |
 
 PGM001 and PGM002 do not fire when the table is created in the same set of changed files, because locking a new/empty table is harmless.
 
@@ -71,6 +73,10 @@ PGM016, PGM017, and PGM018 only fire on tables that existed before the current s
 
 PGM019 includes replacement detection: if the old table name is re-created in the same file (a common rename-and-replace pattern), the finding is suppressed.
 
+PGM021 follows the same pattern as PGM012: create the unique index `CONCURRENTLY` first, then use `ADD CONSTRAINT ... UNIQUE USING INDEX` to promote it. This avoids a full table scan under an `ACCESS EXCLUSIVE` lock.
+
+PGM022 only fires on tables that existed before the current set of changed files. Dropping a table created in the same changeset is harmless.
+
 PGM008 is not a standalone rule -- it is a behavior modifier. All findings produced by other rules on `.down.sql` or rollback migrations are automatically capped to Info severity.
 
 ### PostgreSQL "Don't Do This" Rules

src/rules/mod.rs

@@ -27,6 +27,8 @@ pub mod pgm017;
 pub mod pgm018;
 pub mod pgm019;
 pub mod pgm020;
+pub mod pgm021;
+pub mod pgm022;
 pub mod pgm101;
 pub mod pgm102;
 pub mod pgm103;
@@ -228,6 +230,8 @@ impl RuleRegistry {
         self.register(Box::new(pgm018::Pgm018));
         self.register(Box::new(pgm019::Pgm019));
         self.register(Box::new(pgm020::Pgm020));
+        self.register(Box::new(pgm021::Pgm021));
+        self.register(Box::new(pgm022::Pgm022));
         self.register(Box::new(pgm101::Pgm101));
         self.register(Box::new(pgm102::Pgm102));
         self.register(Box::new(pgm103::Pgm103));
@@ -267,8 +271,8 @@ mod tests {
         let mut registry = RuleRegistry::new();
         registry.register_defaults();
 
-        // We should have 25 rules (PGM001-PGM007, PGM009-PGM020, PGM101-PGM105, PGM108; PGM008 is not a rule)
-        assert_eq!(registry.rules.len(), 25);
+        // We should have 27 rules (PGM001-PGM007, PGM009-PGM022, PGM101-PGM105, PGM108; PGM008 is not a rule)
+        assert_eq!(registry.rules.len(), 27);
     }
 
     #[test]

src/rules/pgm021.rs

@@ -0,0 +1,215 @@
+//! PGM021 — `ADD UNIQUE` on existing table without `USING INDEX`
+//!
+//! Detects `ALTER TABLE ... ADD CONSTRAINT ... UNIQUE (columns)` on tables that
+//! already exist where the target columns don't already have a covering unique
+//! index. The safe pattern is to first create a unique index CONCURRENTLY, then
+//! add the constraint using that index.
+
+use crate::parser::ir::{AlterTableAction, IrNode, Located, TableConstraint};
+use crate::rules::{Finding, LintContext, Rule, Severity, alter_table_check};
+
+/// Rule that flags adding a UNIQUE constraint to an existing table without a
+/// pre-existing unique index on the constraint columns.
+pub struct Pgm021;
+
+impl Rule for Pgm021 {
+    fn id(&self) -> &'static str {
+        "PGM021"
+    }
+
+    fn default_severity(&self) -> Severity {
+        Severity::Critical
+    }
+
+    fn description(&self) -> &'static str {
+        "ADD UNIQUE on existing table without USING INDEX"
+    }
+
+    fn explain(&self) -> &'static str {
+        "PGM021 — ADD UNIQUE on existing table without USING INDEX\n\
+         \n\
+         What it detects:\n\
+         ALTER TABLE ... ADD CONSTRAINT ... UNIQUE (columns) where the table\n\
+         already exists and the target columns don't have a pre-existing unique\n\
+         index.\n\
+         \n\
+         Why it's dangerous:\n\
+         Adding a UNIQUE constraint inline builds a unique index under an ACCESS\n\
+         EXCLUSIVE lock, blocking all reads and writes for the duration. For\n\
+         large tables this can cause extended downtime. Unlike CHECK and FOREIGN\n\
+         KEY constraints, NOT VALID does NOT apply to UNIQUE constraints, so\n\
+         there is no NOT VALID escape hatch.\n\
+         \n\
+         Example (bad):\n\
+           ALTER TABLE orders ADD CONSTRAINT uq_email UNIQUE (email);\n\
+         \n\
+         Fix (safe pattern — build unique index concurrently first):\n\
+           CREATE UNIQUE INDEX CONCURRENTLY idx_orders_email ON orders (email);\n\
+           ALTER TABLE orders ADD CONSTRAINT uq_email UNIQUE USING INDEX idx_orders_email;"
+    }
+
+    fn check(&self, statements: &[Located<IrNode>], ctx: &LintContext<'_>) -> Vec<Finding> {
+        alter_table_check::check_alter_actions(statements, ctx, |at, action, stmt, ctx| {
+            let AlterTableAction::AddConstraint(TableConstraint::Unique { columns, .. }) = action
+            else {
+                return vec![];
+            };
+
+            let table_key = at.name.catalog_key();
+            let Some(table) = ctx.catalog_before.get_table(table_key) else {
+                return vec![];
+            };
+
+            if table.has_unique_covering(columns) {
+                return vec![];
+            }
+
+            vec![self.make_finding(
+                format!(
+                    "ADD UNIQUE on existing table '{table}' without a \
+                     pre-existing unique index on column(s) [{columns}]. \
+                     Create a unique index CONCURRENTLY first, then use \
+                     ADD CONSTRAINT ... UNIQUE USING INDEX.",
+                    table = at.name.display_name(),
+                    columns = columns.join(", "),
+                ),
+                ctx.file,
+                &stmt.span,
+            )]
+        })
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use crate::catalog::Catalog;
+    use crate::catalog::builder::CatalogBuilder;
+    use crate::parser::ir::*;
+    use crate::rules::test_helpers::{located, make_ctx};
+    use std::collections::HashSet;
+    use std::path::PathBuf;
+
+    fn add_unique_stmt(table: &str, columns: &[&str]) -> Located<IrNode> {
+        located(IrNode::AlterTable(AlterTable {
+            name: QualifiedName::unqualified(table),
+            actions: vec![AlterTableAction::AddConstraint(TableConstraint::Unique {
+                name: Some(format!("uq_{}", columns.join("_"))),
+                columns: columns.iter().map(|s| s.to_string()).collect(),
+            })],
+        }))
+    }
+
+    #[test]
+    fn test_add_unique_no_existing_index_fires() {
+        let before = CatalogBuilder::new()
+            .table("orders", |t| {
+                t.column("id", "bigint", false)
+                    .column("email", "text", false);
+            })
+            .build();
+        let after = before.clone();
+        let file = PathBuf::from("migrations/002.sql");
+        let created = HashSet::new();
+        let ctx = make_ctx(&before, &after, &file, &created);
+
+        let stmts = vec![add_unique_stmt("orders", &["email"])];
+
+        let findings = Pgm021.check(&stmts, &ctx);
+        insta::assert_yaml_snapshot!(findings);
+    }
+
+    #[test]
+    fn test_add_unique_with_existing_unique_index_no_finding() {
+        let before = CatalogBuilder::new()
+            .table("orders", |t| {
+                t.column("id", "bigint", false)
+                    .column("email", "text", false)
+                    .index("idx_orders_email", &["email"], true);
+            })
+            .build();
+        let after = before.clone();
+        let file = PathBuf::from("migrations/002.sql");
+        let created = HashSet::new();
+        let ctx = make_ctx(&before, &after, &file, &created);
+
+        let stmts = vec![add_unique_stmt("orders", &["email"])];
+
+        let findings = Pgm021.check(&stmts, &ctx);
+        assert!(findings.is_empty());
+    }
+
+    #[test]
+    fn test_add_unique_on_new_table_no_finding() {
+        let before = Catalog::new();
+        let after = CatalogBuilder::new()
+            .table("orders", |t| {
+                t.column("id", "bigint", false)
+                    .column("email", "text", false);
+            })
+            .build();
+        let file = PathBuf::from("migrations/001.sql");
+        let mut created = HashSet::new();
+        created.insert("orders".to_string());
+        let ctx = make_ctx(&before, &after, &file, &created);
+
+        let stmts = vec![add_unique_stmt("orders", &["email"])];
+
+        let findings = Pgm021.check(&stmts, &ctx);
+        assert!(findings.is_empty());
+    }
+
+    #[test]
+    fn test_table_not_in_catalog_no_finding() {
+        let before = Catalog::new();
+        let after = before.clone();
+        let file = PathBuf::from("migrations/002.sql");
+        let created = HashSet::new();
+        let ctx = make_ctx(&before, &after, &file, &created);
+
+        let stmts = vec![add_unique_stmt("nonexistent", &["email"])];
+
+        let findings = Pgm021.check(&stmts, &ctx);
+        assert!(findings.is_empty());
+    }
+
+    #[test]
+    fn test_non_unique_index_still_fires() {
+        let before = CatalogBuilder::new()
+            .table("orders", |t| {
+                t.column("id", "bigint", false)
+                    .column("email", "text", false)
+                    .index("idx_orders_email", &["email"], false); // NOT unique
+            })
+            .build();
+        let after = before.clone();
+        let file = PathBuf::from("migrations/002.sql");
+        let created = HashSet::new();
+        let ctx = make_ctx(&before, &after, &file, &created);
+
+        let stmts = vec![add_unique_stmt("orders", &["email"])];
+
+        let findings = Pgm021.check(&stmts, &ctx);
+        insta::assert_yaml_snapshot!(findings);
+    }
+
+    #[test]
+    fn test_add_unique_with_existing_unique_constraint_no_finding() {
+        let before = CatalogBuilder::new()
+            .table("orders", |t| {
+                t.column("id", "bigint", false)
+                    .column("email", "text", false)
+                    .unique("uq_orders_email", &["email"]);
+            })
+            .build();
+        let after = before.clone();
+        let file = PathBuf::from("migrations/002.sql");
+        let created = HashSet::new();
+        let ctx = make_ctx(&before, &after, &file, &created);
+
+        let stmts = vec![add_unique_stmt("orders", &["email"])];
+
+        let findings = Pgm021.check(&stmts, &ctx);
+        assert!(findings.is_empty());
+    }
+}