progress

Author: psteinroe

PLAN.md

@@ -17,6 +17,15 @@ pub struct RuleContext<'a, R: Rule> {
     // the file context which contains other statements in that file in case you need them
     file_context: &'a AnalysedFileContext,
 }
+
+pub struct AnalysedFileContext<'a> {
+    // all other statements in this file
+    pub all_stmts: &'a Vec<pgt_query::NodeEnum>,
+    // total count of statements in this file
+    pub stmt_count: usize,
+    // all statements before the currently analysed one
+    pub previous_stmts: Vec<&'a pgt_query::NodeEnum>,
+}
 ```
 
 In squawk, you will see:
@@ -41,27 +50,23 @@ LEARNINGS:
 - RuleDiagnostic methods: `detail(span, msg)` takes two parameters, `note(msg)` takes only one parameter
 - To check Postgres version: access `ctx.schema_cache().is_some_and(|sc| sc.version.major_version)` which gives e.g. 17
 - NEVER skip anything, or use a subset of something. ALWAYS do the full thing. For example, copy the entire non-volatile functions list from Squawk, not just a subset.
+- If you are missing features from our context to be able to properly implement a rule, DO NOT DO IT. Instead, add that rule to the NEEDS FEATURES list below.
 - Remember to run `just gen-lint` after creating a new rule to generate all necessary files
 
 Please update the list below with the rules that we need to migrate, and the ones that are already migrated. Keep the list up-to-date.
 
+NEEDS FEATURES:
+
 TODO:
-- ban_concurrent_index_creation_in_transaction
-- changing_column_type
-- constraint_missing_not_valid
-- disallow_unique_constraint
-- prefer_big_int
-- prefer_bigint_over_int
-- prefer_bigint_over_smallint
-- prefer_identity
-- prefer_robust_stmts
 - prefer_text_field
 - prefer_timestamptz
 - renaming_column
 - renaming_table
 - require_concurrent_index_creation
 - require_concurrent_index_deletion
 - transaction_nesting
+- disallow_unique_constraint
+- prefer_robust_stmts
 
 DONE:
 - adding_field_with_default ✓ (ported from Squawk)
@@ -70,9 +75,16 @@ DONE:
 - adding_primary_key_constraint ✓ (ported from Squawk)
 - adding_required_field (already exists in pgt_analyser)
 - ban_char_field ✓ (ported from Squawk)
+- ban_concurrent_index_creation_in_transaction ✓ (ported from Squawk)
 - ban_drop_column (already exists in pgt_analyser)
+- changing_column_type ✓ (ported from Squawk)
+- constraint_missing_not_valid ✓ (ported from Squawk)
 - ban_drop_database (already exists in pgt_analyser, as bad_drop_database in squawk)
 - ban_drop_not_null (already exists in pgt_analyser)
 - ban_drop_table (already exists in pgt_analyser)
+- prefer_big_int ✓ (ported from Squawk)
+- prefer_bigint_over_int ✓ (ported from Squawk)
+- prefer_bigint_over_smallint ✓ (ported from Squawk)
+- prefer_identity ✓ (ported from Squawk)
 
 

crates/pgt_analyse/src/analysed_file_context.rs

@@ -1,7 +1,19 @@
-#[derive(Default)]
-pub struct AnalysedFileContext {}
+pub struct AnalysedFileContext<'a> {
+    pub all_stmts: &'a Vec<pgt_query::NodeEnum>,
+    pub stmt_count: usize,
+    pub previous_stmts: Vec<&'a pgt_query::NodeEnum>,
+}
+
+impl<'a> AnalysedFileContext<'a> {
+    pub fn new(stmts: &'a Vec<pgt_query::NodeEnum>) -> Self {
+        Self {
+            all_stmts: stmts,
+            stmt_count: stmts.len(),
+            previous_stmts: Vec::new(),
+        }
+    }
 
-impl AnalysedFileContext {
-    #[allow(unused)]
-    pub fn update_from(&mut self, stmt_root: &pgt_query::NodeEnum) {}
+    pub fn update_from(&mut self, stmt_root: &'a pgt_query::NodeEnum) {
+        self.previous_stmts.push(stmt_root);
+    }
 }

crates/pgt_analyse/src/context.rs

@@ -10,7 +10,7 @@ pub struct RuleContext<'a, R: Rule> {
     stmt: &'a pgt_query::NodeEnum,
     options: &'a R::Options,
     schema_cache: Option<&'a SchemaCache>,
-    file_context: &'a AnalysedFileContext,
+    file_context: &'a AnalysedFileContext<'a>,
 }
 
 impl<'a, R> RuleContext<'a, R>

crates/pgt_analyse/src/registry.rs

@@ -159,7 +159,7 @@ impl RuleRegistry {
 pub struct RegistryRuleParams<'a> {
     pub root: &'a pgt_query::NodeEnum,
     pub options: &'a AnalyserOptions,
-    pub analysed_file_context: &'a AnalysedFileContext,
+    pub analysed_file_context: &'a AnalysedFileContext<'a>,
     pub schema_cache: Option<&'a pgt_schema_cache::SchemaCache>,
 }
 

crates/pgt_analyser/src/lib.rs

@@ -62,25 +62,29 @@ impl<'a> Analyser<'a> {
     pub fn run(&self, params: AnalyserParams) -> Vec<RuleDiagnostic> {
         let mut diagnostics = vec![];
 
-        let mut file_context = AnalysedFileContext::default();
+        let roots: Vec<pgt_query::NodeEnum> = params.stmts.iter().map(|s| s.root.clone()).collect();
+        let mut file_context = AnalysedFileContext::new(&roots);
+
+        for (i, stmt) in params.stmts.into_iter().enumerate() {
+            let stmt_diagnostics: Vec<_> = {
+                let rule_params = RegistryRuleParams {
+                    root: &roots[i],
+                    options: self.options,
+                    analysed_file_context: &file_context,
+                    schema_cache: params.schema_cache,
+                };
 
-        for stmt in params.stmts {
-            let rule_params = RegistryRuleParams {
-                root: &stmt.root,
-                options: self.options,
-                analysed_file_context: &file_context,
-                schema_cache: params.schema_cache,
-            };
-
-            diagnostics.extend(
                 self.registry
                     .rules
                     .iter()
                     .flat_map(|rule| (rule.run)(&rule_params))
-                    .map(|r| r.span(stmt.range)),
-            );
+                    .map(|r| r.span(stmt.range))
+                    .collect()
+            }; // end immutable borrow
+
+            diagnostics.extend(stmt_diagnostics);
 
-            file_context.update_from(&stmt.root);
+            file_context.update_from(&roots[i]);
         }
 
         diagnostics

crates/pgt_analyser/src/lint/safety.rs

@@ -7,9 +7,16 @@ pub mod adding_not_null_field;
 pub mod adding_primary_key_constraint;
 pub mod adding_required_field;
 pub mod ban_char_field;
+pub mod ban_concurrent_index_creation_in_transaction;
 pub mod ban_drop_column;
 pub mod ban_drop_database;
 pub mod ban_drop_not_null;
 pub mod ban_drop_table;
 pub mod ban_truncate_cascade;
-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_drop_column :: BanDropColumn , self :: ban_drop_database :: BanDropDatabase , self :: ban_drop_not_null :: BanDropNotNull , self :: ban_drop_table :: BanDropTable , self :: ban_truncate_cascade :: BanTruncateCascade ,] } }
+pub mod changing_column_type;
+pub mod constraint_missing_not_valid;
+pub mod prefer_big_int;
+pub mod prefer_bigint_over_int;
+pub mod prefer_bigint_over_smallint;
+pub mod prefer_identity;
+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 :: prefer_big_int :: PreferBigInt , self :: prefer_bigint_over_int :: PreferBigintOverInt , self :: prefer_bigint_over_smallint :: PreferBigintOverSmallint , self :: prefer_identity :: PreferIdentity , ] } }

crates/pgt_analyser/src/lint/safety/ban_concurrent_index_creation_in_transaction.rs

@@ -0,0 +1,53 @@
+use pgt_analyse::{Rule, RuleDiagnostic, RuleSource, context::RuleContext, declare_lint_rule};
+use pgt_console::markup;
+use pgt_diagnostics::Severity;
+
+declare_lint_rule! {
+    /// Concurrent index creation is not allowed within a transaction.
+    ///
+    /// `CREATE INDEX CONCURRENTLY` cannot be used within a transaction block. This will cause an error in Postgres.
+    ///
+    /// Migration tools usually run each migration in a transaction, so using `CREATE INDEX CONCURRENTLY` will fail in such tools.
+    ///
+    /// ## Examples
+    ///
+    /// ### Invalid
+    ///
+    /// ```sql,expect_diagnostic
+    /// CREATE INDEX CONCURRENTLY "field_name_idx" ON "table_name" ("field_name");
+    /// ```
+    ///
+    pub BanConcurrentIndexCreationInTransaction {
+        version: "next",
+        name: "banConcurrentIndexCreationInTransaction",
+        severity: Severity::Error,
+        recommended: true,
+        sources: &[RuleSource::Squawk("ban-concurrent-index-creation-in-transaction")],
+    }
+}
+
+impl Rule for BanConcurrentIndexCreationInTransaction {
+    type Options = ();
+
+    fn run(ctx: &RuleContext<Self>) -> Vec<RuleDiagnostic> {
+        let mut diagnostics = Vec::new();
+
+        // check if the current statement is CREATE INDEX CONCURRENTLY and there is at least one
+        // other statement in the same context (indicating a transaction block)
+        //
+        // since our analyser assumes we're always in a transaction context, we always flag concurrent indexes
+        if let pgt_query::NodeEnum::IndexStmt(stmt) = ctx.stmt() {
+            if stmt.concurrent && ctx.file_context().stmt_count > 1 {
+                diagnostics.push(RuleDiagnostic::new(
+                    rule_category!(),
+                    None,
+                    markup! {
+                        "CREATE INDEX CONCURRENTLY cannot be used inside a transaction block."
+                    }
+                ).detail(None, "Run CREATE INDEX CONCURRENTLY outside of a transaction. Migration tools usually run in transactions, so you may need to run this statement in its own migration or manually."));
+            }
+        }
+
+        diagnostics
+    }
+}

