Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
103 changes: 103 additions & 0 deletions PLAN.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,103 @@
# pgfence Gap: Remaining Rules & Infrastructure

Remaining gaps after the initial 11-rule PR. Based on source review of [pgfence](https://github.com/flvmnt/pgfence).

## Gaps

### 1. New Statement Rules (trivial)

#### 1a. `BanEnableDisableTrigger`
- **Match:** `AlterTableStmt` → `AlterTableCmd` with `subtype()` in `{AtEnableTrig, AtDisableTrig, AtEnableTrigAll, AtDisableTrigAll, AtEnableTrigUser, AtDisableTrigUser}`
- **Severity:** Warning, not recommended (same tier as `banCreateTrigger`)
- **Lock:** SHARE ROW EXCLUSIVE
- **pgfence ref:** `trigger.ts:48-62`, rule `enable-disable-trigger`

#### 1b. `BanUpdateWithoutWhere`
- **Match:** `UpdateStmt` where `where_clause.is_none()`
- **Severity:** Warning, recommended (same as `banDeleteWithoutWhere`)
- **pgfence ref:** `destructive.ts:83-99`, rule `update-in-migration`

#### 1c. `BanAddExclusionConstraint`
- **Match:** `AlterTableStmt` → `AlterTableCmd(AtAddConstraint)` → `Constraint` where `contype() == ConstrExclusion`
- **Severity:** Warning, recommended (no concurrent alternative exists — only mitigation is `lock_timeout`)
- **pgfence ref:** `add-constraint.ts:100-116`, rule `add-constraint-exclude`
- **Note:** Also check `CreateStmt` → `constraints` for exclusion constraints in `CREATE TABLE`

#### 1d. `WarnRefreshMatviewConcurrent`
- **Match:** `RefreshMatViewStmt` where `concurrent == true`
- **Severity:** Warning, not recommended (still takes EXCLUSIVE lock blocking DDL, but reads work)
- **pgfence ref:** `refresh-matview.ts:24-36`, rule `refresh-matview-concurrent`
- **Note:** Informational — concurrent refresh still blocks writes. Separate from `banBlockingRefreshMatview`.

### 2. Policy / Cross-Statement Rules (medium)

These require extending `TransactionState` in `linter_context.rs`.

#### 2a. `RequireStatementTimeout`
- **Condition:** File contains a dangerous lock statement (ALTER TABLE, CREATE INDEX non-concurrent) but no `SET statement_timeout` or `SET LOCAL statement_timeout` before it.
- **Pattern:** Same as existing `lockTimeoutWarning` — extend `TransactionState` with `has_statement_timeout()`.
- **Severity:** Warning, not recommended (opt-in policy)
- **pgfence ref:** `policy.ts:76-94`, rule `missing-statement-timeout`

#### 2b. `RequireIdleInTransactionTimeout`
- **Condition:** File contains dangerous statements but no `SET idle_in_transaction_session_timeout`.
- **Severity:** Warning, not recommended
- **pgfence ref:** `policy.ts:113-127`, rule `missing-idle-timeout`

#### 2c. `BanNotValidValidateSameTransaction`
- **Condition:** `ALTER TABLE ... ADD CONSTRAINT ... NOT VALID` followed by `ALTER TABLE ... VALIDATE CONSTRAINT` on the same constraint within the same file/transaction.
- **Detection:**
1. On `AtAddConstraint` with `Constraint.skip_validation == true` → record constraint name in `TransactionState`
2. On `AtValidateConstraint` → check if constraint name was recorded → emit diagnostic
3. Reset on `COMMIT`/`ROLLBACK` (already tracked by `TransactionState`)
- **Severity:** Error, recommended (defeats the purpose of NOT VALID)
- **pgfence ref:** `policy.ts:134-172`, rule `not-valid-validate-same-tx`
- **Impl:** Extend `TransactionState` with `not_valid_constraints: HashSet<String>`. The constraint name comes from `AlterTableCmd.name` for validate, and from `Constraint.conname` for add.

#### 2d. `WarnWideLockWindow`
- **Condition:** Within a transaction, ACCESS EXCLUSIVE held on table A, then ACCESS EXCLUSIVE acquired on table B.
- **Detection:** Track `access_exclusive_tables: HashSet<String>` in `TransactionState`. On any statement that takes ACCESS EXCLUSIVE, check if set is non-empty with a different table.
- **Severity:** Warning, recommended
- **pgfence ref:** `policy.ts:174-203` + `transaction-state.ts:37-58`, rule `wide-lock-window`
- **Note:** Requires knowing which statements take ACCESS EXCLUSIVE. `AlterTableStmt` and non-concurrent `IndexStmt` are the main ones.

### 3. Infrastructure Improvements (larger)

#### 3a. `TransactionState` extensions
File: `crates/pgls_analyser/src/linter_context.rs`

Add fields:
- `statement_timeout_set: bool`
- `idle_in_transaction_timeout_set: bool`
- `not_valid_constraints: HashSet<String>` — constraint names added with NOT VALID in current tx
- `access_exclusive_tables: HashSet<(String, String)>` — (schema, table) pairs with ACCESS EXCLUSIVE in current tx

The existing `TransactionState` builder in `FileContext` already processes `VariableSetStmt` for `lock_timeout`. Extend it to also detect `statement_timeout` and `idle_in_transaction_session_timeout`. For NOT VALID tracking, process `AlterTableCmd` nodes as they're visited.

Check: `linter_context.rs` `FileContext::new()` — this is where `TransactionState` is built from `previous_stmts`. The constraint tracking needs to happen at statement-processing time, not just at context creation.

#### 3b. ALTER COLUMN TYPE widening (deferred)
pgfence distinguishes safe type changes (e.g., `varchar(N)` → `text`) from dangerous ones. Their approach: classify by **target type only** (source type not in the AST). Target `text` or `varchar` without length = LOW risk. Everything else = HIGH.

Our `changingColumnType` flags all type changes unconditionally. To match pgfence we'd need to inspect `ColumnDef.type_name` in the `AtAlterColumnType` command. This is doable but lower priority — our current rule is more conservative (fewer false negatives, more false positives).

**Defer** to a follow-up PR.

## Implementation Order

1. **1b, 1c** — `BanUpdateWithoutWhere`, `BanAddExclusionConstraint` (trivial, no infra changes)
2. **1a** — `BanEnableDisableTrigger` (trivial)
3. **3a** — `TransactionState` extensions (needed by 2a-2d)
4. **2c** — `BanNotValidValidateSameTransaction` (highest value cross-statement rule)
5. **2a, 2b** — `RequireStatementTimeout`, `RequireIdleInTransactionTimeout`
6. **2d** — `WarnWideLockWindow`
7. **1d** — `WarnRefreshMatviewConcurrent` (nice-to-have, informational)
8. **3b** — ALTER COLUMN TYPE widening (deferred)

## Out of Scope

- **DB stats risk adjustment** — pgfence adjusts risk based on row count from live DB. We do static analysis only.
- **ORM/framework SQL extraction** — pgfence extracts SQL from Prisma/Knex/Drizzle/etc. We handle raw SQL.
- **Plugin system** — pgfence supports custom rule plugins. Not needed.
- **Savepoint-level lock tracking** — pgfence tracks lock snapshots at savepoints. Our `TransactionState` resets on COMMIT/ROLLBACK but doesn't model savepoints. Low priority.
- **Cross-file table tracking** — pgfence tracks `CREATE TABLE` across files in a batch to skip false positives in later files. Our `lockTimeoutWarning` already does per-file tracking. Cross-file is a workspace-level concern, defer.
6 changes: 6 additions & 0 deletions crates/pgls_analyse/src/metadata.rs
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,8 @@ pub enum RuleSource {
Squawk(&'static str),
/// Rules from [Eugene](https://github.com/kaaveland/eugene)
Eugene(&'static str),
/// Rules from [pgfence](https://github.com/flvmnt/pgfence)
Pgfence(&'static str),
}

impl PartialEq for RuleSource {
Expand All @@ -109,6 +111,7 @@ impl std::fmt::Display for RuleSource {
match self {
Self::Squawk(_) => write!(f, "Squawk"),
Self::Eugene(_) => write!(f, "Eugene"),
Self::Pgfence(_) => write!(f, "pgfence"),
}
}
}
Expand All @@ -132,13 +135,15 @@ impl RuleSource {
match self {
Self::Squawk(rule_name) => rule_name,
Self::Eugene(rule_name) => rule_name,
Self::Pgfence(rule_name) => rule_name,
}
}

pub fn to_namespaced_rule_name(&self) -> String {
match self {
Self::Squawk(rule_name) => format!("squawk/{rule_name}"),
Self::Eugene(rule_name) => format!("eugene/{rule_name}"),
Self::Pgfence(rule_name) => format!("pgfence/{rule_name}"),
}
}

Expand All @@ -148,6 +153,7 @@ impl RuleSource {
Self::Eugene(rule_name) => {
format!("https://kaveland.no/eugene/hints/{rule_name}/index.html")
}
Self::Pgfence(_) => "https://github.com/flvmnt/pgfence".to_string(),
}
}

Expand Down
21 changes: 20 additions & 1 deletion crates/pgls_analyser/src/lint/safety.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,26 @@ pub mod adding_foreign_key_constraint;
pub mod adding_not_null_field;
pub mod adding_primary_key_constraint;
pub mod adding_required_field;
pub mod ban_add_exclusion_constraint;
pub mod ban_alter_enum_add_value;
pub mod ban_attach_partition;
pub mod ban_blocking_refresh_matview;
pub mod ban_char_field;
pub mod ban_concurrent_index_creation_in_transaction;
pub mod ban_create_trigger;
pub mod ban_delete_without_where;
pub mod ban_drop_column;
pub mod ban_drop_database;
pub mod ban_drop_not_null;
pub mod ban_drop_schema;
pub mod ban_drop_table;
pub mod ban_drop_trigger;
pub mod ban_enable_disable_trigger;
pub mod ban_not_valid_validate_same_transaction;
pub mod ban_truncate;
pub mod ban_truncate_cascade;
pub mod ban_update_without_where;
pub mod ban_vacuum_full;
pub mod changing_column_type;
pub mod constraint_missing_not_valid;
pub mod creating_enum;
Expand All @@ -30,8 +43,14 @@ pub mod prefer_text_field;
pub mod prefer_timestamptz;
pub mod renaming_column;
pub mod renaming_table;
pub mod require_concurrent_detach_partition;
pub mod require_concurrent_index_creation;
pub mod require_concurrent_index_deletion;
pub mod require_concurrent_reindex;
pub mod require_idle_in_transaction_timeout;
pub mod require_statement_timeout;
pub mod running_statement_while_holding_access_exclusive;
pub mod transaction_nesting;
declare_lint_group! { pub Safety { name : "safety" , rules : [self :: add_serial_column :: AddSerialColumn , 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 :: creating_enum :: CreatingEnum , self :: disallow_unique_constraint :: DisallowUniqueConstraint , self :: lock_timeout_warning :: LockTimeoutWarning , self :: multiple_alter_table :: MultipleAlterTable , self :: prefer_big_int :: PreferBigInt , self :: prefer_bigint_over_int :: PreferBigintOverInt , self :: prefer_bigint_over_smallint :: PreferBigintOverSmallint , self :: prefer_identity :: PreferIdentity , self :: prefer_jsonb :: PreferJsonb , 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 :: running_statement_while_holding_access_exclusive :: RunningStatementWhileHoldingAccessExclusive , self :: transaction_nesting :: TransactionNesting ,] } }
pub mod warn_refresh_matview_concurrent;
pub mod warn_wide_lock_window;
declare_lint_group! { pub Safety { name : "safety" , rules : [self :: add_serial_column :: AddSerialColumn , 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_add_exclusion_constraint :: BanAddExclusionConstraint , self :: ban_alter_enum_add_value :: BanAlterEnumAddValue , self :: ban_attach_partition :: BanAttachPartition , self :: ban_blocking_refresh_matview :: BanBlockingRefreshMatview , self :: ban_char_field :: BanCharField , self :: ban_concurrent_index_creation_in_transaction :: BanConcurrentIndexCreationInTransaction , self :: ban_create_trigger :: BanCreateTrigger , self :: ban_delete_without_where :: BanDeleteWithoutWhere , self :: ban_drop_column :: BanDropColumn , self :: ban_drop_database :: BanDropDatabase , self :: ban_drop_not_null :: BanDropNotNull , self :: ban_drop_schema :: BanDropSchema , self :: ban_drop_table :: BanDropTable , self :: ban_drop_trigger :: BanDropTrigger , self :: ban_enable_disable_trigger :: BanEnableDisableTrigger , self :: ban_not_valid_validate_same_transaction :: BanNotValidValidateSameTransaction , self :: ban_truncate :: BanTruncate , self :: ban_truncate_cascade :: BanTruncateCascade , self :: ban_update_without_where :: BanUpdateWithoutWhere , self :: ban_vacuum_full :: BanVacuumFull , self :: changing_column_type :: ChangingColumnType , self :: constraint_missing_not_valid :: ConstraintMissingNotValid , self :: creating_enum :: CreatingEnum , self :: disallow_unique_constraint :: DisallowUniqueConstraint , self :: lock_timeout_warning :: LockTimeoutWarning , self :: multiple_alter_table :: MultipleAlterTable , self :: prefer_big_int :: PreferBigInt , self :: prefer_bigint_over_int :: PreferBigintOverInt , self :: prefer_bigint_over_smallint :: PreferBigintOverSmallint , self :: prefer_identity :: PreferIdentity , self :: prefer_jsonb :: PreferJsonb , 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_detach_partition :: RequireConcurrentDetachPartition , self :: require_concurrent_index_creation :: RequireConcurrentIndexCreation , self :: require_concurrent_index_deletion :: RequireConcurrentIndexDeletion , self :: require_concurrent_reindex :: RequireConcurrentReindex , self :: require_idle_in_transaction_timeout :: RequireIdleInTransactionTimeout , self :: require_statement_timeout :: RequireStatementTimeout , self :: running_statement_while_holding_access_exclusive :: RunningStatementWhileHoldingAccessExclusive , self :: transaction_nesting :: TransactionNesting , self :: warn_refresh_matview_concurrent :: WarnRefreshMatviewConcurrent , self :: warn_wide_lock_window :: WarnWideLockWindow ,] } }
2 changes: 1 addition & 1 deletion crates/pgls_analyser/src/lint/safety/add_serial_column.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ declare_lint_rule! {
/// Adding a column with a SERIAL type or GENERATED ALWAYS AS ... STORED causes a full table rewrite.
///
/// When adding a column with a SERIAL type (serial, bigserial, smallserial) or a GENERATED ALWAYS AS ... STORED column
/// to an existing table, PostgreSQL must rewrite the entire table while holding an ACCESS EXCLUSIVE lock.
/// to an existing table, Postgres must rewrite the entire table while holding an ACCESS EXCLUSIVE lock.
/// This blocks all reads and writes to the table for the duration of the rewrite operation.
///
/// SERIAL types are implemented using sequences and DEFAULT values, while GENERATED ... STORED columns require
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,13 @@ use pgls_diagnostics::Severity;
declare_lint_rule! {
/// Adding a column with a DEFAULT value may lead to a table rewrite while holding an ACCESS EXCLUSIVE lock.
///
/// In PostgreSQL versions before 11, adding a column with a DEFAULT value causes a full table rewrite,
/// In Postgres versions before 11, adding a column with a DEFAULT value causes a full table rewrite,
/// which holds an ACCESS EXCLUSIVE lock on the table and blocks all reads and writes.
///
/// In PostgreSQL 11+, this behavior was optimized for non-volatile defaults. However:
/// In Postgres 11+, this behavior was optimized for non-volatile defaults. However:
/// - Volatile default values (like random() or custom functions) still cause table rewrites
/// - Generated columns (GENERATED ALWAYS AS) always require table rewrites
/// - Non-volatile defaults are safe in PostgreSQL 11+
/// - Non-volatile defaults are safe in Postgres 11+
///
/// ## Examples
///
Expand Down Expand Up @@ -45,7 +45,7 @@ impl LinterRule for AddingFieldWithDefault {
fn run(ctx: &LinterRuleContext<Self>) -> Vec<LinterDiagnostic> {
let mut diagnostics = Vec::new();

// Check PostgreSQL version - in 11+, non-volatile defaults are safe
// Check Postgres version - in 11+, non-volatile defaults are safe
let pg_version = ctx.schema_cache().and_then(|sc| sc.version.major_version);

if let pgls_query::NodeEnum::AlterTableStmt(stmt) = &ctx.stmt() {
Expand Down Expand Up @@ -110,7 +110,7 @@ impl LinterRule for AddingFieldWithDefault {
"Adding a column with a volatile default value causes a table rewrite."
},
)
.detail(None, "Even in PostgreSQL 11+, volatile default values require a full table rewrite.")
.detail(None, "Even in Postgres 11+, volatile default values require a full table rewrite.")
.note("Add the column without a default, then set the default in a separate statement."),
);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ declare_lint_rule! {
/// Adding a foreign key constraint requires a table scan and a SHARE ROW EXCLUSIVE lock on both tables, which blocks writes.
///
/// Adding a foreign key constraint to an existing table can cause downtime by locking both tables while
/// verifying the constraint. PostgreSQL needs to check that all existing values in the referencing
/// verifying the constraint. Postgres needs to check that all existing values in the referencing
/// column exist in the referenced table.
///
/// Instead, add the constraint as NOT VALID in one transaction, then VALIDATE it in another transaction.
Expand Down Expand Up @@ -111,7 +111,7 @@ fn check_foreign_key_constraint(
} else {
(
"Adding a foreign key constraint requires a table scan and locks on both tables.",
"This will block writes to both the referencing and referenced tables while PostgreSQL verifies the constraint.",
"This will block writes to both the referencing and referenced tables while Postgres verifies the constraint.",
"Add the constraint as NOT VALID first, then VALIDATE it in a separate transaction.",
)
};
Expand Down
4 changes: 2 additions & 2 deletions crates/pgls_analyser/src/lint/safety/adding_not_null_field.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,11 @@ use pgls_diagnostics::Severity;
declare_lint_rule! {
/// Setting a column NOT NULL blocks reads while the table is scanned.
///
/// In PostgreSQL versions before 11, adding a NOT NULL constraint to an existing column requires
/// In Postgres versions before 11, adding a NOT NULL constraint to an existing column requires
/// a full table scan to verify that all existing rows satisfy the constraint. This operation
/// takes an ACCESS EXCLUSIVE lock, blocking all reads and writes.
///
/// In PostgreSQL 11+, this operation is much faster as it can skip the full table scan for
/// In Postgres 11+, this operation is much faster as it can skip the full table scan for
/// newly added columns with default values.
///
/// Instead of using SET NOT NULL, consider using a CHECK constraint with NOT VALID, then
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ use pgls_diagnostics::Severity;
declare_lint_rule! {
/// Adding a primary key constraint results in locks and table rewrites.
///
/// When you add a PRIMARY KEY constraint, PostgreSQL needs to scan the entire table
/// When you add a PRIMARY KEY constraint, Postgres needs to scan the entire table
/// to verify uniqueness and build the underlying index. This requires an ACCESS EXCLUSIVE
/// lock which blocks all reads and writes.
///
Expand Down
Loading
Loading