progress

Author: psteinroe

PLAN.md

@@ -58,12 +58,6 @@ Please update the list below with the rules that we need to migrate, and the one
 NEEDS FEATURES:
 
 TODO:
-- renaming_column
-- renaming_table
-- require_concurrent_index_creation
-- require_concurrent_index_deletion
-- transaction_nesting
-- prefer_robust_stmts
 
 DONE:
 - adding_field_with_default ✓ (ported from Squawk)
@@ -86,5 +80,11 @@ DONE:
 - prefer_text_field ✓ (ported from Squawk)
 - prefer_timestamptz ✓ (ported from Squawk)
 - disallow_unique_constraint ✓ (ported from Squawk)
+- renaming_column ✓ (ported from Squawk)
+- renaming_table ✓ (ported from Squawk)
+- require_concurrent_index_creation ✓ (ported from Squawk)
+- require_concurrent_index_deletion ✓ (ported from Squawk)
+- transaction_nesting ✓ (ported from Squawk)
+- prefer_robust_stmts ✓ (ported from Squawk - simplified version)
 
 

crates/pgt_analyser/src/lint/safety.rs

@@ -20,6 +20,12 @@ pub mod prefer_big_int;
 pub mod prefer_bigint_over_int;
 pub mod prefer_bigint_over_smallint;
 pub mod prefer_identity;
+pub mod prefer_robust_stmts;
 pub mod prefer_text_field;
 pub mod prefer_timestamptz;
-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_text_field :: PreferTextField , self :: prefer_timestamptz :: PreferTimestamptz ,] } }
+pub mod renaming_column;
+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_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 ,] } }

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

@@ -0,0 +1,111 @@
+use pgt_analyse::{Rule, RuleDiagnostic, RuleSource, context::RuleContext, declare_lint_rule};
+use pgt_console::markup;
+use pgt_diagnostics::Severity;
+
+declare_lint_rule! {
+    /// Prefer statements with guards for robustness in migrations.
+    ///
+    /// When running migrations outside of transactions (e.g., CREATE INDEX CONCURRENTLY),
+    /// statements should be made robust by using guards like IF NOT EXISTS or IF EXISTS.
+    /// This allows migrations to be safely re-run if they fail partway through.
+    ///
+    /// ## Examples
+    ///
+    /// ### Invalid
+    ///
+    /// ```sql,expect_diagnostic
+    /// CREATE INDEX CONCURRENTLY users_email_idx ON users (email);
+    /// ```
+    ///
+    /// ```sql,expect_diagnostic
+    /// DROP INDEX CONCURRENTLY users_email_idx;
+    /// ```
+    ///
+    /// ### Valid
+    ///
+    /// ```sql
+    /// CREATE INDEX CONCURRENTLY IF NOT EXISTS users_email_idx ON users (email);
+    /// ```
+    ///
+    /// ```sql
+    /// DROP INDEX CONCURRENTLY IF EXISTS users_email_idx;
+    /// ```
+    ///
+    pub PreferRobustStmts {
+        version: "next",
+        name: "preferRobustStmts",
+        severity: Severity::Warning,
+        recommended: false,
+        sources: &[RuleSource::Squawk("prefer-robust-stmts")],
+    }
+}
+
+impl Rule for PreferRobustStmts {
+    type Options = ();
+
+    fn run(ctx: &RuleContext<Self>) -> Vec<RuleDiagnostic> {
+        let mut diagnostics = Vec::new();
+
+        // Skip if we only have one statement in the file
+        if ctx.file_context().stmt_count <= 1 {
+            return diagnostics;
+        }
+
+        // Since we assume we're always in a transaction, we only check for
+        // statements that explicitly run outside transactions
+        match &ctx.stmt() {
+            pgt_query::NodeEnum::IndexStmt(stmt) => {
+                // Concurrent index creation runs outside transaction
+                if stmt.concurrent {
+                    // Check for unnamed index
+                    if stmt.idxname.is_empty() {
+                        diagnostics.push(RuleDiagnostic::new(
+                            rule_category!(),
+                            None,
+                            markup! {
+                                "Concurrent index should have an explicit name."
+                            },
+                        ).detail(None, "Use an explicit name for a concurrently created index to make migrations more robust."));
+                    }
+                    // Check for IF NOT EXISTS
+                    if !stmt.if_not_exists {
+                        diagnostics.push(
+                            RuleDiagnostic::new(
+                                rule_category!(),
+                                None,
+                                markup! {
+                                    "Concurrent index creation should use IF NOT EXISTS."
+                                },
+                            )
+                            .detail(
+                                None,
+                                "Add IF NOT EXISTS to make the migration re-runnable if it fails.",
+                            ),
+                        );
+                    }
+                }
+            }
+            pgt_query::NodeEnum::DropStmt(stmt) => {
+                // Concurrent drop runs outside transaction
+                if stmt.concurrent && !stmt.missing_ok {
+                    diagnostics.push(
+                        RuleDiagnostic::new(
+                            rule_category!(),
+                            None,
+                            markup! {
+                                "Concurrent drop should use IF EXISTS."
+                            },
+                        )
+                        .detail(
+                            None,
+                            "Add IF EXISTS to make the migration re-runnable if it fails.",
+                        ),
+                    );
+                }
+            }
+            _ => {}
+        }
+
+        diagnostics
+    }
+}

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