crates/pgt_analyser/src/lint/safety/changing_column_type.rs

@@ -0,0 +1,55 @@
+use pgt_analyse::{Rule, RuleDiagnostic, RuleSource, context::RuleContext, declare_lint_rule};
+use pgt_console::markup;
+use pgt_diagnostics::Severity;
+
+declare_lint_rule! {
+    /// Changing a column type may break existing clients.
+    ///
+    /// Changing a column's data type requires an exclusive lock on the table while the entire table is rewritten.
+    /// This can take a long time for large tables and will block reads and writes.
+    ///
+    /// Instead of changing the type directly, consider creating a new column with the desired type,
+    /// migrating the data, and then dropping the old column.
+    ///
+    /// ## Examples
+    ///
+    /// ### Invalid
+    ///
+    /// ```sql,expect_diagnostic
+    /// ALTER TABLE "core_recipe" ALTER COLUMN "edits" TYPE text USING "edits"::text;
+    /// ```
+    ///
+    pub ChangingColumnType {
+        version: "next",
+        name: "changingColumnType",
+        severity: Severity::Warning,
+        recommended: false,
+        sources: &[RuleSource::Squawk("changing-column-type")],
+    }
+}
+
+impl Rule for ChangingColumnType {
+    type Options = ();
+
+    fn run(ctx: &RuleContext<Self>) -> Vec<RuleDiagnostic> {
+        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::AtAlterColumnType {
+                        diagnostics.push(RuleDiagnostic::new(
+                            rule_category!(),
+                            None,
+                            markup! {
+                                "Changing a column type requires a table rewrite and blocks reads and writes."
+                            }
+                        ).detail(None, "Consider creating a new column with the desired type, migrating data, and then dropping the old column."));
+                    }
+                }
+            }
+        }
+
+        diagnostics
+    }
+}

