diff --git a/docs/lint-report.md b/docs/lint-report.md index 63ab874..6aa1826 100644 --- a/docs/lint-report.md +++ b/docs/lint-report.md @@ -78,6 +78,7 @@ codes make the conservatism visible instead of burying it: | `blocking-idiom` | warning | `CREATE INDEX i ON t (c)` | The submitted form blocks readers or writers and a safer native form exists; `suggestion` carries it when the linter can construct one. | | `table-rewrite` | warning | `ALTER TABLE t ALTER COLUMN c TYPE jsonb USING c::jsonb` | The operation provably rewrites the table — only the engine's copy-and-swap path can run it online. | | `possible-table-rewrite` | warning | `ALTER TABLE t ALTER COLUMN c TYPE bigint` | The linter cannot verify the operation against live column facts, so the engine would fail closed to the rewrite path — but the change may be a free relabel a live database would prove. | +| `app-breaking-rename` | warning | `ALTER TABLE t RENAME COLUMN email TO email_address` | PostgreSQL runs a column or table rename as a metadata-only catalog flip, but a rename cannot land atomically across running application instances — code still referencing the old name breaks the instant it commits. For a column, expand/contract instead: add the new column, dual-write and backfill, switch reads, then drop the old column as its own reviewed change. For a table, coordinate the rename with the application deploy that adopts the new name. Index renames are not flagged — SQL never references an index by name. | | `destructive` | warning | `ALTER TABLE t DROP COLUMN legacy` | The operation discards live structure (a column, constraint, or index drop) and cannot be undone by re-running the schema. | ## Severities (`severity`) diff --git a/docs/low-level-design.md b/docs/low-level-design.md index bdc5ee5..f744227 100644 --- a/docs/low-level-design.md +++ b/docs/low-level-design.md @@ -277,7 +277,12 @@ live schema (introspected) ─┘ │ - **Renames are ambiguous and are not guessed.** A column present in live but absent in desired plus a new column in desired is, by default, a *drop + add*, not a rename. Rename intent must eventually be stated explicitly; no rename-intent flag exists today. The engine does not - heuristically pair columns. + heuristically pair columns. The imperative path executes a direct column or table rename when + asked — it is metadata-only for PostgreSQL — but classifies it `app-breaking-rename`: the + rename cannot land atomically across running application instances. For a column the safe + sequence is expand/contract (add the new column, dual-write and backfill, switch reads, drop + the old column separately); for a table, coordinate the rename with the application deploy + that adopts the new name. - **Diff is review-only.** `diff` prints every derived statement and its classified route without executing. Copy-and-swap routes render as unavailable until that backend exists. - **Out-of-band drift is surfaced, not steamrolled.** If the live table differs from what the diff --git a/docs/plan-report.md b/docs/plan-report.md index e798a28..15e023f 100644 --- a/docs/plan-report.md +++ b/docs/plan-report.md @@ -94,6 +94,7 @@ with SQL it does not fully understand. | `fast-default` | ADD COLUMN with a constant default — the catalog stores the default, no rewrite. | | `binary-coercible` | A type change PostgreSQL relabels without a rewrite (widen varchar, varchar to text, widen numeric precision). | | `safer-idiom` | Native, but the submitted form blocks; `safer_sql` carries the online rewrite when one can be constructed. | +| `app-breaking-rename` | A column or table rename — metadata-only for PostgreSQL, but running application code still referencing the old name breaks the instant it commits. For a column the safe sequence is expand/contract: add the new column, dual-write and backfill, switch reads, then drop the old column as its own reviewed change. For a table, coordinate the rename with the application deploy that adopts the new name. Index renames stay `metadata-only` — SQL never references an index by name. | | `volatile-default` | ADD COLUMN whose default the planner cannot prove constant — PostgreSQL rewrites the table. | | `generated-stored` | Adding a stored generated column computes every row — a full rewrite. | | `type-rewrite` | A type conversion PostgreSQL cannot relabel — rewrite plus reindex. | diff --git a/pkg/lint/lint.go b/pkg/lint/lint.go index d78e3b4..1280bf7 100644 --- a/pkg/lint/lint.go +++ b/pkg/lint/lint.go @@ -56,6 +56,14 @@ const ( // database would prove. The route is what the engine would do, not a // proven property of the change. CodePossibleTableRewrite Code = "possible-table-rewrite" + // CodeAppBreakingRename: the statement renames a column or table in + // place — metadata-only for PostgreSQL, but running application code + // still referencing the old name breaks the instant it commits. For + // a column the safe sequence is expand/contract: add the new column, + // dual-write and backfill, switch reads, then drop the old column as + // its own reviewed change. For a table, coordinate the rename with + // the application deploy that adopts the new name. + CodeAppBreakingRename Code = "app-breaking-rename" // CodeDestructive: the operation discards live structure (a column, // constraint, or index drop) and cannot be undone by re-running the // schema. Index drops are included because the linter cannot see @@ -198,10 +206,14 @@ func decisionFinding(d planner.Decision) (Finding, bool) { f.Code, f.Severity = CodeTableRewrite, SeverityWarning return f, true case planner.RouteNative: - if d.Reason == planner.ReasonSaferIdiom { + switch d.Reason { + case planner.ReasonSaferIdiom: f.Code, f.Severity = CodeBlockingIdiom, SeverityWarning f.Suggestion, f.SuggestionExecution = d.SaferSQL, d.SaferSQLExecution return f, true + case planner.ReasonAppBreakingRename: + f.Code, f.Severity = CodeAppBreakingRename, SeverityWarning + return f, true } return Finding{}, false default: diff --git a/pkg/lint/lint_test.go b/pkg/lint/lint_test.go index c23c1ec..facab7c 100644 --- a/pkg/lint/lint_test.go +++ b/pkg/lint/lint_test.go @@ -88,6 +88,29 @@ func TestCheckFlagsUnsupportedAsError(t *testing.T) { assert.Equal(t, 0, report.Warnings) } +// A column or table rename is metadata-only for PostgreSQL but breaks +// running application code the instant it commits; the finding steers to +// a safe sequence without blocking execution. Index renames stay clean — +// SQL never references an index by name. +func TestCheckFlagsRenamesAsAppBreaking(t *testing.T) { + report, err := lint.Check(` + ALTER TABLE t RENAME COLUMN a TO b; + ALTER TABLE t RENAME TO t2; + ALTER INDEX i RENAME TO i2; + `) + require.NoError(t, err) + require.Len(t, report.Findings, 2) + for i, f := range report.Findings { + assert.Equal(t, i+1, f.Statement) + assert.Equal(t, lint.CodeAppBreakingRename, f.Code) + assert.Equal(t, lint.SeverityWarning, f.Severity) + assert.Equal(t, planner.ReasonAppBreakingRename, f.Reason) + assert.Empty(t, f.Suggestion) + } + assert.Equal(t, 0, report.Errors) + assert.Equal(t, 2, report.Warnings) +} + // Destructive findings come from the classifier's destructive flag, so // the linter and the plan report mark the same operations by // construction. Index drops are destructive even in the concurrent form: diff --git a/pkg/planner/planner.go b/pkg/planner/planner.go index 588f61c..ea257a9 100644 --- a/pkg/planner/planner.go +++ b/pkg/planner/planner.go @@ -115,6 +115,15 @@ const ( // ReasonSaferIdiom: native, but the submitted form blocks; SaferSQL // carries the online rewrite when one can be constructed. ReasonSaferIdiom Reason = "safer-idiom" + // ReasonAppBreakingRename: PostgreSQL executes the rename as a brief + // metadata-only catalog flip, but it cannot land atomically across + // running application instances — code still referencing the old + // column or table name starts erroring the instant it commits. For a + // column the safe sequence is expand/contract: add the new column, + // dual-write and backfill, switch reads, then drop the old column as + // its own reviewed change. For a table, coordinate the rename with + // the application deploy that adopts the new name. + ReasonAppBreakingRename Reason = "app-breaking-rename" // ReasonVolatileDefault: ADD COLUMN whose default the planner cannot // prove constant — PostgreSQL rewrites the table. ReasonVolatileDefault Reason = "volatile-default" @@ -318,11 +327,21 @@ func classifyOp(op statement.Op, st statement.Statement, facts Facts, sql string } case statement.OpDropColumn, statement.OpSetDefault, statement.OpDropDefault, - statement.OpDropNotNull, statement.OpRenameColumn, statement.OpRenameTable, + statement.OpDropNotNull, statement.OpRenameIndex, statement.OpSetColumnOptions, statement.OpSetRelOptions, statement.OpSetSchema, statement.OpDropConstraint: d.Route, d.Reason = RouteNative, ReasonMetadataOnly + case statement.OpRenameColumn, statement.OpRenameTable: + // Metadata-only for PostgreSQL, but not for the application: a + // rename cannot land atomically across deployed instances, so + // code querying the old name breaks the instant it commits. The + // engine still executes it when asked; the typed reason lets + // lint and plan consumers steer to a safe sequence instead. + // Index renames stay metadata-only above — SQL never references + // an index by name. + d.Route, d.Reason = RouteNative, ReasonAppBreakingRename + case statement.OpAlterColumnType: d.Route, d.Reason, d.Unverified = classifyTypeChange(op, facts) diff --git a/pkg/planner/planner_test.go b/pkg/planner/planner_test.go index 229cd54..cebd879 100644 --- a/pkg/planner/planner_test.go +++ b/pkg/planner/planner_test.go @@ -138,7 +138,7 @@ func TestClassifyReferenceRows(t *testing.T) { {"drop default", "ALTER TABLE t ALTER COLUMN age DROP DEFAULT", planner.RouteNative, planner.ReasonMetadataOnly, 0}, {"set not null", "ALTER TABLE t ALTER COLUMN age SET NOT NULL", planner.RouteNative, planner.ReasonSaferIdiom, 4}, {"drop not null", "ALTER TABLE t ALTER COLUMN age DROP NOT NULL", planner.RouteNative, planner.ReasonMetadataOnly, 0}, - {"rename column", "ALTER TABLE t RENAME COLUMN a TO b", planner.RouteNative, planner.ReasonMetadataOnly, 0}, + {"rename column", "ALTER TABLE t RENAME COLUMN a TO b", planner.RouteNative, planner.ReasonAppBreakingRename, 0}, {"set statistics", "ALTER TABLE t ALTER COLUMN age SET STATISTICS 500", planner.RouteNative, planner.ReasonMetadataOnly, 0}, {"set storage", "ALTER TABLE t ALTER COLUMN blob SET STORAGE EXTERNAL", planner.RouteNative, planner.ReasonMetadataOnly, 0}, {"set column options", "ALTER TABLE t ALTER COLUMN age SET (n_distinct = 100)", planner.RouteNative, planner.ReasonMetadataOnly, 0}, @@ -166,7 +166,7 @@ func TestClassifyReferenceRows(t *testing.T) { {"drop constraint", "ALTER TABLE t DROP CONSTRAINT c", planner.RouteNative, planner.ReasonMetadataOnly, 0}, // Table and partition operations. - {"rename table", "ALTER TABLE t RENAME TO t2", planner.RouteNative, planner.ReasonMetadataOnly, 0}, + {"rename table", "ALTER TABLE t RENAME TO t2", planner.RouteNative, planner.ReasonAppBreakingRename, 0}, {"set schema", "ALTER TABLE t SET SCHEMA s2", planner.RouteNative, planner.ReasonMetadataOnly, 0}, {"set tablespace", "ALTER TABLE t SET TABLESPACE fast", planner.RouteCopyAndSwap, planner.ReasonRelocation, 0}, {"set fillfactor", "ALTER TABLE t SET (fillfactor = 70)", planner.RouteNative, planner.ReasonMetadataOnly, 0}, diff --git a/pkg/schemadiff/diff_test.go b/pkg/schemadiff/diff_test.go index 71b92ed..d15dcb8 100644 --- a/pkg/schemadiff/diff_test.go +++ b/pkg/schemadiff/diff_test.go @@ -58,6 +58,36 @@ func TestDiffAddColumn(t *testing.T) { assert.False(t, changes[0].Destructive) } +// A rename expressed declaratively — the old column name gone, a new one +// present — is a drop plus an add, never an inferred rename: the differ +// does not heuristically pair columns, and the drop stays destructive so +// the caller gates it. Carrying the data over is expand/contract work +// (add, dual-write and backfill, switch reads, drop), not a diff. +func TestDiffRenameShapeIsDropPlusAdd(t *testing.T) { + live := Model{ + Table: "users", + Columns: []Column{ + {Name: "id", Type: "bigint", NotNull: true}, + {Name: "email", Type: "text"}, + }, + } + desired := Model{ + Table: "users", + Columns: []Column{ + {Name: "id", Type: "bigint", NotNull: true}, + {Name: "email_address", Type: "text"}, + }, + } + changes, err := Diff("public", live, desired) + require.NoError(t, err) + require.Equal(t, []string{ + `ALTER TABLE "public"."users" DROP COLUMN "email"`, + `ALTER TABLE "public"."users" ADD COLUMN "email_address" text`, + }, sqls(changes)) + assert.True(t, changes[0].Destructive) + assert.False(t, changes[1].Destructive) +} + func TestDiffDropColumnIsDestructive(t *testing.T) { desired := base() desired.Columns = desired.Columns[:1] // drop "name"