@@ -0,0 +1,49 @@
+use pgt_analyse::{Rule, RuleDiagnostic, RuleSource, context::RuleContext, declare_lint_rule};
+use pgt_console::markup;
+use pgt_diagnostics::Severity;
+
+declare_lint_rule! {
+    /// Renaming columns may break existing queries and application code.
+    ///
+    /// Renaming a column that is being used by an existing application or query can cause unexpected downtime.
+    /// Consider creating a new column instead and migrating the data, then dropping the old column after ensuring
+    /// no dependencies exist.
+    ///
+    /// ## Examples
+    ///
+    /// ### Invalid
+    ///
+    /// ```sql,expect_diagnostic
+    /// ALTER TABLE users RENAME COLUMN email TO email_address;
+    /// ```
+    ///
+    pub RenamingColumn {
+        version: "next",
+        name: "renamingColumn",
+        severity: Severity::Warning,
+        recommended: false,
+        sources: &[RuleSource::Squawk("renaming-column")],
+    }
+}
+
+impl Rule for RenamingColumn {
+    type Options = ();
+
+    fn run(ctx: &RuleContext<Self>) -> Vec<RuleDiagnostic> {
+        let mut diagnostics = Vec::new();
+
+        if let pgt_query::NodeEnum::RenameStmt(stmt) = &ctx.stmt() {
+            if stmt.rename_type() == pgt_query::protobuf::ObjectType::ObjectColumn {
+                diagnostics.push(RuleDiagnostic::new(
+                    rule_category!(),
+                    None,
+                    markup! {
+                        "Renaming a column may break existing clients."
+                    },
+                ).detail(None, "Consider creating a new column with the desired name and migrating data instead."));
+            }
+        }
+
+        diagnostics
+    }
+}

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

@@ -0,0 +1,49 @@
+use pgt_analyse::{Rule, RuleDiagnostic, RuleSource, context::RuleContext, declare_lint_rule};
+use pgt_console::markup;
+use pgt_diagnostics::Severity;
+
+declare_lint_rule! {
+    /// Renaming tables may break existing queries and application code.
+    ///
+    /// Renaming a table that is being referenced by existing applications, views, functions, or foreign keys
+    /// can cause unexpected downtime. Consider creating a view with the old table name pointing to the new table,
+    /// or carefully coordinate the rename with application deployments.
+    ///
+    /// ## Examples
+    ///
+    /// ### Invalid
+    ///
+    /// ```sql,expect_diagnostic
+    /// ALTER TABLE users RENAME TO app_users;
+    /// ```
+    ///
+    pub RenamingTable {
+        version: "next",
+        name: "renamingTable",
+        severity: Severity::Warning,
+        recommended: false,
+        sources: &[RuleSource::Squawk("renaming-table")],
+    }
+}
+
+impl Rule for RenamingTable {
+    type Options = ();
+
+    fn run(ctx: &RuleContext<Self>) -> Vec<RuleDiagnostic> {
+        let mut diagnostics = Vec::new();
+
+        if let pgt_query::NodeEnum::RenameStmt(stmt) = &ctx.stmt() {
+            if stmt.rename_type() == pgt_query::protobuf::ObjectType::ObjectTable {
+                diagnostics.push(RuleDiagnostic::new(
+                    rule_category!(),
+                    None,
+                    markup! {
+                        "Renaming a table may break existing clients."
+                    },
+                ).detail(None, "Consider creating a view with the old table name instead, or coordinate the rename carefully with application deployments."));
+            }
+        }
+
+        diagnostics
+    }
+}

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