crates/pgt_analyser/src/lint/safety/constraint_missing_not_valid.rs

@@ -0,0 +1,74 @@
+use pgt_analyse::{Rule, RuleDiagnostic, RuleSource, context::RuleContext, declare_lint_rule};
+use pgt_console::markup;
+use pgt_diagnostics::Severity;
+
+declare_lint_rule! {
+    /// Adding constraints without NOT VALID blocks all reads and writes.
+    ///
+    /// When adding a CHECK or FOREIGN KEY constraint, PostgreSQL must validate all existing rows,
+    /// which requires a full table scan. This blocks reads and writes for the duration.
+    ///
+    /// Instead, add the constraint with NOT VALID first, then VALIDATE CONSTRAINT in a separate
+    /// transaction. This allows reads and writes to continue while validation happens.
+    ///
+    /// ## Examples
+    ///
+    /// ### Invalid
+    ///
+    /// ```sql,expect_diagnostic
+    /// ALTER TABLE distributors ADD CONSTRAINT distfk FOREIGN KEY (address) REFERENCES addresses (address);
+    /// ```
+    ///
+    /// ### Valid
+    ///
+    /// ```sql
+    /// ALTER TABLE distributors ADD CONSTRAINT distfk FOREIGN KEY (address) REFERENCES addresses (address) NOT VALID;
+    /// ```
+    ///
+    pub ConstraintMissingNotValid {
+        version: "next",
+        name: "constraintMissingNotValid",
+        severity: Severity::Warning,
+        recommended: false,
+        sources: &[RuleSource::Squawk("constraint-missing-not-valid")],
+    }
+}
+
+impl Rule for ConstraintMissingNotValid {
+    type Options = ();
+
+    fn run(ctx: &RuleContext<Self>) -> Vec<RuleDiagnostic> {
+        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 {
+                    // Check if we're adding a constraint
+                    if let Some(pgt_query::NodeEnum::Constraint(constraint)) =
+                        cmd.def.as_ref().and_then(|d| d.node.as_ref())
+                    {
+                        // Skip if the constraint has NOT VALID
+                        if constraint.initially_valid {
+                            // Only warn for CHECK and FOREIGN KEY constraints
+                            match constraint.contype() {
+                                pgt_query::protobuf::ConstrType::ConstrCheck
+                                | pgt_query::protobuf::ConstrType::ConstrForeign => {
+                                    diagnostics.push(RuleDiagnostic::new(
+                                        rule_category!(),
+                                        None,
+                                        markup! {
+                                            "Adding a constraint without NOT VALID will block reads and writes while validating existing rows."
+                                        }
+                                    ).detail(None, "Add the constraint as NOT VALID in one transaction, then run VALIDATE CONSTRAINT in a separate transaction."));
+                                }
+                                _ => {}
+                            }
+                        }
+                    }
+                }
+            }
+        }
+
+        diagnostics
+    }
+}