@@ -0,0 +1,83 @@
+use pgt_analyse::{Rule, RuleDiagnostic, RuleSource, context::RuleContext, declare_lint_rule};
+use pgt_console::markup;
+use pgt_diagnostics::Severity;
+
+declare_lint_rule! {
+    /// Creating indexes non-concurrently can lock the table for writes.
+    ///
+    /// When creating an index on an existing table, using CREATE INDEX without CONCURRENTLY will lock the table
+    /// against writes for the duration of the index build. This can cause downtime in production systems.
+    /// Use CREATE INDEX CONCURRENTLY to build the index without blocking concurrent operations.
+    ///
+    /// ## Examples
+    ///
+    /// ### Invalid
+    ///
+    /// ```sql,expect_diagnostic
+    /// CREATE INDEX users_email_idx ON users (email);
+    /// ```
+    ///
+    /// ### Valid
+    ///
+    /// ```sql
+    /// CREATE INDEX CONCURRENTLY users_email_idx ON users (email);
+    /// ```
+    ///
+    pub RequireConcurrentIndexCreation {
+        version: "next",
+        name: "requireConcurrentIndexCreation",
+        severity: Severity::Warning,
+        recommended: false,
+        sources: &[RuleSource::Squawk("require-concurrent-index-creation")],
+    }
+}
+
+impl Rule for RequireConcurrentIndexCreation {
+    type Options = ();
+
+    fn run(ctx: &RuleContext<Self>) -> Vec<RuleDiagnostic> {
+        let mut diagnostics = Vec::new();
+
+        if let pgt_query::NodeEnum::IndexStmt(stmt) = &ctx.stmt() {
+            if !stmt.concurrent {
+                // Check if this table was created in the same transaction/file
+                let table_name = stmt
+                    .relation
+                    .as_ref()
+                    .map(|r| r.relname.as_str())
+                    .unwrap_or("");
+
+                if !table_name.is_empty()
+                    && !is_table_created_in_file(ctx.file_context(), table_name)
+                {
+                    diagnostics.push(RuleDiagnostic::new(
+                        rule_category!(),
+                        None,
+                        markup! {
+                            "Creating an index non-concurrently blocks writes to the table."
+                        },
+                    ).detail(None, "Use CREATE INDEX CONCURRENTLY to avoid blocking concurrent operations on the table."));
+                }
+            }
+        }
+
+        diagnostics
+    }
+}
+
+fn is_table_created_in_file(
+    file_context: &pgt_analyse::AnalysedFileContext,
+    table_name: &str,
+) -> bool {
+    // Check all statements in the file to see if this table was created
+    for stmt in file_context.all_stmts {
+        if let pgt_query::NodeEnum::CreateStmt(create_stmt) = stmt {
+            if let Some(relation) = &create_stmt.relation {
+                if relation.relname == table_name {
+                    return true;
+                }
+            }
+        }
+    }
+    false
+}

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

@@ -0,0 +1,57 @@
+use pgt_analyse::{Rule, RuleDiagnostic, RuleSource, context::RuleContext, declare_lint_rule};
+use pgt_console::markup;
+use pgt_diagnostics::Severity;
+
+declare_lint_rule! {
+    /// Dropping indexes non-concurrently can lock the table for reads.
+    ///
+    /// When dropping an index, using DROP INDEX without CONCURRENTLY will lock the table
+    /// preventing reads and writes for the duration of the drop. This can cause downtime in production systems.
+    /// Use DROP INDEX CONCURRENTLY to drop the index without blocking concurrent operations.
+    ///
+    /// ## Examples
+    ///
+    /// ### Invalid
+    ///
+    /// ```sql,expect_diagnostic
+    /// DROP INDEX IF EXISTS users_email_idx;
+    /// ```
+    ///
+    /// ### Valid
+    ///
+    /// ```sql
+    /// DROP INDEX CONCURRENTLY IF EXISTS users_email_idx;
+    /// ```
+    ///
+    pub RequireConcurrentIndexDeletion {
+        version: "next",
+        name: "requireConcurrentIndexDeletion",
+        severity: Severity::Warning,
+        recommended: false,
+        sources: &[RuleSource::Squawk("require-concurrent-index-deletion")],
+    }
+}
+
+impl Rule for RequireConcurrentIndexDeletion {
+    type Options = ();
+
+    fn run(ctx: &RuleContext<Self>) -> Vec<RuleDiagnostic> {
+        let mut diagnostics = Vec::new();
+
+        if let pgt_query::NodeEnum::DropStmt(stmt) = &ctx.stmt() {
+            if !stmt.concurrent
+                && stmt.remove_type() == pgt_query::protobuf::ObjectType::ObjectIndex
+            {
+                diagnostics.push(RuleDiagnostic::new(
+                    rule_category!(),
+                    None,
+                    markup! {
+                        "Dropping an index non-concurrently blocks reads and writes to the table."
+                    },
+                ).detail(None, "Use DROP INDEX CONCURRENTLY to avoid blocking concurrent operations on the table."));
+            }
+        }
+
+        diagnostics
+    }
+}