From d4521a780632b307943169bf88d86120a91dd4ee Mon Sep 17 00:00:00 2001 From: tianzhou Date: Wed, 9 Sep 2026 03:38:06 -0700 Subject: [PATCH 01/11] fix: detect and apply GENERATED ALWAYS AS changes on existing columns (#591) columnsEqual never looked at IsGenerated, GeneratedKind, or GeneratedExpr, so changing the expression of a generated column, or switching a column between plain and generated, produced an empty plan and left every existing database computing the old value. The generation clause is now part of the column comparison, and the change is applied with the narrowest DDL PostgreSQL offers: - expression change: ALTER COLUMN ... SET EXPRESSION AS (PostgreSQL 17+) - STORED -> plain: ALTER COLUMN ... DROP EXPRESSION - plain -> generated, STORED <-> VIRTUAL, VIRTUAL -> plain, and expression changes on PostgreSQL 14-16: DROP COLUMN + ADD COLUMN. A generated column holds no data of its own, so nothing is lost; indexes and constraints that DROP COLUMN takes with it are re-created from the desired state, and foreign keys bound to a replaced unique/PK constraint go through the existing pre-drop/post-add path. The target major version is threaded into the diff through the new GenerateMigrationForTarget so the plan command can pick the right form. Co-Authored-By: Claude Fable 5.1 --- cmd/plan/plan.go | 10 +- internal/diff/column.go | 75 ++++++++++++ internal/diff/diff.go | 19 ++- internal/diff/generated_column_test.go | 100 ++++++++++++++++ internal/diff/table.go | 86 +++++++++++++- .../issue_591_alter_generated_column/diff.sql | 20 ++++ .../issue_591_alter_generated_column/new.sql | 18 +++ .../issue_591_alter_generated_column/old.sql | 18 +++ .../plan.json | 110 ++++++++++++++++++ .../issue_591_alter_generated_column/plan.sql | 36 ++++++ .../issue_591_alter_generated_column/plan.txt | 62 ++++++++++ .../diff.sql | 9 ++ .../issue_591_alter_generated_virtual/new.sql | 6 + .../issue_591_alter_generated_virtual/old.sql | 6 + .../plan.json | 44 +++++++ .../plan.sql | 9 ++ .../plan.txt | 25 ++++ 17 files changed, 644 insertions(+), 9 deletions(-) create mode 100644 internal/diff/generated_column_test.go create mode 100644 testdata/diff/create_table/issue_591_alter_generated_column/diff.sql create mode 100644 testdata/diff/create_table/issue_591_alter_generated_column/new.sql create mode 100644 testdata/diff/create_table/issue_591_alter_generated_column/old.sql create mode 100644 testdata/diff/create_table/issue_591_alter_generated_column/plan.json create mode 100644 testdata/diff/create_table/issue_591_alter_generated_column/plan.sql create mode 100644 testdata/diff/create_table/issue_591_alter_generated_column/plan.txt create mode 100644 testdata/diff/create_table/issue_591_alter_generated_virtual/diff.sql create mode 100644 testdata/diff/create_table/issue_591_alter_generated_virtual/new.sql create mode 100644 testdata/diff/create_table/issue_591_alter_generated_virtual/old.sql create mode 100644 testdata/diff/create_table/issue_591_alter_generated_virtual/plan.json create mode 100644 testdata/diff/create_table/issue_591_alter_generated_virtual/plan.sql create mode 100644 testdata/diff/create_table/issue_591_alter_generated_virtual/plan.txt diff --git a/cmd/plan/plan.go b/cmd/plan/plan.go index 057ded03..f234353b 100644 --- a/cmd/plan/plan.go +++ b/cmd/plan/plan.go @@ -395,17 +395,17 @@ func GeneratePlan(config *PlanConfig, provider postgres.DesiredStateProvider) (* normalizeSchemaNames(desiredStateIR, schemaToInspect, config.Schema) } - // Generate diff (current -> desired) using IR directly - diffs := diff.GenerateMigration(currentStateIR, desiredStateIR, config.Schema) - // Extract the target database's major version (e.g. "PostgreSQL 18.1" -> 18) - // to gate version-specific rewrites. Zero (unknown) falls back to the - // version-portable rewrite patterns. + // to gate version-specific DDL and rewrites. Zero (unknown) falls back to + // the version-portable patterns. targetMajorVersion := 0 if v, ok := strings.CutPrefix(currentStateIR.Metadata.DatabaseVersion, "PostgreSQL "); ok { fmt.Sscanf(v, "%d", &targetMajorVersion) } + // Generate diff (current -> desired) using IR directly + diffs := diff.GenerateMigrationForTarget(currentStateIR, desiredStateIR, config.Schema, targetMajorVersion) + // Create plan from diffs with fingerprint migrationPlan := plan.NewPlanWithFingerprint(diffs, sourceFingerprint, targetMajorVersion, currentStateIR) diff --git a/internal/diff/column.go b/internal/diff/column.go index 3b822952..6b9c50da 100644 --- a/internal/diff/column.go +++ b/internal/diff/column.go @@ -31,6 +31,17 @@ func (cd *ColumnDiff) generateColumnSQL(tableSchema, tableName string, targetSch hasOldDefault := oldDefault != nil && *oldDefault != "" needsUsing := hasTypeChange && needsUsingClause(oldBaseType, newBaseType) + // A STORED generated column turning into a plain column keeps its current + // values and simply stops being recomputed (issue #591). Emit it first so + // the remaining clauses (type, default, NOT NULL) act on a plain column. + // VIRTUAL -> plain and the other transitions without an ALTER form are + // handled by re-creating the column instead (see generatedColumnNeedsRecreate). + if cd.Old.IsGenerated && !cd.New.IsGenerated { + sql := fmt.Sprintf("ALTER TABLE %s ALTER COLUMN %s DROP EXPRESSION;", + qualifiedTableName, ir.QuoteIdentifier(cd.New.Name)) + statements = append(statements, sql) + } + // If type is changing with USING clause and there's an existing default, drop the default first if needsUsing && hasOldDefault { sql := fmt.Sprintf("ALTER TABLE %s ALTER COLUMN %s DROP DEFAULT;", @@ -54,6 +65,15 @@ func (cd *ColumnDiff) generateColumnSQL(tableSchema, tableName string, targetSch } } + // Handle generation expression changes (issue #591). SET EXPRESSION AS + // (PostgreSQL 17+) recomputes STORED values in place; on older servers the + // column is re-created instead and never reaches this point. + if cd.Old.IsGenerated && cd.New.IsGenerated && generatedExpr(cd.Old) != generatedExpr(cd.New) { + sql := fmt.Sprintf("ALTER TABLE %s ALTER COLUMN %s SET EXPRESSION AS (%s);", + qualifiedTableName, ir.QuoteIdentifier(cd.New.Name), generatedExpr(cd.New)) + statements = append(statements, sql) + } + // Handle nullable changes if cd.Old.IsNullable != cd.New.IsNullable { if cd.New.IsNullable { @@ -217,5 +237,60 @@ func columnsEqual(old, new *ir.Column, targetSchema string) bool { return false } + // Compare the generation clause (issue #591) + if generatedColumnChanged(old, new) { + return false + } + return true } + +// generatedExpr returns the generation expression of a column, or "" when the +// column is not generated. +func generatedExpr(c *ir.Column) string { + if !c.IsGenerated || c.GeneratedExpr == nil { + return "" + } + return *c.GeneratedExpr +} + +// generatedColumnChanged reports whether the generation clause differs between +// two versions of a column: plain vs generated, STORED vs VIRTUAL, or the +// expression itself. Both expressions come from pg_get_expr on the same +// inspector, so they compare textually (issue #591). +func generatedColumnChanged(old, new *ir.Column) bool { + if old.IsGenerated != new.IsGenerated { + return true + } + if !new.IsGenerated { + return false + } + return old.GeneratedKind != new.GeneratedKind || generatedExpr(old) != generatedExpr(new) +} + +// generatedColumnNeedsRecreate reports whether a generation-clause change can +// only be applied by dropping and re-adding the column. PostgreSQL has no ALTER +// form for turning a plain column into a generated one, for switching between +// STORED and VIRTUAL, or for dropping the expression of a VIRTUAL column, and +// ALTER COLUMN ... SET EXPRESSION AS only exists on PostgreSQL 17+. A generated +// column holds no data of its own, so recreating it loses nothing; the indexes +// and constraints that DROP COLUMN takes with it are re-created by diffTables. +// targetMajorVersion 0 means unknown and is treated as a current server. +func generatedColumnNeedsRecreate(old, new *ir.Column, targetMajorVersion int) bool { + if !generatedColumnChanged(old, new) { + return false + } + switch { + case !old.IsGenerated: + // plain -> generated + return true + case !new.IsGenerated: + // generated -> plain: DROP EXPRESSION only works on STORED columns + return old.GeneratedKind == "v" + case old.GeneratedKind != new.GeneratedKind: + return true + default: + // expression change: SET EXPRESSION AS needs PostgreSQL 17+ + return targetMajorVersion != 0 && targetMajorVersion < 17 + } +} diff --git a/internal/diff/diff.go b/internal/diff/diff.go index 5997df58..18ef3257 100644 --- a/internal/diff/diff.go +++ b/internal/diff/diff.go @@ -270,6 +270,7 @@ type Diff struct { } type ddlDiff struct { + targetMajorVersion int // target PostgreSQL major version, 0 if unknown (gates version-specific DDL) addedSchemas []*ir.Schema droppedSchemas []*ir.Schema modifiedSchemas []*schemaDiff @@ -478,7 +479,16 @@ type rlsChange struct { // GenerateMigration generates the migration diff using standard "smart qualification" // (the target-schema prefix is omitted on entity names). func GenerateMigration(oldIR, newIR *ir.IR, targetSchema string) []Diff { - return GenerateMigrationWithOptions(oldIR, newIR, targetSchema, false) + return generateMigration(oldIR, newIR, targetSchema, false, 0) +} + +// GenerateMigrationForTarget is GenerateMigration for a target database whose +// PostgreSQL major version is known (0 if unknown, treated as a current +// server). The version gates DDL that only newer servers accept, such as +// ALTER COLUMN ... SET EXPRESSION AS (PostgreSQL 17+); older targets get a +// version-portable equivalent instead (issue #591). +func GenerateMigrationForTarget(oldIR, newIR *ir.IR, targetSchema string, targetMajorVersion int) []Diff { + return generateMigration(oldIR, newIR, targetSchema, false, targetMajorVersion) } // GenerateMigrationWithOptions is like GenerateMigration, but when qualifySchema is @@ -491,7 +501,12 @@ func GenerateMigration(oldIR, newIR *ir.IR, targetSchema string) []Diff { // stay bare because the IR stores them without schema identity (#493). Default // behavior (false) is unchanged for plan/apply. func GenerateMigrationWithOptions(oldIR, newIR *ir.IR, targetSchema string, qualifySchema bool) []Diff { + return generateMigration(oldIR, newIR, targetSchema, qualifySchema, 0) +} + +func generateMigration(oldIR, newIR *ir.IR, targetSchema string, qualifySchema bool, targetMajorVersion int) []Diff { diff := &ddlDiff{ + targetMajorVersion: targetMajorVersion, addedSchemas: []*ir.Schema{}, droppedSchemas: []*ir.Schema{}, modifiedSchemas: []*schemaDiff{}, @@ -635,7 +650,7 @@ func GenerateMigrationWithOptions(oldIR, newIR *ir.IR, targetSchema string, qual diff.modifiedTables = append(diff.modifiedTables, tableDiff) } } else { - if tableDiff := diffTables(oldTable, newTable, targetSchema); tableDiff != nil { + if tableDiff := diffTables(oldTable, newTable, targetSchema, diff.targetMajorVersion); tableDiff != nil { diff.modifiedTables = append(diff.modifiedTables, tableDiff) } } diff --git a/internal/diff/generated_column_test.go b/internal/diff/generated_column_test.go new file mode 100644 index 00000000..bc190e6e --- /dev/null +++ b/internal/diff/generated_column_test.go @@ -0,0 +1,100 @@ +package diff + +import ( + "strings" + "testing" + + "github.com/pgplex/pgschema/ir" +) + +// generatedColumnIR builds a one-table IR whose column "doubled" is +// GENERATED ALWAYS AS (expr) STORED, with a plain index on it. +func generatedColumnIR(expr string) *ir.IR { + table := &ir.Table{ + Schema: "public", + Name: "metrics", + Type: ir.TableTypeBase, + Columns: []*ir.Column{ + {Name: "a", Position: 1, DataType: "integer", IsNullable: false}, + {Name: "doubled", Position: 2, DataType: "integer", IsNullable: true, + IsGenerated: true, GeneratedKind: "s", GeneratedExpr: &expr}, + }, + Constraints: map[string]*ir.Constraint{}, + Indexes: map[string]*ir.Index{ + "metrics_doubled_idx": { + Schema: "public", Table: "metrics", Name: "metrics_doubled_idx", + Type: ir.IndexTypeRegular, Method: "btree", + Columns: []*ir.IndexColumn{{Name: "doubled", Position: 1, Direction: "ASC"}}, + }, + }, + } + return &ir.IR{ + Schemas: map[string]*ir.Schema{ + "public": {Name: "public", Tables: map[string]*ir.Table{"metrics": table}}, + }, + } +} + +func migrationSQL(diffs []Diff) string { + var b strings.Builder + for _, d := range diffs { + for _, s := range d.Statements { + b.WriteString(s.SQL) + b.WriteString("\n") + } + } + return b.String() +} + +// A generated-column expression change uses SET EXPRESSION AS on PostgreSQL +// 17+ (and when the version is unknown), and re-creates the column together +// with its dependent index on older servers, where that clause does not +// exist (issue #591). +func TestGeneratedExpressionChange_VersionGate(t *testing.T) { + oldIR := generatedColumnIR("(a * 3)") + newIR := generatedColumnIR("(a * 2)") + + for _, version := range []int{0, 17, 18} { + got := migrationSQL(GenerateMigrationForTarget(oldIR, newIR, "public", version)) + want := "ALTER TABLE metrics ALTER COLUMN doubled SET EXPRESSION AS ((a * 2));\n" + if got != want { + t.Errorf("version %d: got\n%s\nwant\n%s", version, got, want) + } + } + + for _, version := range []int{14, 16} { + got := migrationSQL(GenerateMigrationForTarget(oldIR, newIR, "public", version)) + want := strings.Join([]string{ + "ALTER TABLE metrics DROP COLUMN doubled;", + "ALTER TABLE metrics ADD COLUMN doubled integer GENERATED ALWAYS AS ((a * 2)) STORED;", + "CREATE INDEX IF NOT EXISTS metrics_doubled_idx ON metrics (doubled);", + "", + }, "\n") + if got != want { + t.Errorf("version %d: got\n%s\nwant\n%s", version, got, want) + } + } +} + +func TestExprReferencesAnyColumn(t *testing.T) { + cols := map[string]bool{"b": true, "my col": true} + cases := []struct { + expr string + want bool + }{ + {"(b + 1)", true}, + {"b", true}, + {"(\"my col\" * 2)", true}, + {"(b > 10)", true}, + {"(bb + 1)", false}, + {"(a + 1)", false}, + {"b(a)", false}, // function call, not a column + {"('b'::text)", false}, // string literal + {"", false}, + } + for _, c := range cases { + if got := exprReferencesAnyColumn(c.expr, cols); got != c.want { + t.Errorf("exprReferencesAnyColumn(%q) = %v, want %v", c.expr, got, c.want) + } + } +} diff --git a/internal/diff/table.go b/internal/diff/table.go index 95562051..210cb7bf 100644 --- a/internal/diff/table.go +++ b/internal/diff/table.go @@ -2,6 +2,7 @@ package diff import ( "fmt" + "regexp" "sort" "strings" @@ -137,7 +138,10 @@ func diffTriggers(oldTable, newTable *ir.Table, diff *tableDiff) { // diffTables compares two tables and returns the differences // targetSchema is used to normalize type names before comparison -func diffTables(oldTable, newTable *ir.Table, targetSchema string) *tableDiff { +// targetMajorVersion is the target database's PostgreSQL major version (0 if +// unknown); it decides whether a generated-column expression change can use +// SET EXPRESSION AS (PG17+) or must re-create the column (issue #591). +func diffTables(oldTable, newTable *ir.Table, targetSchema string, targetMajorVersion int) *tableDiff { diff := &tableDiff{ Table: newTable, AddedColumns: []*ir.Column{}, @@ -184,10 +188,19 @@ func diffTables(oldTable, newTable *ir.Table, targetSchema string) *tableDiff { } } - // Find modified columns + // Find modified columns. A generation-clause change that PostgreSQL cannot + // ALTER in place is applied as DROP COLUMN + ADD COLUMN instead (issue #591); + // recreatedColumns drives the re-creation of their dependent objects below. + recreatedColumns := make(map[string]bool) for name, newColumn := range newColumns { if oldColumn, exists := oldColumns[name]; exists { if !columnsEqual(oldColumn, newColumn, targetSchema) { + if generatedColumnNeedsRecreate(oldColumn, newColumn, targetMajorVersion) { + diff.DroppedColumns = append(diff.DroppedColumns, oldColumn) + diff.AddedColumns = append(diff.AddedColumns, newColumn) + recreatedColumns[name] = true + continue + } diff.ModifiedColumns = append(diff.ModifiedColumns, &ColumnDiff{ Old: oldColumn, New: newColumn, @@ -229,6 +242,17 @@ func diffTables(oldTable, newTable *ir.Table, targetSchema string) *tableDiff { // Find modified constraints for name, newConstraint := range newConstraints { if oldConstraint, exists := oldConstraints[name]; exists { + // A constraint on a re-created column goes away with DROP COLUMN, so + // it must be added back once the column exists again. Listing it as + // dropped (the DROP statement itself is skipped, see + // constraintDroppedWithColumns) also lets + // planFKRecreationForReplacedConstraints re-bind foreign keys that + // depend on a unique/PK constraint among them. (#591) + if constraintDroppedWithColumns(newConstraint, recreatedColumns) { + diff.DroppedConstraints = append(diff.DroppedConstraints, oldConstraint) + diff.AddedConstraints = append(diff.AddedConstraints, newConstraint) + continue + } if !constraintsEqual(oldConstraint, newConstraint) { diff.ModifiedConstraints = append(diff.ModifiedConstraints, &ConstraintDiff{ Old: oldConstraint, @@ -260,6 +284,10 @@ func diffTables(oldTable, newTable *ir.Table, targetSchema string) *tableDiff { // Find dropped indexes for name, index := range oldIndexes { if _, exists := newIndexes[name]; !exists { + // Already gone with the re-created column it depends on. (#591) + if indexReferencesColumns(index, recreatedColumns) { + continue + } diff.DroppedIndexes = append(diff.DroppedIndexes, index) } } @@ -267,6 +295,12 @@ func diffTables(oldTable, newTable *ir.Table, targetSchema string) *tableDiff { // Find modified indexes (comment changes and structural changes) for name, newIndex := range newIndexes { if oldIndex, exists := oldIndexes[name]; exists { + // DROP COLUMN removes every index on a re-created column, so the + // desired-state index is created afresh afterwards. (#591) + if indexReferencesColumns(newIndex, recreatedColumns) { + diff.AddedIndexes = append(diff.AddedIndexes, newIndex) + continue + } structurallyEqual := indexesStructurallyEqual(oldIndex, newIndex) commentChanged := oldIndex.Comment != newIndex.Comment @@ -1044,6 +1078,54 @@ func constraintDroppedWithColumns(constraint *ir.Constraint, droppedColumnSet ma return false } +// indexReferencesColumns reports whether an index depends on any of the given +// columns, i.e. whether ALTER TABLE ... DROP COLUMN of one of them removes it: +// as a key or INCLUDE column, inside an expression, or in the partial-index +// predicate. (#591) +func indexReferencesColumns(index *ir.Index, columns map[string]bool) bool { + if index == nil || len(columns) == 0 { + return false + } + for _, col := range index.Columns { + // pg_get_indexdef renders a key column as its (possibly quoted) name + // and an expression column as the expression text. + if columns[col.Name] || exprReferencesAnyColumn(col.Name, columns) { + return true + } + } + for _, name := range index.IncludeColumns { + if columns[name] || exprReferencesAnyColumn(name, columns) { + return true + } + } + return index.IsPartial && exprReferencesAnyColumn(index.Where, columns) +} + +// sqlStringLiteralRegex matches a single-quoted SQL string literal, including +// doubled-quote escapes. +var sqlStringLiteralRegex = regexp.MustCompile(`'(?:[^']|'')*'`) + +// exprReferencesAnyColumn reports whether a SQL expression as rendered by +// pg_get_expr mentions any of the columns as a bare or quoted identifier. +// String literals are blanked out first, and a name directly followed by "(" +// is a function call rather than a column. A false positive only costs a +// redundant CREATE INDEX that fails loudly at apply time, whereas a miss would +// silently lose the dependent index. (#591) +func exprReferencesAnyColumn(expr string, columns map[string]bool) bool { + if expr == "" || len(columns) == 0 { + return false + } + expr = sqlStringLiteralRegex.ReplaceAllString(expr, "''") + for column := range columns { + q := regexp.QuoteMeta(column) + re := regexp.MustCompile(`(?:^|[^\w"])(?:` + q + `|"` + q + `")(?:[^\w"(]|$)`) + if re.MatchString(expr) { + return true + } + } + return false +} + // generateAlterTableStatements generates SQL statements for table modifications // Note: DroppedTriggers are skipped here because they are already processed in the DROP phase // (see generateDropTriggersFromModifiedTables in trigger.go) diff --git a/testdata/diff/create_table/issue_591_alter_generated_column/diff.sql b/testdata/diff/create_table/issue_591_alter_generated_column/diff.sql new file mode 100644 index 00000000..ad55b441 --- /dev/null +++ b/testdata/diff/create_table/issue_591_alter_generated_column/diff.sql @@ -0,0 +1,20 @@ +ALTER TABLE metric_refs DROP CONSTRAINT metric_refs_tripled_fkey; + +ALTER TABLE metrics DROP COLUMN tripled; + +ALTER TABLE metrics +ADD COLUMN tripled integer GENERATED ALWAYS AS ((a * 3)) STORED CONSTRAINT metrics_tripled_key UNIQUE; + +ALTER TABLE metrics ALTER COLUMN doubled SET EXPRESSION AS ((a * 2)); + +ALTER TABLE metrics ALTER COLUMN label DROP EXPRESSION; + +ALTER TABLE metrics ALTER COLUMN label SET DEFAULT 'none'; + +ALTER TABLE metrics +ADD CONSTRAINT metrics_tripled_check CHECK (tripled > 0); + +CREATE INDEX IF NOT EXISTS metrics_tripled_idx ON metrics ((tripled + 1)) WHERE (tripled > 10); + +ALTER TABLE metric_refs +ADD CONSTRAINT metric_refs_tripled_fkey FOREIGN KEY (tripled) REFERENCES metrics (tripled); diff --git a/testdata/diff/create_table/issue_591_alter_generated_column/new.sql b/testdata/diff/create_table/issue_591_alter_generated_column/new.sql new file mode 100644 index 00000000..b19da6cf --- /dev/null +++ b/testdata/diff/create_table/issue_591_alter_generated_column/new.sql @@ -0,0 +1,18 @@ +CREATE TABLE public.metrics ( + id integer PRIMARY KEY, + a integer NOT NULL, + doubled integer GENERATED ALWAYS AS (a * 2) STORED, + label text DEFAULT 'none', + tripled integer GENERATED ALWAYS AS (a * 3) STORED, + CONSTRAINT metrics_tripled_check CHECK (tripled > 0), + CONSTRAINT metrics_tripled_key UNIQUE (tripled) +); + +CREATE INDEX metrics_doubled_idx ON public.metrics (doubled); + +CREATE INDEX metrics_tripled_idx ON public.metrics ((tripled + 1)) WHERE tripled > 10; + +CREATE TABLE public.metric_refs ( + tripled integer, + CONSTRAINT metric_refs_tripled_fkey FOREIGN KEY (tripled) REFERENCES public.metrics (tripled) +); diff --git a/testdata/diff/create_table/issue_591_alter_generated_column/old.sql b/testdata/diff/create_table/issue_591_alter_generated_column/old.sql new file mode 100644 index 00000000..79a0c34d --- /dev/null +++ b/testdata/diff/create_table/issue_591_alter_generated_column/old.sql @@ -0,0 +1,18 @@ +CREATE TABLE public.metrics ( + id integer PRIMARY KEY, + a integer NOT NULL, + doubled integer GENERATED ALWAYS AS (a * 3) STORED, + label text GENERATED ALWAYS AS ('n=' || a) STORED, + tripled integer, + CONSTRAINT metrics_tripled_check CHECK (tripled > 0), + CONSTRAINT metrics_tripled_key UNIQUE (tripled) +); + +CREATE INDEX metrics_doubled_idx ON public.metrics (doubled); + +CREATE INDEX metrics_tripled_idx ON public.metrics ((tripled + 1)) WHERE tripled > 10; + +CREATE TABLE public.metric_refs ( + tripled integer, + CONSTRAINT metric_refs_tripled_fkey FOREIGN KEY (tripled) REFERENCES public.metrics (tripled) +); diff --git a/testdata/diff/create_table/issue_591_alter_generated_column/plan.json b/testdata/diff/create_table/issue_591_alter_generated_column/plan.json new file mode 100644 index 00000000..a4a72ac0 --- /dev/null +++ b/testdata/diff/create_table/issue_591_alter_generated_column/plan.json @@ -0,0 +1,110 @@ +{ + "version": "1.0.0", + "pgschema_version": "1.13.0", + "created_at": "1970-01-01T00:00:00Z", + "source_fingerprint": { + "hash": "5de493c70ef78fca50074be3dd6d594c731b27ee3810f598460fdea5bb75c106" + }, + "groups": [ + { + "steps": [ + { + "sql": "ALTER TABLE metric_refs DROP CONSTRAINT metric_refs_tripled_fkey;", + "type": "table.constraint", + "operation": "drop", + "path": "public.metric_refs.metric_refs_tripled_fkey" + }, + { + "sql": "ALTER TABLE metrics DROP COLUMN tripled;", + "type": "table.column", + "operation": "drop", + "path": "public.metrics.tripled" + }, + { + "sql": "ALTER TABLE metrics\nADD COLUMN tripled integer GENERATED ALWAYS AS ((a * 3)) STORED CONSTRAINT metrics_tripled_key UNIQUE;", + "type": "table.column", + "operation": "create", + "path": "public.metrics.tripled" + }, + { + "sql": "ALTER TABLE metrics ALTER COLUMN doubled SET EXPRESSION AS ((a * 2));", + "type": "table.column", + "operation": "alter", + "path": "public.metrics.doubled" + }, + { + "sql": "ALTER TABLE metrics ALTER COLUMN label DROP EXPRESSION;", + "type": "table.column", + "operation": "alter", + "path": "public.metrics.label" + }, + { + "sql": "ALTER TABLE metrics ALTER COLUMN label SET DEFAULT 'none';", + "type": "table.column", + "operation": "alter", + "path": "public.metrics.label" + }, + { + "sql": "ALTER TABLE metrics\nADD CONSTRAINT metrics_tripled_check CHECK (tripled > 0) NOT VALID;", + "type": "table.constraint", + "operation": "create", + "path": "public.metrics.metrics_tripled_check" + } + ] + }, + { + "steps": [ + { + "sql": "ALTER TABLE metrics VALIDATE CONSTRAINT metrics_tripled_check;", + "type": "table.constraint", + "operation": "create", + "path": "public.metrics.metrics_tripled_check" + } + ] + }, + { + "steps": [ + { + "sql": "CREATE INDEX CONCURRENTLY IF NOT EXISTS metrics_tripled_idx ON metrics ((tripled + 1)) WHERE (tripled > 10);", + "type": "table.index", + "operation": "create", + "path": "public.metrics.metrics_tripled_idx" + } + ] + }, + { + "steps": [ + { + "sql": "SELECT \n COALESCE(i.indisvalid, false) as done,\n CASE \n WHEN p.blocks_total > 0 THEN p.blocks_done * 100 / p.blocks_total\n ELSE 0\n END as progress\nFROM pg_class c\nLEFT JOIN pg_index i ON c.oid = i.indexrelid\nLEFT JOIN pg_stat_progress_create_index p ON c.oid = p.index_relid\nWHERE c.relname = 'metrics_tripled_idx';", + "directive": { + "type": "wait", + "message": "Creating index metrics_tripled_idx" + }, + "type": "table.index", + "operation": "create", + "path": "public.metrics.metrics_tripled_idx" + } + ] + }, + { + "steps": [ + { + "sql": "ALTER TABLE metric_refs\nADD CONSTRAINT metric_refs_tripled_fkey FOREIGN KEY (tripled) REFERENCES metrics (tripled) NOT VALID;", + "type": "table.constraint", + "operation": "create", + "path": "public.metric_refs.metric_refs_tripled_fkey" + } + ] + }, + { + "steps": [ + { + "sql": "ALTER TABLE metric_refs VALIDATE CONSTRAINT metric_refs_tripled_fkey;", + "type": "table.constraint", + "operation": "create", + "path": "public.metric_refs.metric_refs_tripled_fkey" + } + ] + } + ] +} diff --git a/testdata/diff/create_table/issue_591_alter_generated_column/plan.sql b/testdata/diff/create_table/issue_591_alter_generated_column/plan.sql new file mode 100644 index 00000000..8b7fa209 --- /dev/null +++ b/testdata/diff/create_table/issue_591_alter_generated_column/plan.sql @@ -0,0 +1,36 @@ +ALTER TABLE metric_refs DROP CONSTRAINT metric_refs_tripled_fkey; + +ALTER TABLE metrics DROP COLUMN tripled; + +ALTER TABLE metrics +ADD COLUMN tripled integer GENERATED ALWAYS AS ((a * 3)) STORED CONSTRAINT metrics_tripled_key UNIQUE; + +ALTER TABLE metrics ALTER COLUMN doubled SET EXPRESSION AS ((a * 2)); + +ALTER TABLE metrics ALTER COLUMN label DROP EXPRESSION; + +ALTER TABLE metrics ALTER COLUMN label SET DEFAULT 'none'; + +ALTER TABLE metrics +ADD CONSTRAINT metrics_tripled_check CHECK (tripled > 0) NOT VALID; + +ALTER TABLE metrics VALIDATE CONSTRAINT metrics_tripled_check; + +CREATE INDEX CONCURRENTLY IF NOT EXISTS metrics_tripled_idx ON metrics ((tripled + 1)) WHERE (tripled > 10); + +-- pgschema:wait +SELECT + COALESCE(i.indisvalid, false) as done, + CASE + WHEN p.blocks_total > 0 THEN p.blocks_done * 100 / p.blocks_total + ELSE 0 + END as progress +FROM pg_class c +LEFT JOIN pg_index i ON c.oid = i.indexrelid +LEFT JOIN pg_stat_progress_create_index p ON c.oid = p.index_relid +WHERE c.relname = 'metrics_tripled_idx'; + +ALTER TABLE metric_refs +ADD CONSTRAINT metric_refs_tripled_fkey FOREIGN KEY (tripled) REFERENCES metrics (tripled) NOT VALID; + +ALTER TABLE metric_refs VALIDATE CONSTRAINT metric_refs_tripled_fkey; diff --git a/testdata/diff/create_table/issue_591_alter_generated_column/plan.txt b/testdata/diff/create_table/issue_591_alter_generated_column/plan.txt new file mode 100644 index 00000000..1126c374 --- /dev/null +++ b/testdata/diff/create_table/issue_591_alter_generated_column/plan.txt @@ -0,0 +1,62 @@ +Plan: 2 to modify. + +Summary by type: + tables: 2 to modify + +Tables: + ~ metric_refs + - metric_refs_tripled_fkey (constraint) + + metric_refs_tripled_fkey (constraint) + ~ metrics + ~ doubled (column) + ~ label (column) + - tripled (column) + + tripled (column) + + metrics_tripled_check (constraint) + + metrics_tripled_idx (index) + +DDL to be executed: +-------------------------------------------------- + +-- Transaction Group #1 +ALTER TABLE metric_refs DROP CONSTRAINT metric_refs_tripled_fkey; + +ALTER TABLE metrics DROP COLUMN tripled; + +ALTER TABLE metrics +ADD COLUMN tripled integer GENERATED ALWAYS AS ((a * 3)) STORED CONSTRAINT metrics_tripled_key UNIQUE; + +ALTER TABLE metrics ALTER COLUMN doubled SET EXPRESSION AS ((a * 2)); + +ALTER TABLE metrics ALTER COLUMN label DROP EXPRESSION; + +ALTER TABLE metrics ALTER COLUMN label SET DEFAULT 'none'; + +ALTER TABLE metrics +ADD CONSTRAINT metrics_tripled_check CHECK (tripled > 0) NOT VALID; + +-- Transaction Group #2 +ALTER TABLE metrics VALIDATE CONSTRAINT metrics_tripled_check; + +-- Transaction Group #3 +CREATE INDEX CONCURRENTLY IF NOT EXISTS metrics_tripled_idx ON metrics ((tripled + 1)) WHERE (tripled > 10); + +-- Transaction Group #4 +-- pgschema:wait +SELECT + COALESCE(i.indisvalid, false) as done, + CASE + WHEN p.blocks_total > 0 THEN p.blocks_done * 100 / p.blocks_total + ELSE 0 + END as progress +FROM pg_class c +LEFT JOIN pg_index i ON c.oid = i.indexrelid +LEFT JOIN pg_stat_progress_create_index p ON c.oid = p.index_relid +WHERE c.relname = 'metrics_tripled_idx'; + +-- Transaction Group #5 +ALTER TABLE metric_refs +ADD CONSTRAINT metric_refs_tripled_fkey FOREIGN KEY (tripled) REFERENCES metrics (tripled) NOT VALID; + +-- Transaction Group #6 +ALTER TABLE metric_refs VALIDATE CONSTRAINT metric_refs_tripled_fkey; diff --git a/testdata/diff/create_table/issue_591_alter_generated_virtual/diff.sql b/testdata/diff/create_table/issue_591_alter_generated_virtual/diff.sql new file mode 100644 index 00000000..9fd07a75 --- /dev/null +++ b/testdata/diff/create_table/issue_591_alter_generated_virtual/diff.sql @@ -0,0 +1,9 @@ +ALTER TABLE vt DROP COLUMN v2; + +ALTER TABLE vt DROP COLUMN s1; + +ALTER TABLE vt ADD COLUMN v2 integer; + +ALTER TABLE vt ADD COLUMN s1 integer GENERATED ALWAYS AS ((a * 2)) VIRTUAL; + +ALTER TABLE vt ALTER COLUMN v1 SET EXPRESSION AS ((a * 2)); diff --git a/testdata/diff/create_table/issue_591_alter_generated_virtual/new.sql b/testdata/diff/create_table/issue_591_alter_generated_virtual/new.sql new file mode 100644 index 00000000..2859e390 --- /dev/null +++ b/testdata/diff/create_table/issue_591_alter_generated_virtual/new.sql @@ -0,0 +1,6 @@ +CREATE TABLE public.vt ( + a integer NOT NULL, + v1 integer GENERATED ALWAYS AS (a * 2) VIRTUAL, + v2 integer, + s1 integer GENERATED ALWAYS AS (a * 2) VIRTUAL +); diff --git a/testdata/diff/create_table/issue_591_alter_generated_virtual/old.sql b/testdata/diff/create_table/issue_591_alter_generated_virtual/old.sql new file mode 100644 index 00000000..97615873 --- /dev/null +++ b/testdata/diff/create_table/issue_591_alter_generated_virtual/old.sql @@ -0,0 +1,6 @@ +CREATE TABLE public.vt ( + a integer NOT NULL, + v1 integer GENERATED ALWAYS AS (a * 3) VIRTUAL, + v2 integer GENERATED ALWAYS AS (a * 2) VIRTUAL, + s1 integer GENERATED ALWAYS AS (a * 2) STORED +); diff --git a/testdata/diff/create_table/issue_591_alter_generated_virtual/plan.json b/testdata/diff/create_table/issue_591_alter_generated_virtual/plan.json new file mode 100644 index 00000000..45736538 --- /dev/null +++ b/testdata/diff/create_table/issue_591_alter_generated_virtual/plan.json @@ -0,0 +1,44 @@ +{ + "version": "1.0.0", + "pgschema_version": "1.13.0", + "created_at": "1970-01-01T00:00:00Z", + "source_fingerprint": { + "hash": "78438bf9ab91cda45c838d3d085890bbc5d757a8d67a01f5585451e56f7521e4" + }, + "groups": [ + { + "steps": [ + { + "sql": "ALTER TABLE vt DROP COLUMN v2;", + "type": "table.column", + "operation": "drop", + "path": "public.vt.v2" + }, + { + "sql": "ALTER TABLE vt DROP COLUMN s1;", + "type": "table.column", + "operation": "drop", + "path": "public.vt.s1" + }, + { + "sql": "ALTER TABLE vt ADD COLUMN v2 integer;", + "type": "table.column", + "operation": "create", + "path": "public.vt.v2" + }, + { + "sql": "ALTER TABLE vt ADD COLUMN s1 integer GENERATED ALWAYS AS ((a * 2)) VIRTUAL;", + "type": "table.column", + "operation": "create", + "path": "public.vt.s1" + }, + { + "sql": "ALTER TABLE vt ALTER COLUMN v1 SET EXPRESSION AS ((a * 2));", + "type": "table.column", + "operation": "alter", + "path": "public.vt.v1" + } + ] + } + ] +} diff --git a/testdata/diff/create_table/issue_591_alter_generated_virtual/plan.sql b/testdata/diff/create_table/issue_591_alter_generated_virtual/plan.sql new file mode 100644 index 00000000..9fd07a75 --- /dev/null +++ b/testdata/diff/create_table/issue_591_alter_generated_virtual/plan.sql @@ -0,0 +1,9 @@ +ALTER TABLE vt DROP COLUMN v2; + +ALTER TABLE vt DROP COLUMN s1; + +ALTER TABLE vt ADD COLUMN v2 integer; + +ALTER TABLE vt ADD COLUMN s1 integer GENERATED ALWAYS AS ((a * 2)) VIRTUAL; + +ALTER TABLE vt ALTER COLUMN v1 SET EXPRESSION AS ((a * 2)); diff --git a/testdata/diff/create_table/issue_591_alter_generated_virtual/plan.txt b/testdata/diff/create_table/issue_591_alter_generated_virtual/plan.txt new file mode 100644 index 00000000..5539125a --- /dev/null +++ b/testdata/diff/create_table/issue_591_alter_generated_virtual/plan.txt @@ -0,0 +1,25 @@ +Plan: 1 to modify. + +Summary by type: + tables: 1 to modify + +Tables: + ~ vt + - s1 (column) + + s1 (column) + ~ v1 (column) + - v2 (column) + + v2 (column) + +DDL to be executed: +-------------------------------------------------- + +ALTER TABLE vt DROP COLUMN v2; + +ALTER TABLE vt DROP COLUMN s1; + +ALTER TABLE vt ADD COLUMN v2 integer; + +ALTER TABLE vt ADD COLUMN s1 integer GENERATED ALWAYS AS ((a * 2)) VIRTUAL; + +ALTER TABLE vt ALTER COLUMN v1 SET EXPRESSION AS ((a * 2)); From 7ab3aa6405894d0914ac5e46662db76dd51387ba Mon Sep 17 00:00:00 2001 From: tianzhou Date: Wed, 9 Sep 2026 04:03:12 -0700 Subject: [PATCH 02/11] fix: re-create dependents of a replaced generated column and normalize its expression (#591) Review follow-ups for the generated-column change: - Strip same-schema qualifiers from GeneratedExpr in the IR normalizer, as is already done for defaults and index expressions. pg_get_expr qualifies a same-schema function depending on the inspecting session's search_path, so the current state read public.calc_priority() while the desired state read calc_priority(), and the second plan re-emitted SET EXPRESSION AS (dependency/table_fk_to_generated_column idempotency failure in CI). - Views that read a re-created column are put through the existing pre-drop/recreate cycle even when unchanged, including their transitive dependents; otherwise DROP COLUMN fails with SQLSTATE 2BP01. - Column grants touching a re-created column are left out of the old state so the desired grant is re-issued after the column exists again. - Foreign keys bound to a standalone unique index on a re-created column go through the #439 pre-drop/post-add path like FKs bound to a constraint. - The implicit-drop decision for a same-named index inspects the old definition, so an index moving onto the re-created column takes the normal drop + add path instead of leaving the stale index in place. - Quoted identifiers with embedded quotes are matched in their "a""b" form. Co-Authored-By: Claude Fable 5.1 --- internal/diff/column.go | 5 +- internal/diff/diff.go | 85 +++++++-- internal/diff/generated_column_test.go | 4 +- internal/diff/table.go | 38 +++- ir/normalize.go | 14 +- .../diff.sql | 34 ++++ .../new.sql | 32 ++++ .../old.sql | 32 ++++ .../plan.json | 162 ++++++++++++++++++ .../plan.sql | 62 +++++++ .../plan.txt | 100 +++++++++++ .../table_fk_to_generated_column/diff.sql | 2 +- 12 files changed, 549 insertions(+), 21 deletions(-) create mode 100644 testdata/diff/create_table/issue_591_recreate_generated_dependents/diff.sql create mode 100644 testdata/diff/create_table/issue_591_recreate_generated_dependents/new.sql create mode 100644 testdata/diff/create_table/issue_591_recreate_generated_dependents/old.sql create mode 100644 testdata/diff/create_table/issue_591_recreate_generated_dependents/plan.json create mode 100644 testdata/diff/create_table/issue_591_recreate_generated_dependents/plan.sql create mode 100644 testdata/diff/create_table/issue_591_recreate_generated_dependents/plan.txt diff --git a/internal/diff/column.go b/internal/diff/column.go index 6b9c50da..75603420 100644 --- a/internal/diff/column.go +++ b/internal/diff/column.go @@ -285,7 +285,10 @@ func generatedColumnNeedsRecreate(old, new *ir.Column, targetMajorVersion int) b // plain -> generated return true case !new.IsGenerated: - // generated -> plain: DROP EXPRESSION only works on STORED columns + // generated -> plain: DROP EXPRESSION only works on STORED columns. + // A VIRTUAL column has no stored values to keep (which is why + // PostgreSQL refuses DROP EXPRESSION for it), so the re-created + // plain column starts out NULL; the plan shows it as dropped + added. return old.GeneratedKind == "v" case old.GeneratedKind != new.GeneratedKind: return true diff --git a/internal/diff/diff.go b/internal/diff/diff.go index 18ef3257..65fae07c 100644 --- a/internal/diff/diff.go +++ b/internal/diff/diff.go @@ -434,14 +434,20 @@ type tableDiff struct { AddedTriggers []*ir.Trigger DroppedTriggers []*ir.Trigger ModifiedTriggers []*triggerDiff - AddedPolicies []*ir.RLSPolicy - DroppedPolicies []*ir.RLSPolicy - ModifiedPolicies []*policyDiff - RLSChanges []*rlsChange - CommentChanged bool - OldComment string - NewComment string - PersistenceChanged bool + // RecreatedColumns names columns applied as DROP COLUMN + ADD COLUMN + // because their generation clause cannot be altered in place (issue #591). + // They appear in both DroppedColumns and AddedColumns; dependent objects + // outside the table (views, column grants, FKs bound to a unique index) + // are re-created from this set. + RecreatedColumns map[string]bool + AddedPolicies []*ir.RLSPolicy + DroppedPolicies []*ir.RLSPolicy + ModifiedPolicies []*policyDiff + RLSChanges []*rlsChange + CommentChanged bool + OldComment string + NewComment string + PersistenceChanged bool } // ColumnDiff represents changes to a column @@ -659,6 +665,9 @@ func generateMigration(oldIR, newIR *ir.IR, targetSchema string, qualifySchema b diff.allNewTables = newTables + // Columns re-created by this migration, keyed by schema.table (issue #591) + recreatedColumnsByTable := collectRecreatedColumns(diff.modifiedTables) + // Compare rows of data-managed tables diff.dataDiffs = diffTableData(oldTables, newTables) @@ -935,6 +944,10 @@ func generateMigration(oldIR, newIR *ir.IR, targetSchema string, qualifySchema b newView := newViews[key] if oldView, exists := oldViews[key]; exists { structurallyDifferent := !viewsEqual(oldView, newView) + // A view that reads a column being re-created (DROP COLUMN + ADD + // COLUMN) would block the DROP with SQLSTATE 2BP01, so it goes + // through the pre-drop/recreate cycle even when unchanged (#591). + dependsOnRecreated := viewDependsOnRecreatedColumn(newView, recreatedColumnsByTable) // Check if the view definition itself changed (excluding options). // This is used to decide if materialized views need DROP+CREATE: // option-only changes should use ALTER VIEW SET/RESET, not recreation. @@ -985,13 +998,14 @@ func generateMigration(oldIR, newIR *ir.IR, targetSchema string, qualifySchema b addedTriggers, droppedTriggers, modifiedTriggers := diffViewTriggers(oldView, newView) triggersChanged := len(addedTriggers) > 0 || len(droppedTriggers) > 0 || len(modifiedTriggers) > 0 - if structurallyDifferent || commentChanged || indexesChanged || triggersChanged { + if structurallyDifferent || commentChanged || indexesChanged || triggersChanged || dependsOnRecreated { // For materialized views with definition changes, mark for recreation. // For regular views with column changes incompatible with CREATE OR REPLACE VIEW, // also mark for recreation (issue #308). // Use definitionChanged (not structurallyDifferent) so that option-only changes // on materialized views use ALTER SET/RESET instead of DROP+CREATE. - needsRecreate := definitionChanged && (newView.Materialized || viewColumnsRequireRecreate(oldView, newView)) + needsRecreate := dependsOnRecreated || + (definitionChanged && (newView.Materialized || viewColumnsRequireRecreate(oldView, newView))) if needsRecreate { diff.modifiedViews = append(diff.modifiedViews, &viewDiff{ @@ -1465,6 +1479,13 @@ func generateMigration(oldIR, newIR *ir.IR, targetSchema string, qualifySchema b for _, dbSchema := range oldIR.Schemas { for _, cp := range dbSchema.ColumnPrivileges { + // DROP COLUMN discards the column's ACL, so a grant touching a + // re-created column is gone once the migration runs; leaving it + // out of the old state makes the desired grant come back as an + // addition after the column exists again (#591). + if columnPrivilegeTouchesColumns(cp, recreatedColumnsByTable[dbSchema.Name+"."+cp.TableName]) { + continue + } key := cp.GetFullKey() oldColPrivs[key] = cp } @@ -2364,6 +2385,50 @@ func filterPreDroppedViews(views []*ir.View, preDropped map[string]bool) []*ir.V return filtered } +// collectRecreatedColumns indexes the columns re-created by this migration +// (see tableDiff.RecreatedColumns) by their schema.table key. (#591) +func collectRecreatedColumns(modifiedTables []*tableDiff) map[string]map[string]bool { + byTable := make(map[string]map[string]bool) + for _, td := range modifiedTables { + if len(td.RecreatedColumns) > 0 { + byTable[td.Table.Schema+"."+td.Table.Name] = td.RecreatedColumns + } + } + return byTable +} + +// viewDependsOnRecreatedColumn reports whether a view reads a column that this +// migration re-creates, in which case the view must be dropped before the +// column and created again afterwards. The column check is a textual match on +// the view definition, so a same-named column of another table the view also +// reads can cause a redundant recreation. (#591) +func viewDependsOnRecreatedColumn(view *ir.View, recreatedColumnsByTable map[string]map[string]bool) bool { + for tableKey, columns := range recreatedColumnsByTable { + schema, table, ok := strings.Cut(tableKey, ".") + if !ok { + continue + } + if viewDependsOnTable(view, schema, table) && exprReferencesAnyColumn(view.Definition, columns) { + return true + } + } + return false +} + +// columnPrivilegeTouchesColumns reports whether a column grant covers any of +// the given columns. +func columnPrivilegeTouchesColumns(cp *ir.ColumnPrivilege, columns map[string]bool) bool { + if len(columns) == 0 { + return false + } + for _, name := range cp.Columns { + if columns[name] { + return true + } + } + return false +} + // getTableNameWithSchema returns the table name with schema qualification only when necessary // If the table schema is different from the target schema, it returns "schema.table" // If they are the same, it returns just "table" diff --git a/internal/diff/generated_column_test.go b/internal/diff/generated_column_test.go index bc190e6e..bd2cbbb1 100644 --- a/internal/diff/generated_column_test.go +++ b/internal/diff/generated_column_test.go @@ -77,7 +77,7 @@ func TestGeneratedExpressionChange_VersionGate(t *testing.T) { } func TestExprReferencesAnyColumn(t *testing.T) { - cols := map[string]bool{"b": true, "my col": true} + cols := map[string]bool{"b": true, "my col": true, `a"b`: true} cases := []struct { expr string want bool @@ -85,6 +85,8 @@ func TestExprReferencesAnyColumn(t *testing.T) { {"(b + 1)", true}, {"b", true}, {"(\"my col\" * 2)", true}, + {`("a""b" + 1)`, true}, // embedded quote doubled by pg_get_expr + {`("a"b" + 1)`, false}, {"(b > 10)", true}, {"(bb + 1)", false}, {"(a + 1)", false}, diff --git a/internal/diff/table.go b/internal/diff/table.go index 210cb7bf..87f7082c 100644 --- a/internal/diff/table.go +++ b/internal/diff/table.go @@ -209,6 +209,10 @@ func diffTables(oldTable, newTable *ir.Table, targetSchema string, targetMajorVe } } + if len(recreatedColumns) > 0 { + diff.RecreatedColumns = recreatedColumns + } + // Compare constraints oldConstraints := make(map[string]*ir.Constraint) newConstraints := make(map[string]*ir.Constraint) @@ -296,8 +300,11 @@ func diffTables(oldTable, newTable *ir.Table, targetSchema string, targetMajorVe for name, newIndex := range newIndexes { if oldIndex, exists := oldIndexes[name]; exists { // DROP COLUMN removes every index on a re-created column, so the - // desired-state index is created afresh afterwards. (#591) - if indexReferencesColumns(newIndex, recreatedColumns) { + // desired-state index is created afresh afterwards. The old + // definition decides: a same-named index that moves from another + // column onto the re-created one still exists and takes the normal + // drop + add path below. (#591) + if indexReferencesColumns(oldIndex, recreatedColumns) { diff.AddedIndexes = append(diff.AddedIndexes, newIndex) continue } @@ -652,8 +659,20 @@ func generateModifyTablesSQL(diffs []*tableDiff, droppedTables []*ir.Table, fkPr func planFKRecreationForReplacedConstraints(modifiedTables []*tableDiff, addedTables []*ir.Table, oldTables, newTables map[string]*ir.Table) (preDrops []*ir.Constraint, postAdds []*deferredConstraint, suppressedInlineFKs map[string]bool) { // Unique/PK constraints removed by this migration, keyed by their table replaced := make(map[string][]*ir.Constraint) + // Standalone unique indexes that DROP COLUMN removes along with a + // re-created column; a foreign key can be bound to such an index just like + // to a unique constraint. (#591) + replacedUniqueIndexes := make(map[string][]*ir.Index) for _, td := range modifiedTables { key := td.Table.Schema + "." + td.Table.Name + if oldTable := oldTables[key]; oldTable != nil && len(td.RecreatedColumns) > 0 { + for _, name := range sortedKeys(oldTable.Indexes) { + idx := oldTable.Indexes[name] + if idx.Type == ir.IndexTypeUnique && !idx.IsPartial && indexReferencesColumns(idx, td.RecreatedColumns) { + replacedUniqueIndexes[key] = append(replacedUniqueIndexes[key], idx) + } + } + } for _, c := range td.DroppedConstraints { if c.Type == ir.ConstraintTypeUnique || c.Type == ir.ConstraintTypePrimaryKey { replaced[key] = append(replaced[key], c) @@ -686,7 +705,7 @@ func planFKRecreationForReplacedConstraints(modifiedTables []*tableDiff, addedTa } } - if len(replaced) == 0 && len(addedConstraints) == 0 && len(addedUniqueIndexes) == 0 { + if len(replaced) == 0 && len(replacedUniqueIndexes) == 0 && len(addedConstraints) == 0 && len(addedUniqueIndexes) == 0 { return nil, nil, nil } @@ -703,12 +722,14 @@ func planFKRecreationForReplacedConstraints(modifiedTables []*tableDiff, addedTa continue } newFK := newTable.Constraints[name] - oldBound := fkReferencesAnyConstraint(fk, replaced[fkReferencedTableKey(fk)]) + oldBound := fkReferencesAnyConstraint(fk, replaced[fkReferencedTableKey(fk)]) || + fkReferencesAnyUniqueIndex(fk, replacedUniqueIndexes[fkReferencedTableKey(fk)]) // A changed FK whose new definition targets a replaced constraint // must also wait for the replacement, even if its old definition // was bound elsewhere. newBound := newFK != nil && !constraintsEqual(fk, newFK) && - fkReferencesAnyConstraint(newFK, replaced[fkReferencedTableKey(newFK)]) + (fkReferencesAnyConstraint(newFK, replaced[fkReferencedTableKey(newFK)]) || + fkReferencesAnyUniqueIndex(newFK, replacedUniqueIndexes[fkReferencedTableKey(newFK)])) if !oldBound && !newBound { continue } @@ -732,6 +753,7 @@ func planFKRecreationForReplacedConstraints(modifiedTables []*tableDiff, addedTa } refKey := fkReferencedTableKey(fk) if !fkReferencesAnyConstraint(fk, replaced[refKey]) && + !fkReferencesAnyUniqueIndex(fk, replacedUniqueIndexes[refKey]) && !fkReferencesAnyConstraint(fk, addedConstraints[refKey]) && !fkReferencesAnyUniqueIndex(fk, addedUniqueIndexes[refKey]) { continue @@ -1117,8 +1139,10 @@ func exprReferencesAnyColumn(expr string, columns map[string]bool) bool { } expr = sqlStringLiteralRegex.ReplaceAllString(expr, "''") for column := range columns { - q := regexp.QuoteMeta(column) - re := regexp.MustCompile(`(?:^|[^\w"])(?:` + q + `|"` + q + `")(?:[^\w"(]|$)`) + bare := regexp.QuoteMeta(column) + // pg_get_expr doubles embedded quotes inside a quoted identifier. + quoted := regexp.QuoteMeta(`"` + strings.ReplaceAll(column, `"`, `""`) + `"`) + re := regexp.MustCompile(`(?:^|[^\w"])(?:` + bare + `|` + quoted + `)(?:[^\w"(]|$)`) if re.MatchString(expr) { return true } diff --git a/ir/normalize.go b/ir/normalize.go index 156eec75..e825e860 100644 --- a/ir/normalize.go +++ b/ir/normalize.go @@ -186,7 +186,19 @@ func normalizeTable(table *Table) { // normalizeColumn normalizes column default values // tableSchema is used to strip same-schema qualifiers from function calls func normalizeColumn(column *Column, tableSchema string) { - if column == nil || column.DefaultValue == nil { + if column == nil { + return + } + + // pg_get_expr qualifies same-schema functions and types in a generation + // expression depending on the inspecting session's search_path; strip the + // qualifier so current and desired state compare textually (issue #591). + if column.GeneratedExpr != nil && tableSchema != "" { + stripped := StripSchemaPrefixFromBody(*column.GeneratedExpr, tableSchema) + column.GeneratedExpr = &stripped + } + + if column.DefaultValue == nil { return } diff --git a/testdata/diff/create_table/issue_591_recreate_generated_dependents/diff.sql b/testdata/diff/create_table/issue_591_recreate_generated_dependents/diff.sql new file mode 100644 index 00000000..e67203a6 --- /dev/null +++ b/testdata/diff/create_table/issue_591_recreate_generated_dependents/diff.sql @@ -0,0 +1,34 @@ +DROP VIEW IF EXISTS big_orders RESTRICT; + +DROP VIEW IF EXISTS order_totals RESTRICT; + +ALTER TABLE shipments DROP CONSTRAINT shipments_order_code_fkey; + +ALTER TABLE orders DROP COLUMN total; + +ALTER TABLE orders DROP COLUMN code; + +ALTER TABLE orders ADD COLUMN total integer GENERATED ALWAYS AS ((qty * price)) STORED; + +ALTER TABLE orders ADD COLUMN code text GENERATED ALWAYS AS (('ORD-'::text || (id)::text)) STORED; + +DROP INDEX IF EXISTS orders_lookup_idx; + +CREATE INDEX IF NOT EXISTS orders_lookup_idx ON orders (total); + +CREATE UNIQUE INDEX IF NOT EXISTS orders_code_key ON orders (code); + +ALTER TABLE shipments +ADD CONSTRAINT shipments_order_code_fkey FOREIGN KEY (order_code) REFERENCES orders (code); + +CREATE OR REPLACE VIEW order_totals AS + SELECT id, + total + FROM orders; + +CREATE OR REPLACE VIEW big_orders AS + SELECT id + FROM order_totals + WHERE total > 100; + +GRANT SELECT (id, total) ON TABLE orders TO app_reader; diff --git a/testdata/diff/create_table/issue_591_recreate_generated_dependents/new.sql b/testdata/diff/create_table/issue_591_recreate_generated_dependents/new.sql new file mode 100644 index 00000000..1ed8ce03 --- /dev/null +++ b/testdata/diff/create_table/issue_591_recreate_generated_dependents/new.sql @@ -0,0 +1,32 @@ +DO $$ +BEGIN + IF NOT EXISTS (SELECT 1 FROM pg_roles WHERE rolname = 'app_reader') THEN + CREATE ROLE app_reader; + END IF; +END $$; + +CREATE TABLE public.orders ( + id integer PRIMARY KEY, + qty integer NOT NULL, + price integer NOT NULL, + total integer GENERATED ALWAYS AS (qty * price) STORED, + code text GENERATED ALWAYS AS ('ORD-' || id::text) STORED +); + +CREATE UNIQUE INDEX orders_code_key ON public.orders (code); + +CREATE INDEX orders_lookup_idx ON public.orders (total); + +CREATE TABLE public.shipments ( + id integer PRIMARY KEY, + order_code text, + CONSTRAINT shipments_order_code_fkey FOREIGN KEY (order_code) REFERENCES public.orders (code) +); + +CREATE VIEW public.order_totals AS SELECT id, total FROM public.orders; + +CREATE VIEW public.big_orders AS SELECT id FROM public.order_totals WHERE total > 100; + +CREATE VIEW public.order_prices AS SELECT id, price FROM public.orders; + +GRANT SELECT (id, total) ON public.orders TO app_reader; diff --git a/testdata/diff/create_table/issue_591_recreate_generated_dependents/old.sql b/testdata/diff/create_table/issue_591_recreate_generated_dependents/old.sql new file mode 100644 index 00000000..c6bdf5dd --- /dev/null +++ b/testdata/diff/create_table/issue_591_recreate_generated_dependents/old.sql @@ -0,0 +1,32 @@ +DO $$ +BEGIN + IF NOT EXISTS (SELECT 1 FROM pg_roles WHERE rolname = 'app_reader') THEN + CREATE ROLE app_reader; + END IF; +END $$; + +CREATE TABLE public.orders ( + id integer PRIMARY KEY, + qty integer NOT NULL, + price integer NOT NULL, + total integer, + code text +); + +CREATE UNIQUE INDEX orders_code_key ON public.orders (code); + +CREATE INDEX orders_lookup_idx ON public.orders (price); + +CREATE TABLE public.shipments ( + id integer PRIMARY KEY, + order_code text, + CONSTRAINT shipments_order_code_fkey FOREIGN KEY (order_code) REFERENCES public.orders (code) +); + +CREATE VIEW public.order_totals AS SELECT id, total FROM public.orders; + +CREATE VIEW public.big_orders AS SELECT id FROM public.order_totals WHERE total > 100; + +CREATE VIEW public.order_prices AS SELECT id, price FROM public.orders; + +GRANT SELECT (id, total) ON public.orders TO app_reader; diff --git a/testdata/diff/create_table/issue_591_recreate_generated_dependents/plan.json b/testdata/diff/create_table/issue_591_recreate_generated_dependents/plan.json new file mode 100644 index 00000000..f5a154d0 --- /dev/null +++ b/testdata/diff/create_table/issue_591_recreate_generated_dependents/plan.json @@ -0,0 +1,162 @@ +{ + "version": "1.0.0", + "pgschema_version": "1.13.0", + "created_at": "1970-01-01T00:00:00Z", + "source_fingerprint": { + "hash": "9d16cac1391edcd0cebffe22c50891525861d04d7f459e61de41da64255372af" + }, + "groups": [ + { + "steps": [ + { + "sql": "DROP VIEW IF EXISTS big_orders RESTRICT;", + "type": "view", + "operation": "recreate", + "path": "public.big_orders" + }, + { + "sql": "DROP VIEW IF EXISTS order_totals RESTRICT;", + "type": "view", + "operation": "recreate", + "path": "public.order_totals" + }, + { + "sql": "ALTER TABLE shipments DROP CONSTRAINT shipments_order_code_fkey;", + "type": "table.constraint", + "operation": "drop", + "path": "public.shipments.shipments_order_code_fkey" + }, + { + "sql": "ALTER TABLE orders DROP COLUMN total;", + "type": "table.column", + "operation": "drop", + "path": "public.orders.total" + }, + { + "sql": "ALTER TABLE orders DROP COLUMN code;", + "type": "table.column", + "operation": "drop", + "path": "public.orders.code" + }, + { + "sql": "ALTER TABLE orders ADD COLUMN total integer GENERATED ALWAYS AS ((qty * price)) STORED;", + "type": "table.column", + "operation": "create", + "path": "public.orders.total" + }, + { + "sql": "ALTER TABLE orders ADD COLUMN code text GENERATED ALWAYS AS (('ORD-'::text || (id)::text)) STORED;", + "type": "table.column", + "operation": "create", + "path": "public.orders.code" + } + ] + }, + { + "steps": [ + { + "sql": "CREATE INDEX CONCURRENTLY IF NOT EXISTS orders_lookup_idx_pgschema_new ON orders (total);", + "type": "table.index", + "operation": "alter", + "path": "public.orders.orders_lookup_idx" + } + ] + }, + { + "steps": [ + { + "sql": "SELECT \n COALESCE(i.indisvalid, false) as done,\n CASE \n WHEN p.blocks_total > 0 THEN p.blocks_done * 100 / p.blocks_total\n ELSE 0\n END as progress\nFROM pg_class c\nLEFT JOIN pg_index i ON c.oid = i.indexrelid\nLEFT JOIN pg_stat_progress_create_index p ON c.oid = p.index_relid\nWHERE c.relname = 'orders_lookup_idx_pgschema_new';", + "directive": { + "type": "wait", + "message": "Creating index orders_lookup_idx_pgschema_new" + }, + "type": "table.index", + "operation": "alter", + "path": "public.orders.orders_lookup_idx" + } + ] + }, + { + "steps": [ + { + "sql": "DROP INDEX IF EXISTS orders_lookup_idx;", + "type": "table.index", + "operation": "alter", + "path": "public.orders.orders_lookup_idx" + }, + { + "sql": "ALTER INDEX orders_lookup_idx_pgschema_new RENAME TO orders_lookup_idx;", + "type": "table.index", + "operation": "alter", + "path": "public.orders.orders_lookup_idx" + } + ] + }, + { + "steps": [ + { + "sql": "CREATE UNIQUE INDEX CONCURRENTLY IF NOT EXISTS orders_code_key ON orders (code);", + "type": "table.index", + "operation": "create", + "path": "public.orders.orders_code_key" + } + ] + }, + { + "steps": [ + { + "sql": "SELECT \n COALESCE(i.indisvalid, false) as done,\n CASE \n WHEN p.blocks_total > 0 THEN p.blocks_done * 100 / p.blocks_total\n ELSE 0\n END as progress\nFROM pg_class c\nLEFT JOIN pg_index i ON c.oid = i.indexrelid\nLEFT JOIN pg_stat_progress_create_index p ON c.oid = p.index_relid\nWHERE c.relname = 'orders_code_key';", + "directive": { + "type": "wait", + "message": "Creating index orders_code_key" + }, + "type": "table.index", + "operation": "create", + "path": "public.orders.orders_code_key" + } + ] + }, + { + "steps": [ + { + "sql": "ALTER TABLE shipments\nADD CONSTRAINT shipments_order_code_fkey FOREIGN KEY (order_code) REFERENCES orders (code) NOT VALID;", + "type": "table.constraint", + "operation": "create", + "path": "public.shipments.shipments_order_code_fkey" + } + ] + }, + { + "steps": [ + { + "sql": "ALTER TABLE shipments VALIDATE CONSTRAINT shipments_order_code_fkey;", + "type": "table.constraint", + "operation": "create", + "path": "public.shipments.shipments_order_code_fkey" + } + ] + }, + { + "steps": [ + { + "sql": "CREATE OR REPLACE VIEW order_totals AS\n SELECT id,\n total\n FROM orders;", + "type": "view", + "operation": "create", + "path": "public.order_totals" + }, + { + "sql": "CREATE OR REPLACE VIEW big_orders AS\n SELECT id\n FROM order_totals\n WHERE total > 100;", + "type": "view", + "operation": "recreate", + "path": "public.big_orders" + }, + { + "sql": "GRANT SELECT (id, total) ON TABLE orders TO app_reader;", + "type": "column_privilege", + "operation": "create", + "path": "column_privileges.TABLE.orders.id,total.app_reader" + } + ] + } + ] +} diff --git a/testdata/diff/create_table/issue_591_recreate_generated_dependents/plan.sql b/testdata/diff/create_table/issue_591_recreate_generated_dependents/plan.sql new file mode 100644 index 00000000..a50ba9c0 --- /dev/null +++ b/testdata/diff/create_table/issue_591_recreate_generated_dependents/plan.sql @@ -0,0 +1,62 @@ +DROP VIEW IF EXISTS big_orders RESTRICT; + +DROP VIEW IF EXISTS order_totals RESTRICT; + +ALTER TABLE shipments DROP CONSTRAINT shipments_order_code_fkey; + +ALTER TABLE orders DROP COLUMN total; + +ALTER TABLE orders DROP COLUMN code; + +ALTER TABLE orders ADD COLUMN total integer GENERATED ALWAYS AS ((qty * price)) STORED; + +ALTER TABLE orders ADD COLUMN code text GENERATED ALWAYS AS (('ORD-'::text || (id)::text)) STORED; + +CREATE INDEX CONCURRENTLY IF NOT EXISTS orders_lookup_idx_pgschema_new ON orders (total); + +-- pgschema:wait +SELECT + COALESCE(i.indisvalid, false) as done, + CASE + WHEN p.blocks_total > 0 THEN p.blocks_done * 100 / p.blocks_total + ELSE 0 + END as progress +FROM pg_class c +LEFT JOIN pg_index i ON c.oid = i.indexrelid +LEFT JOIN pg_stat_progress_create_index p ON c.oid = p.index_relid +WHERE c.relname = 'orders_lookup_idx_pgschema_new'; + +DROP INDEX IF EXISTS orders_lookup_idx; + +ALTER INDEX orders_lookup_idx_pgschema_new RENAME TO orders_lookup_idx; + +CREATE UNIQUE INDEX CONCURRENTLY IF NOT EXISTS orders_code_key ON orders (code); + +-- pgschema:wait +SELECT + COALESCE(i.indisvalid, false) as done, + CASE + WHEN p.blocks_total > 0 THEN p.blocks_done * 100 / p.blocks_total + ELSE 0 + END as progress +FROM pg_class c +LEFT JOIN pg_index i ON c.oid = i.indexrelid +LEFT JOIN pg_stat_progress_create_index p ON c.oid = p.index_relid +WHERE c.relname = 'orders_code_key'; + +ALTER TABLE shipments +ADD CONSTRAINT shipments_order_code_fkey FOREIGN KEY (order_code) REFERENCES orders (code) NOT VALID; + +ALTER TABLE shipments VALIDATE CONSTRAINT shipments_order_code_fkey; + +CREATE OR REPLACE VIEW order_totals AS + SELECT id, + total + FROM orders; + +CREATE OR REPLACE VIEW big_orders AS + SELECT id + FROM order_totals + WHERE total > 100; + +GRANT SELECT (id, total) ON TABLE orders TO app_reader; diff --git a/testdata/diff/create_table/issue_591_recreate_generated_dependents/plan.txt b/testdata/diff/create_table/issue_591_recreate_generated_dependents/plan.txt new file mode 100644 index 00000000..5b518bc2 --- /dev/null +++ b/testdata/diff/create_table/issue_591_recreate_generated_dependents/plan.txt @@ -0,0 +1,100 @@ +Plan: 1 to add, 4 to modify. + +Summary by type: + tables: 2 to modify + views: 2 to modify + column privileges: 1 to add + +Tables: + ~ orders + - code (column) + + code (column) + - total (column) + + total (column) + + orders_code_key (index) + ~ orders_lookup_idx (index - concurrent rebuild) + ~ shipments + - shipments_order_code_fkey (constraint) + + shipments_order_code_fkey (constraint) + +Views: + ~ big_orders + ~ order_totals + +Column privileges: + + app_reader + +DDL to be executed: +-------------------------------------------------- + +-- Transaction Group #1 +DROP VIEW IF EXISTS big_orders RESTRICT; + +DROP VIEW IF EXISTS order_totals RESTRICT; + +ALTER TABLE shipments DROP CONSTRAINT shipments_order_code_fkey; + +ALTER TABLE orders DROP COLUMN total; + +ALTER TABLE orders DROP COLUMN code; + +ALTER TABLE orders ADD COLUMN total integer GENERATED ALWAYS AS ((qty * price)) STORED; + +ALTER TABLE orders ADD COLUMN code text GENERATED ALWAYS AS (('ORD-'::text || (id)::text)) STORED; + +-- Transaction Group #2 +CREATE INDEX CONCURRENTLY IF NOT EXISTS orders_lookup_idx_pgschema_new ON orders (total); + +-- Transaction Group #3 +-- pgschema:wait +SELECT + COALESCE(i.indisvalid, false) as done, + CASE + WHEN p.blocks_total > 0 THEN p.blocks_done * 100 / p.blocks_total + ELSE 0 + END as progress +FROM pg_class c +LEFT JOIN pg_index i ON c.oid = i.indexrelid +LEFT JOIN pg_stat_progress_create_index p ON c.oid = p.index_relid +WHERE c.relname = 'orders_lookup_idx_pgschema_new'; + +-- Transaction Group #4 +DROP INDEX IF EXISTS orders_lookup_idx; + +ALTER INDEX orders_lookup_idx_pgschema_new RENAME TO orders_lookup_idx; + +-- Transaction Group #5 +CREATE UNIQUE INDEX CONCURRENTLY IF NOT EXISTS orders_code_key ON orders (code); + +-- Transaction Group #6 +-- pgschema:wait +SELECT + COALESCE(i.indisvalid, false) as done, + CASE + WHEN p.blocks_total > 0 THEN p.blocks_done * 100 / p.blocks_total + ELSE 0 + END as progress +FROM pg_class c +LEFT JOIN pg_index i ON c.oid = i.indexrelid +LEFT JOIN pg_stat_progress_create_index p ON c.oid = p.index_relid +WHERE c.relname = 'orders_code_key'; + +-- Transaction Group #7 +ALTER TABLE shipments +ADD CONSTRAINT shipments_order_code_fkey FOREIGN KEY (order_code) REFERENCES orders (code) NOT VALID; + +-- Transaction Group #8 +ALTER TABLE shipments VALIDATE CONSTRAINT shipments_order_code_fkey; + +-- Transaction Group #9 +CREATE OR REPLACE VIEW order_totals AS + SELECT id, + total + FROM orders; + +CREATE OR REPLACE VIEW big_orders AS + SELECT id + FROM order_totals + WHERE total > 100; + +GRANT SELECT (id, total) ON TABLE orders TO app_reader; diff --git a/testdata/diff/dependency/table_fk_to_generated_column/diff.sql b/testdata/diff/dependency/table_fk_to_generated_column/diff.sql index ab07d9d7..7df1404b 100644 --- a/testdata/diff/dependency/table_fk_to_generated_column/diff.sql +++ b/testdata/diff/dependency/table_fk_to_generated_column/diff.sql @@ -15,7 +15,7 @@ $$; CREATE TABLE IF NOT EXISTS article ( id integer, title text NOT NULL, - priority integer GENERATED ALWAYS AS (public.calc_priority()) STORED, + priority integer GENERATED ALWAYS AS (calc_priority()) STORED, CONSTRAINT article_pkey PRIMARY KEY (id) ); From 1a7365be4e0e3bfb7eeffe1dd9432164d88d9fad Mon Sep 17 00:00:00 2001 From: tianzhou Date: Wed, 9 Sep 2026 04:23:32 -0700 Subject: [PATCH 03/11] fix: drop policies, triggers, and view grants around a replaced generated column (#591) Second round of review follow-ups for the column recreation path: - Dependency detection for views uses the live (old) definition, so a view whose new definition no longer reads the column is still pre-dropped. - Policies and triggers whose expressions (USING / WITH CHECK, WHEN, UPDATE OF) name a re-created column block DROP COLUMN with SQLSTATE 2BP01. They are now dropped ahead of the column (policies just before the table changes, triggers in the drop phase) and created again from the desired state. - Grants on views this migration drops and creates again (root recreations and their transitive dependents) are left out of the old state so they are re-issued after the views exist again. - $ counts as an identifier character in the expression matcher. Co-Authored-By: Claude Fable 5.1 --- internal/diff/diff.go | 38 +++++++- internal/diff/generated_column_test.go | 2 + internal/diff/table.go | 86 ++++++++++++++++--- .../diff.sql | 21 +++++ .../new.sql | 14 +++ .../old.sql | 14 +++ .../plan.json | 44 +++++++++- .../plan.sql | 21 +++++ .../plan.txt | 34 +++++++- 9 files changed, 256 insertions(+), 18 deletions(-) diff --git a/internal/diff/diff.go b/internal/diff/diff.go index 65fae07c..738e6e01 100644 --- a/internal/diff/diff.go +++ b/internal/diff/diff.go @@ -947,7 +947,8 @@ func generateMigration(oldIR, newIR *ir.IR, targetSchema string, qualifySchema b // A view that reads a column being re-created (DROP COLUMN + ADD // COLUMN) would block the DROP with SQLSTATE 2BP01, so it goes // through the pre-drop/recreate cycle even when unchanged (#591). - dependsOnRecreated := viewDependsOnRecreatedColumn(newView, recreatedColumnsByTable) + // The live (old) definition is what holds the dependency. + dependsOnRecreated := viewDependsOnRecreatedColumn(oldView, recreatedColumnsByTable) // Check if the view definition itself changed (excluding options). // This is used to decide if materialized views need DROP+CREATE: // option-only changes should use ALTER VIEW SET/RESET, not recreation. @@ -1082,6 +1083,11 @@ func generateMigration(oldIR, newIR *ir.IR, targetSchema string, qualifySchema b // Store all new views for dependent view handling (issue #268) diff.allNewViews = newViews + // Views this migration drops and creates again (root recreations and + // their transitive dependents), keyed by schema.name. DROP VIEW discards + // the view's ACL, so their grants are re-issued below (#591). + recreatedViewKeys := collectRecreatedViewKeys(diff.allNewViews, diff.modifiedViews, diff.addedViews) + // Compare sequences across all schemas oldSequences := make(map[string]*ir.Sequence) newSequences := make(map[string]*ir.Sequence) @@ -1284,6 +1290,14 @@ func generateMigration(oldIR, newIR *ir.IR, targetSchema string, qualifySchema b for _, dbSchema := range oldIR.Schemas { for _, p := range dbSchema.Privileges { + // A grant on a view that is dropped and created again by this + // migration does not survive; leaving it out of the old state + // makes the desired grant come back as an addition, emitted after + // the views are recreated (#591). + if (p.ObjectType == ir.PrivilegeObjectTypeTable || p.ObjectType == ir.PrivilegeObjectTypeView) && + recreatedViewKeys[dbSchema.Name+"."+p.ObjectName] { + continue + } key := p.GetFullKey() oldPrivs[key] = p } @@ -2415,6 +2429,28 @@ func viewDependsOnRecreatedColumn(view *ir.View, recreatedColumnsByTable map[str return false } +// collectRecreatedViewKeys returns the schema.name keys of every view the +// migration drops and creates again: views marked RequiresRecreate and the +// views that transitively depend on them (see generateModifyViewsSQL). +func collectRecreatedViewKeys(allNewViews map[string]*ir.View, modifiedViews []*viewDiff, addedViews []*ir.View) map[string]bool { + keys := make(map[string]bool) + for _, vd := range modifiedViews { + if vd.RequiresRecreate { + keys[vd.New.Schema+"."+vd.New.Name] = true + } + } + if len(keys) == 0 { + return keys + } + ctx := findDependentViewsForRecreatedViews(allNewViews, modifiedViews, addedViews) + for _, dependents := range ctx.dependents { + for _, view := range dependents { + keys[view.Schema+"."+view.Name] = true + } + } + return keys +} + // columnPrivilegeTouchesColumns reports whether a column grant covers any of // the given columns. func columnPrivilegeTouchesColumns(cp *ir.ColumnPrivilege, columns map[string]bool) bool { diff --git a/internal/diff/generated_column_test.go b/internal/diff/generated_column_test.go index bd2cbbb1..afc372b3 100644 --- a/internal/diff/generated_column_test.go +++ b/internal/diff/generated_column_test.go @@ -87,6 +87,8 @@ func TestExprReferencesAnyColumn(t *testing.T) { {"(\"my col\" * 2)", true}, {`("a""b" + 1)`, true}, // embedded quote doubled by pg_get_expr {`("a"b" + 1)`, false}, + {"(b$x + 1)", false}, // $ is part of the identifier + {"(x$b + 1)", false}, {"(b > 10)", true}, {"(bb + 1)", false}, {"(a + 1)", false}, diff --git a/internal/diff/table.go b/internal/diff/table.go index 87f7082c..96bf23c1 100644 --- a/internal/diff/table.go +++ b/internal/diff/table.go @@ -90,7 +90,9 @@ func sortConstraintColumnsByPosition(columns []*ir.ConstraintColumn) []*ir.Const } // diffTriggers compares triggers between two tables and populates the diff -func diffTriggers(oldTable, newTable *ir.Table, diff *tableDiff) { +// recreatedColumns names the columns applied as DROP COLUMN + ADD COLUMN; a +// trigger depending on one of them is dropped and created again. (#591) +func diffTriggers(oldTable, newTable *ir.Table, diff *tableDiff, recreatedColumns map[string]bool) { oldTriggers := make(map[string]*ir.Trigger) newTriggers := make(map[string]*ir.Trigger) @@ -123,6 +125,15 @@ func diffTriggers(oldTable, newTable *ir.Table, diff *tableDiff) { // Find modified triggers (structural changes, comment-only, or enabled-state-only) for name, newTrigger := range newTriggers { if oldTrigger, exists := oldTriggers[name]; exists { + // A trigger whose WHEN condition or UPDATE OF list names a column + // being re-created blocks the DROP COLUMN (SQLSTATE 2BP01). It is + // dropped in the drop phase and created again from the desired + // state after the column is back. (#591) + if triggerReferencesColumns(oldTrigger, recreatedColumns) { + diff.DroppedTriggers = append(diff.DroppedTriggers, oldTrigger) + diff.AddedTriggers = append(diff.AddedTriggers, newTrigger) + continue + } structurallyEqual := triggersEqual(oldTrigger, newTrigger) commentChanged := oldTrigger.Comment != newTrigger.Comment enabledChanged := oldTrigger.Disabled != newTrigger.Disabled @@ -326,7 +337,7 @@ func diffTables(oldTable, newTable *ir.Table, targetSchema string, targetMajorVe } // Compare triggers - diffTriggers(oldTable, newTable, diff) + diffTriggers(oldTable, newTable, diff, recreatedColumns) // Compare policies oldPolicies := make(map[string]*ir.RLSPolicy) @@ -361,6 +372,14 @@ func diffTables(oldTable, newTable *ir.Table, targetSchema string, targetMajorVe // Find modified policies for name, newPolicy := range newPolicies { if oldPolicy, exists := oldPolicies[name]; exists { + // A policy whose expressions name a column being re-created blocks + // the DROP COLUMN (SQLSTATE 2BP01); it is dropped before the column + // and created again from the desired state afterwards. (#591) + if policyReferencesColumns(oldPolicy, recreatedColumns) { + diff.DroppedPolicies = append(diff.DroppedPolicies, oldPolicy) + diff.AddedPolicies = append(diff.AddedPolicies, newPolicy) + continue + } if !policiesEqual(oldPolicy, newPolicy) { diff.ModifiedPolicies = append(diff.ModifiedPolicies, &policyDiff{ Old: oldPolicy, @@ -438,7 +457,7 @@ func diffExternalTable(oldTable, newTable *ir.Table) *tableDiff { } // For external tables, only compare triggers (not table structure) - diffTriggers(oldTable, newTable, diff) + diffTriggers(oldTable, newTable, diff, nil) // Return nil if no trigger changes if len(diff.AddedTriggers) == 0 && len(diff.DroppedTriggers) == 0 && len(diff.ModifiedTriggers) == 0 { @@ -1100,6 +1119,41 @@ func constraintDroppedWithColumns(constraint *ir.Constraint, droppedColumnSet ma return false } +// collectDropPolicy emits DROP POLICY for a policy of this table. +func (td *tableDiff) collectDropPolicy(policy *ir.RLSPolicy, targetSchema string, collector *diffCollector) { + tableName := getTableNameWithSchema(td.Table.Schema, td.Table.Name, targetSchema) + sql := fmt.Sprintf("DROP POLICY IF EXISTS %s ON %s;", ir.QuoteIdentifier(policy.Name), tableName) + + context := &diffContext{ + Type: DiffTypeTablePolicy, + Operation: DiffOperationDrop, + Path: fmt.Sprintf("%s.%s.%s", td.Table.Schema, td.Table.Name, policy.Name), + Source: policy, + CanRunInTransaction: true, + } + collector.collect(context, sql) +} + +// policyReferencesColumns reports whether a policy's USING or WITH CHECK +// expression names any of the given columns. (#591) +func policyReferencesColumns(policy *ir.RLSPolicy, columns map[string]bool) bool { + return policy != nil && (exprReferencesAnyColumn(policy.Using, columns) || exprReferencesAnyColumn(policy.WithCheck, columns)) +} + +// triggerReferencesColumns reports whether a trigger depends on any of the +// given columns through its UPDATE OF list or WHEN condition. (#591) +func triggerReferencesColumns(trigger *ir.Trigger, columns map[string]bool) bool { + if trigger == nil || len(columns) == 0 { + return false + } + for _, name := range trigger.UpdateColumns { + if columns[name] { + return true + } + } + return exprReferencesAnyColumn(trigger.Condition, columns) +} + // indexReferencesColumns reports whether an index depends on any of the given // columns, i.e. whether ALTER TABLE ... DROP COLUMN of one of them removes it: // as a key or INCLUDE column, inside an expression, or in the partial-index @@ -1142,7 +1196,8 @@ func exprReferencesAnyColumn(expr string, columns map[string]bool) bool { bare := regexp.QuoteMeta(column) // pg_get_expr doubles embedded quotes inside a quoted identifier. quoted := regexp.QuoteMeta(`"` + strings.ReplaceAll(column, `"`, `""`) + `"`) - re := regexp.MustCompile(`(?:^|[^\w"])(?:` + bare + `|` + quoted + `)(?:[^\w"(]|$)`) + // \w plus $ covers every character of an unquoted identifier. + re := regexp.MustCompile(`(?:^|[^\w$"])(?:` + bare + `|` + quoted + `)(?:[^\w$"(]|$)`) if re.MatchString(expr) { return true } @@ -1182,6 +1237,15 @@ func (td *tableDiff) generateAlterTableStatements(targetSchema string, collector collector.collect(context, sql) } + // Policies that reference a column being re-created must go before the + // DROP COLUMN they would otherwise block; the remaining policy drops keep + // their usual place after the RLS changes below. (#591) + for _, policy := range td.DroppedPolicies { + if policyReferencesColumns(policy, td.RecreatedColumns) { + td.collectDropPolicy(policy, targetSchema, collector) + } + } + // Drop constraints first (before dropping columns) - already sorted by the Diff operation for _, constraint := range td.DroppedConstraints { // Skip constraints already removed by a dropped column. (#384) @@ -1629,17 +1693,11 @@ func (td *tableDiff) generateAlterTableStatements(targetSchema string, collector // Drop policies - already sorted by the Diff operation for _, policy := range td.DroppedPolicies { - tableName := getTableNameWithSchema(td.Table.Schema, td.Table.Name, targetSchema) - sql := fmt.Sprintf("DROP POLICY IF EXISTS %s ON %s;", ir.QuoteIdentifier(policy.Name), tableName) - - context := &diffContext{ - Type: DiffTypeTablePolicy, - Operation: DiffOperationDrop, - Path: fmt.Sprintf("%s.%s.%s", td.Table.Schema, td.Table.Name, policy.Name), - Source: policy, - CanRunInTransaction: true, + // Already dropped ahead of the column it depends on. (#591) + if policyReferencesColumns(policy, td.RecreatedColumns) { + continue } - collector.collect(context, sql) + td.collectDropPolicy(policy, targetSchema, collector) } // Drop triggers - skipped here because they are already dropped in the DROP phase diff --git a/testdata/diff/create_table/issue_591_recreate_generated_dependents/diff.sql b/testdata/diff/create_table/issue_591_recreate_generated_dependents/diff.sql index e67203a6..e564366d 100644 --- a/testdata/diff/create_table/issue_591_recreate_generated_dependents/diff.sql +++ b/testdata/diff/create_table/issue_591_recreate_generated_dependents/diff.sql @@ -2,8 +2,14 @@ DROP VIEW IF EXISTS big_orders RESTRICT; DROP VIEW IF EXISTS order_totals RESTRICT; +DROP VIEW IF EXISTS order_labels RESTRICT; + +DROP TRIGGER IF EXISTS orders_total_trg ON orders; + ALTER TABLE shipments DROP CONSTRAINT shipments_order_code_fkey; +DROP POLICY IF EXISTS orders_big ON orders; + ALTER TABLE orders DROP COLUMN total; ALTER TABLE orders DROP COLUMN code; @@ -12,6 +18,14 @@ ALTER TABLE orders ADD COLUMN total integer GENERATED ALWAYS AS ((qty * price)) ALTER TABLE orders ADD COLUMN code text GENERATED ALWAYS AS (('ORD-'::text || (id)::text)) STORED; +CREATE OR REPLACE TRIGGER orders_total_trg + AFTER UPDATE ON orders + FOR EACH ROW + WHEN (((NEW.total > 0))) + EXECUTE FUNCTION orders_audit(); + +CREATE POLICY orders_big ON orders TO PUBLIC USING (total > 100); + DROP INDEX IF EXISTS orders_lookup_idx; CREATE INDEX IF NOT EXISTS orders_lookup_idx ON orders (total); @@ -21,6 +35,11 @@ CREATE UNIQUE INDEX IF NOT EXISTS orders_code_key ON orders (code); ALTER TABLE shipments ADD CONSTRAINT shipments_order_code_fkey FOREIGN KEY (order_code) REFERENCES orders (code); +CREATE OR REPLACE VIEW order_labels AS + SELECT id, + 'x'::text AS label + FROM orders; + CREATE OR REPLACE VIEW order_totals AS SELECT id, total @@ -31,4 +50,6 @@ CREATE OR REPLACE VIEW big_orders AS FROM order_totals WHERE total > 100; +GRANT SELECT ON TABLE order_totals TO app_reader; + GRANT SELECT (id, total) ON TABLE orders TO app_reader; diff --git a/testdata/diff/create_table/issue_591_recreate_generated_dependents/new.sql b/testdata/diff/create_table/issue_591_recreate_generated_dependents/new.sql index 1ed8ce03..65a1b411 100644 --- a/testdata/diff/create_table/issue_591_recreate_generated_dependents/new.sql +++ b/testdata/diff/create_table/issue_591_recreate_generated_dependents/new.sql @@ -5,6 +5,10 @@ BEGIN END IF; END $$; +CREATE FUNCTION public.orders_audit() RETURNS trigger + LANGUAGE plpgsql + AS $$ BEGIN RETURN NEW; END $$; + CREATE TABLE public.orders ( id integer PRIMARY KEY, qty integer NOT NULL, @@ -29,4 +33,14 @@ CREATE VIEW public.big_orders AS SELECT id FROM public.order_totals WHERE total CREATE VIEW public.order_prices AS SELECT id, price FROM public.orders; +CREATE VIEW public.order_labels AS SELECT id, 'x'::text AS label FROM public.orders; + +ALTER TABLE public.orders ENABLE ROW LEVEL SECURITY; + +CREATE POLICY orders_big ON public.orders USING (total > 100); + +CREATE TRIGGER orders_total_trg AFTER UPDATE ON public.orders FOR EACH ROW WHEN (NEW.total > 0) EXECUTE FUNCTION public.orders_audit(); + GRANT SELECT (id, total) ON public.orders TO app_reader; + +GRANT SELECT ON public.order_totals TO app_reader; diff --git a/testdata/diff/create_table/issue_591_recreate_generated_dependents/old.sql b/testdata/diff/create_table/issue_591_recreate_generated_dependents/old.sql index c6bdf5dd..459d4f1e 100644 --- a/testdata/diff/create_table/issue_591_recreate_generated_dependents/old.sql +++ b/testdata/diff/create_table/issue_591_recreate_generated_dependents/old.sql @@ -5,6 +5,10 @@ BEGIN END IF; END $$; +CREATE FUNCTION public.orders_audit() RETURNS trigger + LANGUAGE plpgsql + AS $$ BEGIN RETURN NEW; END $$; + CREATE TABLE public.orders ( id integer PRIMARY KEY, qty integer NOT NULL, @@ -29,4 +33,14 @@ CREATE VIEW public.big_orders AS SELECT id FROM public.order_totals WHERE total CREATE VIEW public.order_prices AS SELECT id, price FROM public.orders; +CREATE VIEW public.order_labels AS SELECT id, code AS label FROM public.orders; + +ALTER TABLE public.orders ENABLE ROW LEVEL SECURITY; + +CREATE POLICY orders_big ON public.orders USING (total > 100); + +CREATE TRIGGER orders_total_trg AFTER UPDATE ON public.orders FOR EACH ROW WHEN (NEW.total > 0) EXECUTE FUNCTION public.orders_audit(); + GRANT SELECT (id, total) ON public.orders TO app_reader; + +GRANT SELECT ON public.order_totals TO app_reader; diff --git a/testdata/diff/create_table/issue_591_recreate_generated_dependents/plan.json b/testdata/diff/create_table/issue_591_recreate_generated_dependents/plan.json index f5a154d0..ae041a58 100644 --- a/testdata/diff/create_table/issue_591_recreate_generated_dependents/plan.json +++ b/testdata/diff/create_table/issue_591_recreate_generated_dependents/plan.json @@ -3,7 +3,7 @@ "pgschema_version": "1.13.0", "created_at": "1970-01-01T00:00:00Z", "source_fingerprint": { - "hash": "9d16cac1391edcd0cebffe22c50891525861d04d7f459e61de41da64255372af" + "hash": "d55cddab4cdd29708f6dd7af47adf0a45bcbf022f435f3c131b17a3b124a5f61" }, "groups": [ { @@ -20,12 +20,30 @@ "operation": "recreate", "path": "public.order_totals" }, + { + "sql": "DROP VIEW IF EXISTS order_labels RESTRICT;", + "type": "view", + "operation": "recreate", + "path": "public.order_labels" + }, + { + "sql": "DROP TRIGGER IF EXISTS orders_total_trg ON orders;", + "type": "table.trigger", + "operation": "drop", + "path": "public.orders.orders_total_trg" + }, { "sql": "ALTER TABLE shipments DROP CONSTRAINT shipments_order_code_fkey;", "type": "table.constraint", "operation": "drop", "path": "public.shipments.shipments_order_code_fkey" }, + { + "sql": "DROP POLICY IF EXISTS orders_big ON orders;", + "type": "table.policy", + "operation": "drop", + "path": "public.orders.orders_big" + }, { "sql": "ALTER TABLE orders DROP COLUMN total;", "type": "table.column", @@ -49,6 +67,18 @@ "type": "table.column", "operation": "create", "path": "public.orders.code" + }, + { + "sql": "CREATE OR REPLACE TRIGGER orders_total_trg\n AFTER UPDATE ON orders\n FOR EACH ROW\n WHEN (((NEW.total > 0)))\n EXECUTE FUNCTION orders_audit();", + "type": "table.trigger", + "operation": "create", + "path": "public.orders.orders_total_trg" + }, + { + "sql": "CREATE POLICY orders_big ON orders TO PUBLIC USING (total > 100);", + "type": "table.policy", + "operation": "create", + "path": "public.orders.orders_big" } ] }, @@ -138,6 +168,12 @@ }, { "steps": [ + { + "sql": "CREATE OR REPLACE VIEW order_labels AS\n SELECT id,\n 'x'::text AS label\n FROM orders;", + "type": "view", + "operation": "create", + "path": "public.order_labels" + }, { "sql": "CREATE OR REPLACE VIEW order_totals AS\n SELECT id,\n total\n FROM orders;", "type": "view", @@ -150,6 +186,12 @@ "operation": "recreate", "path": "public.big_orders" }, + { + "sql": "GRANT SELECT ON TABLE order_totals TO app_reader;", + "type": "privilege", + "operation": "create", + "path": "privileges.VIEW.order_totals.app_reader" + }, { "sql": "GRANT SELECT (id, total) ON TABLE orders TO app_reader;", "type": "column_privilege", diff --git a/testdata/diff/create_table/issue_591_recreate_generated_dependents/plan.sql b/testdata/diff/create_table/issue_591_recreate_generated_dependents/plan.sql index a50ba9c0..1bb6b9ff 100644 --- a/testdata/diff/create_table/issue_591_recreate_generated_dependents/plan.sql +++ b/testdata/diff/create_table/issue_591_recreate_generated_dependents/plan.sql @@ -2,8 +2,14 @@ DROP VIEW IF EXISTS big_orders RESTRICT; DROP VIEW IF EXISTS order_totals RESTRICT; +DROP VIEW IF EXISTS order_labels RESTRICT; + +DROP TRIGGER IF EXISTS orders_total_trg ON orders; + ALTER TABLE shipments DROP CONSTRAINT shipments_order_code_fkey; +DROP POLICY IF EXISTS orders_big ON orders; + ALTER TABLE orders DROP COLUMN total; ALTER TABLE orders DROP COLUMN code; @@ -12,6 +18,14 @@ ALTER TABLE orders ADD COLUMN total integer GENERATED ALWAYS AS ((qty * price)) ALTER TABLE orders ADD COLUMN code text GENERATED ALWAYS AS (('ORD-'::text || (id)::text)) STORED; +CREATE OR REPLACE TRIGGER orders_total_trg + AFTER UPDATE ON orders + FOR EACH ROW + WHEN (((NEW.total > 0))) + EXECUTE FUNCTION orders_audit(); + +CREATE POLICY orders_big ON orders TO PUBLIC USING (total > 100); + CREATE INDEX CONCURRENTLY IF NOT EXISTS orders_lookup_idx_pgschema_new ON orders (total); -- pgschema:wait @@ -49,6 +63,11 @@ ADD CONSTRAINT shipments_order_code_fkey FOREIGN KEY (order_code) REFERENCES ord ALTER TABLE shipments VALIDATE CONSTRAINT shipments_order_code_fkey; +CREATE OR REPLACE VIEW order_labels AS + SELECT id, + 'x'::text AS label + FROM orders; + CREATE OR REPLACE VIEW order_totals AS SELECT id, total @@ -59,4 +78,6 @@ CREATE OR REPLACE VIEW big_orders AS FROM order_totals WHERE total > 100; +GRANT SELECT ON TABLE order_totals TO app_reader; + GRANT SELECT (id, total) ON TABLE orders TO app_reader; diff --git a/testdata/diff/create_table/issue_591_recreate_generated_dependents/plan.txt b/testdata/diff/create_table/issue_591_recreate_generated_dependents/plan.txt index 5b518bc2..0d7bf082 100644 --- a/testdata/diff/create_table/issue_591_recreate_generated_dependents/plan.txt +++ b/testdata/diff/create_table/issue_591_recreate_generated_dependents/plan.txt @@ -1,8 +1,9 @@ -Plan: 1 to add, 4 to modify. +Plan: 2 to add, 5 to modify. Summary by type: tables: 2 to modify - views: 2 to modify + views: 3 to modify + privileges: 1 to add column privileges: 1 to add Tables: @@ -13,14 +14,22 @@ Tables: + total (column) + orders_code_key (index) ~ orders_lookup_idx (index - concurrent rebuild) + - orders_big (policy) + + orders_big (policy) + - orders_total_trg (trigger) + + orders_total_trg (trigger) ~ shipments - shipments_order_code_fkey (constraint) + shipments_order_code_fkey (constraint) Views: ~ big_orders + ~ order_labels ~ order_totals +Privileges: + + app_reader + Column privileges: + app_reader @@ -32,8 +41,14 @@ DROP VIEW IF EXISTS big_orders RESTRICT; DROP VIEW IF EXISTS order_totals RESTRICT; +DROP VIEW IF EXISTS order_labels RESTRICT; + +DROP TRIGGER IF EXISTS orders_total_trg ON orders; + ALTER TABLE shipments DROP CONSTRAINT shipments_order_code_fkey; +DROP POLICY IF EXISTS orders_big ON orders; + ALTER TABLE orders DROP COLUMN total; ALTER TABLE orders DROP COLUMN code; @@ -42,6 +57,14 @@ ALTER TABLE orders ADD COLUMN total integer GENERATED ALWAYS AS ((qty * price)) ALTER TABLE orders ADD COLUMN code text GENERATED ALWAYS AS (('ORD-'::text || (id)::text)) STORED; +CREATE OR REPLACE TRIGGER orders_total_trg + AFTER UPDATE ON orders + FOR EACH ROW + WHEN (((NEW.total > 0))) + EXECUTE FUNCTION orders_audit(); + +CREATE POLICY orders_big ON orders TO PUBLIC USING (total > 100); + -- Transaction Group #2 CREATE INDEX CONCURRENTLY IF NOT EXISTS orders_lookup_idx_pgschema_new ON orders (total); @@ -87,6 +110,11 @@ ADD CONSTRAINT shipments_order_code_fkey FOREIGN KEY (order_code) REFERENCES ord ALTER TABLE shipments VALIDATE CONSTRAINT shipments_order_code_fkey; -- Transaction Group #9 +CREATE OR REPLACE VIEW order_labels AS + SELECT id, + 'x'::text AS label + FROM orders; + CREATE OR REPLACE VIEW order_totals AS SELECT id, total @@ -97,4 +125,6 @@ CREATE OR REPLACE VIEW big_orders AS FROM order_totals WHERE total > 100; +GRANT SELECT ON TABLE order_totals TO app_reader; + GRANT SELECT (id, total) ON TABLE orders TO app_reader; From 68ef6b741f8132c85f355ce6c4aaa874b45394a8 Mon Sep 17 00:00:00 2001 From: tianzhou Date: Wed, 9 Sep 2026 19:53:13 -0700 Subject: [PATCH 04/11] fix: keep explicit index drops and grouped-grant revokes around a replaced generated column (#591) Third round of review follow-ups: - Indexes that the textual dependency check attributes to a re-created column are dropped explicitly as well as re-created. The DROP uses IF EXISTS, so it is a no-op when DROP COLUMN already took the index and still removes a stale index when the check was a false positive. A name directly preceded by ":" is now treated as a type cast, not a column. - Column grants: the old grant stays in the old state so removals on the surviving columns of a grouped grant are still revoked; a desired grant touching a re-created column is re-issued after the column is back even when it matches the old one. Co-Authored-By: Claude Fable 5.1 --- internal/diff/diff.go | 22 ++++++---- internal/diff/generated_column_test.go | 2 + internal/diff/table.go | 23 +++++----- .../issue_591_alter_generated_column/diff.sql | 2 + .../plan.json | 22 +++++++--- .../issue_591_alter_generated_column/plan.sql | 8 +++- .../issue_591_alter_generated_column/plan.txt | 10 +++-- .../diff.sql | 6 ++- .../plan.json | 44 ++++++++++++------- .../plan.sql | 16 ++++--- .../plan.txt | 18 +++++--- 11 files changed, 111 insertions(+), 62 deletions(-) diff --git a/internal/diff/diff.go b/internal/diff/diff.go index 738e6e01..624127ac 100644 --- a/internal/diff/diff.go +++ b/internal/diff/diff.go @@ -1493,22 +1493,25 @@ func generateMigration(oldIR, newIR *ir.IR, targetSchema string, qualifySchema b for _, dbSchema := range oldIR.Schemas { for _, cp := range dbSchema.ColumnPrivileges { - // DROP COLUMN discards the column's ACL, so a grant touching a - // re-created column is gone once the migration runs; leaving it - // out of the old state makes the desired grant come back as an - // addition after the column exists again (#591). - if columnPrivilegeTouchesColumns(cp, recreatedColumnsByTable[dbSchema.Name+"."+cp.TableName]) { - continue - } key := cp.GetFullKey() oldColPrivs[key] = cp } } + // Desired grants that touch a column this migration re-creates, keyed + // like newColPrivs. DROP COLUMN discards that column's ACL, so such a + // grant must be issued again after the column exists, even when the old + // and desired grants match (#591). The old grant stays in the old state + // so that removals on surviving columns of a grouped grant are still + // revoked by the normal comparison. + newColPrivsOnRecreated := make(map[string]bool) for _, dbSchema := range newIR.Schemas { for _, cp := range dbSchema.ColumnPrivileges { key := cp.GetFullKey() newColPrivs[key] = cp + if columnPrivilegeTouchesColumns(cp, recreatedColumnsByTable[dbSchema.Name+"."+cp.TableName]) { + newColPrivsOnRecreated[key] = true + } } } @@ -1565,9 +1568,10 @@ func generateMigration(oldIR, newIR *ir.IR, targetSchema string, qualifySchema b } } - // Find added column privileges + // Find added column privileges. A matched grant on a re-created column is + // added as well: it is re-issued after the column is back (#591). for fullKey, cp := range newColPrivs { - if !matchedNewColPrivs[fullKey] { + if !matchedNewColPrivs[fullKey] || newColPrivsOnRecreated[fullKey] { diff.addedColumnPrivileges = append(diff.addedColumnPrivileges, cp) } } diff --git a/internal/diff/generated_column_test.go b/internal/diff/generated_column_test.go index afc372b3..43643f3c 100644 --- a/internal/diff/generated_column_test.go +++ b/internal/diff/generated_column_test.go @@ -67,6 +67,7 @@ func TestGeneratedExpressionChange_VersionGate(t *testing.T) { want := strings.Join([]string{ "ALTER TABLE metrics DROP COLUMN doubled;", "ALTER TABLE metrics ADD COLUMN doubled integer GENERATED ALWAYS AS ((a * 2)) STORED;", + "DROP INDEX IF EXISTS metrics_doubled_idx;", "CREATE INDEX IF NOT EXISTS metrics_doubled_idx ON metrics (doubled);", "", }, "\n") @@ -88,6 +89,7 @@ func TestExprReferencesAnyColumn(t *testing.T) { {`("a""b" + 1)`, true}, // embedded quote doubled by pg_get_expr {`("a"b" + 1)`, false}, {"(b$x + 1)", false}, // $ is part of the identifier + {"((a)::b)", false}, // type cast, not a column {"(x$b + 1)", false}, {"(b > 10)", true}, {"(bb + 1)", false}, diff --git a/internal/diff/table.go b/internal/diff/table.go index 96bf23c1..f51f2729 100644 --- a/internal/diff/table.go +++ b/internal/diff/table.go @@ -299,10 +299,6 @@ func diffTables(oldTable, newTable *ir.Table, targetSchema string, targetMajorVe // Find dropped indexes for name, index := range oldIndexes { if _, exists := newIndexes[name]; !exists { - // Already gone with the re-created column it depends on. (#591) - if indexReferencesColumns(index, recreatedColumns) { - continue - } diff.DroppedIndexes = append(diff.DroppedIndexes, index) } } @@ -312,10 +308,12 @@ func diffTables(oldTable, newTable *ir.Table, targetSchema string, targetMajorVe if oldIndex, exists := oldIndexes[name]; exists { // DROP COLUMN removes every index on a re-created column, so the // desired-state index is created afresh afterwards. The old - // definition decides: a same-named index that moves from another - // column onto the re-created one still exists and takes the normal - // drop + add path below. (#591) + // definition decides, and it is dropped explicitly as well: the + // DROP uses IF EXISTS, so it is a no-op when the column drop + // already took the index, and it still removes the old index + // when the textual dependency check was a false positive. (#591) if indexReferencesColumns(oldIndex, recreatedColumns) { + diff.DroppedIndexes = append(diff.DroppedIndexes, oldIndex) diff.AddedIndexes = append(diff.AddedIndexes, newIndex) continue } @@ -1183,10 +1181,11 @@ var sqlStringLiteralRegex = regexp.MustCompile(`'(?:[^']|'')*'`) // exprReferencesAnyColumn reports whether a SQL expression as rendered by // pg_get_expr mentions any of the columns as a bare or quoted identifier. -// String literals are blanked out first, and a name directly followed by "(" -// is a function call rather than a column. A false positive only costs a -// redundant CREATE INDEX that fails loudly at apply time, whereas a miss would -// silently lose the dependent index. (#591) +// String literals are blanked out first; a name directly followed by "(" is a +// function call and a name directly preceded by ":" is a type cast, not a +// column. Callers pair a positive result with IF EXISTS drops and a re-create +// from the desired state, so a false positive costs a redundant drop + create +// while a miss would leave a dependent object behind. (#591) func exprReferencesAnyColumn(expr string, columns map[string]bool) bool { if expr == "" || len(columns) == 0 { return false @@ -1197,7 +1196,7 @@ func exprReferencesAnyColumn(expr string, columns map[string]bool) bool { // pg_get_expr doubles embedded quotes inside a quoted identifier. quoted := regexp.QuoteMeta(`"` + strings.ReplaceAll(column, `"`, `""`) + `"`) // \w plus $ covers every character of an unquoted identifier. - re := regexp.MustCompile(`(?:^|[^\w$"])(?:` + bare + `|` + quoted + `)(?:[^\w$"(]|$)`) + re := regexp.MustCompile(`(?:^|[^\w$":])(?:` + bare + `|` + quoted + `)(?:[^\w$"(]|$)`) if re.MatchString(expr) { return true } diff --git a/testdata/diff/create_table/issue_591_alter_generated_column/diff.sql b/testdata/diff/create_table/issue_591_alter_generated_column/diff.sql index ad55b441..4489a5d3 100644 --- a/testdata/diff/create_table/issue_591_alter_generated_column/diff.sql +++ b/testdata/diff/create_table/issue_591_alter_generated_column/diff.sql @@ -14,6 +14,8 @@ ALTER TABLE metrics ALTER COLUMN label SET DEFAULT 'none'; ALTER TABLE metrics ADD CONSTRAINT metrics_tripled_check CHECK (tripled > 0); +DROP INDEX IF EXISTS metrics_tripled_idx; + CREATE INDEX IF NOT EXISTS metrics_tripled_idx ON metrics ((tripled + 1)) WHERE (tripled > 10); ALTER TABLE metric_refs diff --git a/testdata/diff/create_table/issue_591_alter_generated_column/plan.json b/testdata/diff/create_table/issue_591_alter_generated_column/plan.json index a4a72ac0..9532bd31 100644 --- a/testdata/diff/create_table/issue_591_alter_generated_column/plan.json +++ b/testdata/diff/create_table/issue_591_alter_generated_column/plan.json @@ -65,9 +65,9 @@ { "steps": [ { - "sql": "CREATE INDEX CONCURRENTLY IF NOT EXISTS metrics_tripled_idx ON metrics ((tripled + 1)) WHERE (tripled > 10);", + "sql": "CREATE INDEX CONCURRENTLY IF NOT EXISTS metrics_tripled_idx_pgschema_new ON metrics ((tripled + 1)) WHERE (tripled > 10);", "type": "table.index", - "operation": "create", + "operation": "alter", "path": "public.metrics.metrics_tripled_idx" } ] @@ -75,19 +75,31 @@ { "steps": [ { - "sql": "SELECT \n COALESCE(i.indisvalid, false) as done,\n CASE \n WHEN p.blocks_total > 0 THEN p.blocks_done * 100 / p.blocks_total\n ELSE 0\n END as progress\nFROM pg_class c\nLEFT JOIN pg_index i ON c.oid = i.indexrelid\nLEFT JOIN pg_stat_progress_create_index p ON c.oid = p.index_relid\nWHERE c.relname = 'metrics_tripled_idx';", + "sql": "SELECT \n COALESCE(i.indisvalid, false) as done,\n CASE \n WHEN p.blocks_total > 0 THEN p.blocks_done * 100 / p.blocks_total\n ELSE 0\n END as progress\nFROM pg_class c\nLEFT JOIN pg_index i ON c.oid = i.indexrelid\nLEFT JOIN pg_stat_progress_create_index p ON c.oid = p.index_relid\nWHERE c.relname = 'metrics_tripled_idx_pgschema_new';", "directive": { "type": "wait", - "message": "Creating index metrics_tripled_idx" + "message": "Creating index metrics_tripled_idx_pgschema_new" }, "type": "table.index", - "operation": "create", + "operation": "alter", "path": "public.metrics.metrics_tripled_idx" } ] }, { "steps": [ + { + "sql": "DROP INDEX IF EXISTS metrics_tripled_idx;", + "type": "table.index", + "operation": "alter", + "path": "public.metrics.metrics_tripled_idx" + }, + { + "sql": "ALTER INDEX metrics_tripled_idx_pgschema_new RENAME TO metrics_tripled_idx;", + "type": "table.index", + "operation": "alter", + "path": "public.metrics.metrics_tripled_idx" + }, { "sql": "ALTER TABLE metric_refs\nADD CONSTRAINT metric_refs_tripled_fkey FOREIGN KEY (tripled) REFERENCES metrics (tripled) NOT VALID;", "type": "table.constraint", diff --git a/testdata/diff/create_table/issue_591_alter_generated_column/plan.sql b/testdata/diff/create_table/issue_591_alter_generated_column/plan.sql index 8b7fa209..29089b23 100644 --- a/testdata/diff/create_table/issue_591_alter_generated_column/plan.sql +++ b/testdata/diff/create_table/issue_591_alter_generated_column/plan.sql @@ -16,7 +16,7 @@ ADD CONSTRAINT metrics_tripled_check CHECK (tripled > 0) NOT VALID; ALTER TABLE metrics VALIDATE CONSTRAINT metrics_tripled_check; -CREATE INDEX CONCURRENTLY IF NOT EXISTS metrics_tripled_idx ON metrics ((tripled + 1)) WHERE (tripled > 10); +CREATE INDEX CONCURRENTLY IF NOT EXISTS metrics_tripled_idx_pgschema_new ON metrics ((tripled + 1)) WHERE (tripled > 10); -- pgschema:wait SELECT @@ -28,7 +28,11 @@ SELECT FROM pg_class c LEFT JOIN pg_index i ON c.oid = i.indexrelid LEFT JOIN pg_stat_progress_create_index p ON c.oid = p.index_relid -WHERE c.relname = 'metrics_tripled_idx'; +WHERE c.relname = 'metrics_tripled_idx_pgschema_new'; + +DROP INDEX IF EXISTS metrics_tripled_idx; + +ALTER INDEX metrics_tripled_idx_pgschema_new RENAME TO metrics_tripled_idx; ALTER TABLE metric_refs ADD CONSTRAINT metric_refs_tripled_fkey FOREIGN KEY (tripled) REFERENCES metrics (tripled) NOT VALID; diff --git a/testdata/diff/create_table/issue_591_alter_generated_column/plan.txt b/testdata/diff/create_table/issue_591_alter_generated_column/plan.txt index 1126c374..00cdf329 100644 --- a/testdata/diff/create_table/issue_591_alter_generated_column/plan.txt +++ b/testdata/diff/create_table/issue_591_alter_generated_column/plan.txt @@ -13,7 +13,7 @@ Tables: - tripled (column) + tripled (column) + metrics_tripled_check (constraint) - + metrics_tripled_idx (index) + ~ metrics_tripled_idx (index - concurrent rebuild) DDL to be executed: -------------------------------------------------- @@ -39,7 +39,7 @@ ADD CONSTRAINT metrics_tripled_check CHECK (tripled > 0) NOT VALID; ALTER TABLE metrics VALIDATE CONSTRAINT metrics_tripled_check; -- Transaction Group #3 -CREATE INDEX CONCURRENTLY IF NOT EXISTS metrics_tripled_idx ON metrics ((tripled + 1)) WHERE (tripled > 10); +CREATE INDEX CONCURRENTLY IF NOT EXISTS metrics_tripled_idx_pgschema_new ON metrics ((tripled + 1)) WHERE (tripled > 10); -- Transaction Group #4 -- pgschema:wait @@ -52,9 +52,13 @@ SELECT FROM pg_class c LEFT JOIN pg_index i ON c.oid = i.indexrelid LEFT JOIN pg_stat_progress_create_index p ON c.oid = p.index_relid -WHERE c.relname = 'metrics_tripled_idx'; +WHERE c.relname = 'metrics_tripled_idx_pgschema_new'; -- Transaction Group #5 +DROP INDEX IF EXISTS metrics_tripled_idx; + +ALTER INDEX metrics_tripled_idx_pgschema_new RENAME TO metrics_tripled_idx; + ALTER TABLE metric_refs ADD CONSTRAINT metric_refs_tripled_fkey FOREIGN KEY (tripled) REFERENCES metrics (tripled) NOT VALID; diff --git a/testdata/diff/create_table/issue_591_recreate_generated_dependents/diff.sql b/testdata/diff/create_table/issue_591_recreate_generated_dependents/diff.sql index e564366d..b442b22c 100644 --- a/testdata/diff/create_table/issue_591_recreate_generated_dependents/diff.sql +++ b/testdata/diff/create_table/issue_591_recreate_generated_dependents/diff.sql @@ -26,12 +26,14 @@ CREATE OR REPLACE TRIGGER orders_total_trg CREATE POLICY orders_big ON orders TO PUBLIC USING (total > 100); +DROP INDEX IF EXISTS orders_code_key; + +CREATE UNIQUE INDEX IF NOT EXISTS orders_code_key ON orders (code); + DROP INDEX IF EXISTS orders_lookup_idx; CREATE INDEX IF NOT EXISTS orders_lookup_idx ON orders (total); -CREATE UNIQUE INDEX IF NOT EXISTS orders_code_key ON orders (code); - ALTER TABLE shipments ADD CONSTRAINT shipments_order_code_fkey FOREIGN KEY (order_code) REFERENCES orders (code); diff --git a/testdata/diff/create_table/issue_591_recreate_generated_dependents/plan.json b/testdata/diff/create_table/issue_591_recreate_generated_dependents/plan.json index ae041a58..f9d6f948 100644 --- a/testdata/diff/create_table/issue_591_recreate_generated_dependents/plan.json +++ b/testdata/diff/create_table/issue_591_recreate_generated_dependents/plan.json @@ -85,69 +85,81 @@ { "steps": [ { - "sql": "CREATE INDEX CONCURRENTLY IF NOT EXISTS orders_lookup_idx_pgschema_new ON orders (total);", + "sql": "CREATE UNIQUE INDEX CONCURRENTLY IF NOT EXISTS orders_code_key_pgschema_new ON orders (code);", "type": "table.index", "operation": "alter", - "path": "public.orders.orders_lookup_idx" + "path": "public.orders.orders_code_key" } ] }, { "steps": [ { - "sql": "SELECT \n COALESCE(i.indisvalid, false) as done,\n CASE \n WHEN p.blocks_total > 0 THEN p.blocks_done * 100 / p.blocks_total\n ELSE 0\n END as progress\nFROM pg_class c\nLEFT JOIN pg_index i ON c.oid = i.indexrelid\nLEFT JOIN pg_stat_progress_create_index p ON c.oid = p.index_relid\nWHERE c.relname = 'orders_lookup_idx_pgschema_new';", + "sql": "SELECT \n COALESCE(i.indisvalid, false) as done,\n CASE \n WHEN p.blocks_total > 0 THEN p.blocks_done * 100 / p.blocks_total\n ELSE 0\n END as progress\nFROM pg_class c\nLEFT JOIN pg_index i ON c.oid = i.indexrelid\nLEFT JOIN pg_stat_progress_create_index p ON c.oid = p.index_relid\nWHERE c.relname = 'orders_code_key_pgschema_new';", "directive": { "type": "wait", - "message": "Creating index orders_lookup_idx_pgschema_new" + "message": "Creating index orders_code_key_pgschema_new" }, "type": "table.index", "operation": "alter", - "path": "public.orders.orders_lookup_idx" + "path": "public.orders.orders_code_key" } ] }, { "steps": [ { - "sql": "DROP INDEX IF EXISTS orders_lookup_idx;", + "sql": "DROP INDEX IF EXISTS orders_code_key;", "type": "table.index", "operation": "alter", - "path": "public.orders.orders_lookup_idx" + "path": "public.orders.orders_code_key" }, { - "sql": "ALTER INDEX orders_lookup_idx_pgschema_new RENAME TO orders_lookup_idx;", + "sql": "ALTER INDEX orders_code_key_pgschema_new RENAME TO orders_code_key;", "type": "table.index", "operation": "alter", - "path": "public.orders.orders_lookup_idx" + "path": "public.orders.orders_code_key" } ] }, { "steps": [ { - "sql": "CREATE UNIQUE INDEX CONCURRENTLY IF NOT EXISTS orders_code_key ON orders (code);", + "sql": "CREATE INDEX CONCURRENTLY IF NOT EXISTS orders_lookup_idx_pgschema_new ON orders (total);", "type": "table.index", - "operation": "create", - "path": "public.orders.orders_code_key" + "operation": "alter", + "path": "public.orders.orders_lookup_idx" } ] }, { "steps": [ { - "sql": "SELECT \n COALESCE(i.indisvalid, false) as done,\n CASE \n WHEN p.blocks_total > 0 THEN p.blocks_done * 100 / p.blocks_total\n ELSE 0\n END as progress\nFROM pg_class c\nLEFT JOIN pg_index i ON c.oid = i.indexrelid\nLEFT JOIN pg_stat_progress_create_index p ON c.oid = p.index_relid\nWHERE c.relname = 'orders_code_key';", + "sql": "SELECT \n COALESCE(i.indisvalid, false) as done,\n CASE \n WHEN p.blocks_total > 0 THEN p.blocks_done * 100 / p.blocks_total\n ELSE 0\n END as progress\nFROM pg_class c\nLEFT JOIN pg_index i ON c.oid = i.indexrelid\nLEFT JOIN pg_stat_progress_create_index p ON c.oid = p.index_relid\nWHERE c.relname = 'orders_lookup_idx_pgschema_new';", "directive": { "type": "wait", - "message": "Creating index orders_code_key" + "message": "Creating index orders_lookup_idx_pgschema_new" }, "type": "table.index", - "operation": "create", - "path": "public.orders.orders_code_key" + "operation": "alter", + "path": "public.orders.orders_lookup_idx" } ] }, { "steps": [ + { + "sql": "DROP INDEX IF EXISTS orders_lookup_idx;", + "type": "table.index", + "operation": "alter", + "path": "public.orders.orders_lookup_idx" + }, + { + "sql": "ALTER INDEX orders_lookup_idx_pgschema_new RENAME TO orders_lookup_idx;", + "type": "table.index", + "operation": "alter", + "path": "public.orders.orders_lookup_idx" + }, { "sql": "ALTER TABLE shipments\nADD CONSTRAINT shipments_order_code_fkey FOREIGN KEY (order_code) REFERENCES orders (code) NOT VALID;", "type": "table.constraint", diff --git a/testdata/diff/create_table/issue_591_recreate_generated_dependents/plan.sql b/testdata/diff/create_table/issue_591_recreate_generated_dependents/plan.sql index 1bb6b9ff..e3ebbdee 100644 --- a/testdata/diff/create_table/issue_591_recreate_generated_dependents/plan.sql +++ b/testdata/diff/create_table/issue_591_recreate_generated_dependents/plan.sql @@ -26,7 +26,7 @@ CREATE OR REPLACE TRIGGER orders_total_trg CREATE POLICY orders_big ON orders TO PUBLIC USING (total > 100); -CREATE INDEX CONCURRENTLY IF NOT EXISTS orders_lookup_idx_pgschema_new ON orders (total); +CREATE UNIQUE INDEX CONCURRENTLY IF NOT EXISTS orders_code_key_pgschema_new ON orders (code); -- pgschema:wait SELECT @@ -38,13 +38,13 @@ SELECT FROM pg_class c LEFT JOIN pg_index i ON c.oid = i.indexrelid LEFT JOIN pg_stat_progress_create_index p ON c.oid = p.index_relid -WHERE c.relname = 'orders_lookup_idx_pgschema_new'; +WHERE c.relname = 'orders_code_key_pgschema_new'; -DROP INDEX IF EXISTS orders_lookup_idx; +DROP INDEX IF EXISTS orders_code_key; -ALTER INDEX orders_lookup_idx_pgschema_new RENAME TO orders_lookup_idx; +ALTER INDEX orders_code_key_pgschema_new RENAME TO orders_code_key; -CREATE UNIQUE INDEX CONCURRENTLY IF NOT EXISTS orders_code_key ON orders (code); +CREATE INDEX CONCURRENTLY IF NOT EXISTS orders_lookup_idx_pgschema_new ON orders (total); -- pgschema:wait SELECT @@ -56,7 +56,11 @@ SELECT FROM pg_class c LEFT JOIN pg_index i ON c.oid = i.indexrelid LEFT JOIN pg_stat_progress_create_index p ON c.oid = p.index_relid -WHERE c.relname = 'orders_code_key'; +WHERE c.relname = 'orders_lookup_idx_pgschema_new'; + +DROP INDEX IF EXISTS orders_lookup_idx; + +ALTER INDEX orders_lookup_idx_pgschema_new RENAME TO orders_lookup_idx; ALTER TABLE shipments ADD CONSTRAINT shipments_order_code_fkey FOREIGN KEY (order_code) REFERENCES orders (code) NOT VALID; diff --git a/testdata/diff/create_table/issue_591_recreate_generated_dependents/plan.txt b/testdata/diff/create_table/issue_591_recreate_generated_dependents/plan.txt index 0d7bf082..ef73c809 100644 --- a/testdata/diff/create_table/issue_591_recreate_generated_dependents/plan.txt +++ b/testdata/diff/create_table/issue_591_recreate_generated_dependents/plan.txt @@ -12,7 +12,7 @@ Tables: + code (column) - total (column) + total (column) - + orders_code_key (index) + ~ orders_code_key (index - concurrent rebuild) ~ orders_lookup_idx (index - concurrent rebuild) - orders_big (policy) + orders_big (policy) @@ -66,7 +66,7 @@ CREATE OR REPLACE TRIGGER orders_total_trg CREATE POLICY orders_big ON orders TO PUBLIC USING (total > 100); -- Transaction Group #2 -CREATE INDEX CONCURRENTLY IF NOT EXISTS orders_lookup_idx_pgschema_new ON orders (total); +CREATE UNIQUE INDEX CONCURRENTLY IF NOT EXISTS orders_code_key_pgschema_new ON orders (code); -- Transaction Group #3 -- pgschema:wait @@ -79,15 +79,15 @@ SELECT FROM pg_class c LEFT JOIN pg_index i ON c.oid = i.indexrelid LEFT JOIN pg_stat_progress_create_index p ON c.oid = p.index_relid -WHERE c.relname = 'orders_lookup_idx_pgschema_new'; +WHERE c.relname = 'orders_code_key_pgschema_new'; -- Transaction Group #4 -DROP INDEX IF EXISTS orders_lookup_idx; +DROP INDEX IF EXISTS orders_code_key; -ALTER INDEX orders_lookup_idx_pgschema_new RENAME TO orders_lookup_idx; +ALTER INDEX orders_code_key_pgschema_new RENAME TO orders_code_key; -- Transaction Group #5 -CREATE UNIQUE INDEX CONCURRENTLY IF NOT EXISTS orders_code_key ON orders (code); +CREATE INDEX CONCURRENTLY IF NOT EXISTS orders_lookup_idx_pgschema_new ON orders (total); -- Transaction Group #6 -- pgschema:wait @@ -100,9 +100,13 @@ SELECT FROM pg_class c LEFT JOIN pg_index i ON c.oid = i.indexrelid LEFT JOIN pg_stat_progress_create_index p ON c.oid = p.index_relid -WHERE c.relname = 'orders_code_key'; +WHERE c.relname = 'orders_lookup_idx_pgschema_new'; -- Transaction Group #7 +DROP INDEX IF EXISTS orders_lookup_idx; + +ALTER INDEX orders_lookup_idx_pgschema_new RENAME TO orders_lookup_idx; + ALTER TABLE shipments ADD CONSTRAINT shipments_order_code_fkey FOREIGN KEY (order_code) REFERENCES orders (code) NOT VALID; From 02eb0ebb97b9a8398e9a42f0e82bf7baacda87bb Mon Sep 17 00:00:00 2001 From: tianzhou Date: Wed, 9 Sep 2026 20:35:41 -0700 Subject: [PATCH 05/11] refactor: share one identifier matcher across textual dependency checks (#591) containsIdentifier (view -> relation dependencies) and exprReferencesAnyColumn (expression -> column dependencies) each had their own regex with different gaps: the former missed quoted identifiers with embedded quotes and matched inside string literals, the latter carried the quoting and boundary rules alone. Both now build on identifierRegexp in identifier_match.go, which renders the bare and the quoted spelling (quotes doubled) of every name segment, applies identifier boundaries including $, strips string literals, excludes function calls and type casts in column mode, and caches compiled patterns. viewDependsOnTable matches schema and table segments separately through containsQualifiedIdentifier. Co-Authored-By: Claude Fable 5.1 --- internal/diff/generated_column_test.go | 28 ------ internal/diff/identifier_match.go | 125 +++++++++++++++++++++++++ internal/diff/identifier_match_test.go | 77 +++++++++++++++ internal/diff/table.go | 30 ------ internal/diff/view.go | 33 +------ 5 files changed, 203 insertions(+), 90 deletions(-) create mode 100644 internal/diff/identifier_match.go create mode 100644 internal/diff/identifier_match_test.go diff --git a/internal/diff/generated_column_test.go b/internal/diff/generated_column_test.go index 43643f3c..a19a3ddf 100644 --- a/internal/diff/generated_column_test.go +++ b/internal/diff/generated_column_test.go @@ -76,31 +76,3 @@ func TestGeneratedExpressionChange_VersionGate(t *testing.T) { } } } - -func TestExprReferencesAnyColumn(t *testing.T) { - cols := map[string]bool{"b": true, "my col": true, `a"b`: true} - cases := []struct { - expr string - want bool - }{ - {"(b + 1)", true}, - {"b", true}, - {"(\"my col\" * 2)", true}, - {`("a""b" + 1)`, true}, // embedded quote doubled by pg_get_expr - {`("a"b" + 1)`, false}, - {"(b$x + 1)", false}, // $ is part of the identifier - {"((a)::b)", false}, // type cast, not a column - {"(x$b + 1)", false}, - {"(b > 10)", true}, - {"(bb + 1)", false}, - {"(a + 1)", false}, - {"b(a)", false}, // function call, not a column - {"('b'::text)", false}, // string literal - {"", false}, - } - for _, c := range cases { - if got := exprReferencesAnyColumn(c.expr, cols); got != c.want { - t.Errorf("exprReferencesAnyColumn(%q) = %v, want %v", c.expr, got, c.want) - } - } -} diff --git a/internal/diff/identifier_match.go b/internal/diff/identifier_match.go new file mode 100644 index 00000000..9649bc1c --- /dev/null +++ b/internal/diff/identifier_match.go @@ -0,0 +1,125 @@ +package diff + +import ( + "regexp" + "strings" + "sync" +) + +// Textual dependency checks (does this view read that table, does this +// expression name that column) run over SQL text produced by PostgreSQL's +// deparsers: pg_get_viewdef, pg_get_expr, pg_get_indexdef, pg_get_triggerdef. +// Those render an identifier either bare (all-lowercase names) or +// double-quoted with embedded quotes doubled ("a""b"). Every such check goes +// through identifierRegexp so the two spellings, identifier boundaries, string +// literals, function calls, and type casts are handled in one place (#591). + +// identifierMatchMode selects the boundary rules for a match. +type identifierMatchMode int + +const ( + // relationMatch matches a relation (table or view) name; a following "(" + // or a preceding "::" still counts, e.g. "INTO t(...)" or "::t" rowtype casts. + relationMatch identifierMatchMode = iota + // columnMatch matches a column reference inside an expression; a name + // followed by "(" is a function call and one preceded by ":" is a type + // cast, neither of which is a column. + columnMatch +) + +// sqlStringLiteralRegex matches a single-quoted SQL string literal, including +// doubled-quote escapes. +var sqlStringLiteralRegex = regexp.MustCompile(`'(?:[^']|'')*'`) + +// identifierRegexpCache memoizes compiled patterns; view and expression +// dependency checks run the same names over many definitions. +var identifierRegexpCache sync.Map // map[string]*regexp.Regexp + +// identifierSpellings renders the two ways a deparser can spell name: bare, or +// double-quoted with embedded quotes doubled. +func identifierSpellings(name string) string { + bare := regexp.QuoteMeta(name) + quoted := regexp.QuoteMeta(`"` + strings.ReplaceAll(name, `"`, `""`) + `"`) + return `(?:` + bare + `|` + quoted + `)` +} + +// identifierRegexp returns a pattern matching name as a whole identifier in +// deparsed SQL. parts holds the name's qualification segments (schema, name) +// or a single unqualified name; each segment may appear bare or quoted. +func identifierRegexp(mode identifierMatchMode, parts ...string) *regexp.Regexp { + key := string(rune('0'+mode)) + "\x00" + strings.Join(parts, "\x00") + if re, ok := identifierRegexpCache.Load(key); ok { + return re.(*regexp.Regexp) + } + + spellings := make([]string, len(parts)) + for i, part := range parts { + spellings[i] = identifierSpellings(part) + } + body := strings.Join(spellings, `\.`) + + // Identifier characters never border a whole-identifier match, and a + // quote cannot either, so a bare spelling never matches inside a quoted + // identifier. A qualified name must not sit inside a longer path. + before, after := `[^\w$"]`, `[^\w$"]` + if len(parts) > 1 { + before, after = `[^\w$".]`, `[^\w$".]` + } + if mode == columnMatch { + before = before[:len(before)-1] + `:]` + after = after[:len(after)-1] + `(]` + } + re := regexp.MustCompile(`(?i)(?:^|` + before + `)` + body + `(?:` + after + `|$)`) + identifierRegexpCache.Store(key, re) + return re +} + +// stripStringLiterals blanks out single-quoted literals so their contents +// cannot look like identifiers. +func stripStringLiterals(sqlText string) string { + if !strings.Contains(sqlText, "'") { + return sqlText + } + return sqlStringLiteralRegex.ReplaceAllString(sqlText, "''") +} + +// containsIdentifier reports whether sqlText mentions identifier as a whole +// relation name, in bare or quoted form. A "schema.name" identifier is +// matched segment by segment, so "my schema"."a""b" is found for +// `my schema.a"b`, and "foo" does not match "foobar" or "other.foo.bar". +func containsIdentifier(sqlText, identifier string) bool { + if sqlText == "" || identifier == "" { + return false + } + if schema, name, ok := strings.Cut(identifier, "."); ok { + return containsQualifiedIdentifier(sqlText, schema, name) + } + return identifierRegexp(relationMatch, identifier).MatchString(stripStringLiterals(sqlText)) +} + +// containsQualifiedIdentifier reports whether sqlText mentions schema.name as +// a whole qualified relation name, each segment bare or quoted. +func containsQualifiedIdentifier(sqlText, schema, name string) bool { + if sqlText == "" || schema == "" || name == "" { + return false + } + return identifierRegexp(relationMatch, schema, name).MatchString(stripStringLiterals(sqlText)) +} + +// exprReferencesAnyColumn reports whether a deparsed SQL expression names any +// of the columns as a bare or quoted identifier. Callers pair a positive +// result with IF EXISTS drops and a re-create from the desired state, so a +// false positive costs a redundant drop + create while a miss would leave a +// dependent object behind. (#591) +func exprReferencesAnyColumn(expr string, columns map[string]bool) bool { + if expr == "" || len(columns) == 0 { + return false + } + expr = stripStringLiterals(expr) + for column := range columns { + if identifierRegexp(columnMatch, column).MatchString(expr) { + return true + } + } + return false +} diff --git a/internal/diff/identifier_match_test.go b/internal/diff/identifier_match_test.go new file mode 100644 index 00000000..ba474c3a --- /dev/null +++ b/internal/diff/identifier_match_test.go @@ -0,0 +1,77 @@ +package diff + +import ( + "testing" + + "github.com/pgplex/pgschema/ir" +) + +func TestContainsIdentifier(t *testing.T) { + cases := []struct { + text, ident string + want bool + }{ + {"SELECT id FROM users", "users", true}, + {"SELECT id FROM public.users", "users", true}, + {"SELECT id FROM public.users", "public.users", true}, + {"SELECT id FROM other.public.users", "public.users", false}, + {`SELECT id FROM "Users"`, "Users", true}, + {`SELECT id FROM "a""b"`, `a"b`, true}, + {`SELECT id FROM "a"b"`, `a"b`, false}, + {`SELECT id FROM "my schema"."a""b"`, `my schema.a"b`, true}, + {"SELECT id FROM foobar", "foo", false}, + {"SELECT id FROM foo_bar", "foo", false}, + {"SELECT id FROM foo$bar", "foo", false}, + {"SELECT 'users' FROM t", "users", false}, // string literal + {"SELECT (row)::users FROM t", "users", true}, + {"", "users", false}, + } + for _, c := range cases { + if got := containsIdentifier(c.text, c.ident); got != c.want { + t.Errorf("containsIdentifier(%q, %q) = %v, want %v", c.text, c.ident, got, c.want) + } + } +} + +func TestViewDependsOnTable_QuotedNames(t *testing.T) { + view := &ir.View{Schema: "public", Name: "v", Definition: ` SELECT c FROM "a""b";`} + if !viewDependsOnTable(view, "public", `a"b`) { + t.Errorf("expected view to depend on table a\"b") + } + qualified := &ir.View{Schema: "public", Name: "v", Definition: ` SELECT c FROM "my schema"."Orders";`} + if !viewDependsOnTable(qualified, "my schema", "Orders") { + t.Errorf("expected view to depend on \"my schema\".\"Orders\"") + } + if viewDependsOnTable(qualified, "my schema", "Order") { + t.Errorf("did not expect a prefix match") + } +} + +func TestExprReferencesAnyColumn(t *testing.T) { + cols := map[string]bool{"b": true, "my col": true, `a"b`: true} + cases := []struct { + expr string + want bool + }{ + {"(b + 1)", true}, + {"b", true}, + {"(t.b > 0)", true}, + {"(\"my col\" * 2)", true}, + {"(b > 10)", true}, + {"(bb + 1)", false}, + {"(a + 1)", false}, + {"b(a)", false}, // function call, not a column + {"('b'::text)", false}, // string literal + {`("a""b" + 1)`, true}, // embedded quote doubled by pg_get_expr + {`("a"b" + 1)`, false}, + {"(b$x + 1)", false}, // $ is part of the identifier + {"(x$b + 1)", false}, + {"((a)::b)", false}, // type cast, not a column + {"", false}, + } + for _, c := range cases { + if got := exprReferencesAnyColumn(c.expr, cols); got != c.want { + t.Errorf("exprReferencesAnyColumn(%q) = %v, want %v", c.expr, got, c.want) + } + } +} diff --git a/internal/diff/table.go b/internal/diff/table.go index f51f2729..6eb341bd 100644 --- a/internal/diff/table.go +++ b/internal/diff/table.go @@ -2,7 +2,6 @@ package diff import ( "fmt" - "regexp" "sort" "strings" @@ -1175,35 +1174,6 @@ func indexReferencesColumns(index *ir.Index, columns map[string]bool) bool { return index.IsPartial && exprReferencesAnyColumn(index.Where, columns) } -// sqlStringLiteralRegex matches a single-quoted SQL string literal, including -// doubled-quote escapes. -var sqlStringLiteralRegex = regexp.MustCompile(`'(?:[^']|'')*'`) - -// exprReferencesAnyColumn reports whether a SQL expression as rendered by -// pg_get_expr mentions any of the columns as a bare or quoted identifier. -// String literals are blanked out first; a name directly followed by "(" is a -// function call and a name directly preceded by ":" is a type cast, not a -// column. Callers pair a positive result with IF EXISTS drops and a re-create -// from the desired state, so a false positive costs a redundant drop + create -// while a miss would leave a dependent object behind. (#591) -func exprReferencesAnyColumn(expr string, columns map[string]bool) bool { - if expr == "" || len(columns) == 0 { - return false - } - expr = sqlStringLiteralRegex.ReplaceAllString(expr, "''") - for column := range columns { - bare := regexp.QuoteMeta(column) - // pg_get_expr doubles embedded quotes inside a quoted identifier. - quoted := regexp.QuoteMeta(`"` + strings.ReplaceAll(column, `"`, `""`) + `"`) - // \w plus $ covers every character of an unquoted identifier. - re := regexp.MustCompile(`(?:^|[^\w$":])(?:` + bare + `|` + quoted + `)(?:[^\w$"(]|$)`) - if re.MatchString(expr) { - return true - } - } - return false -} - // generateAlterTableStatements generates SQL statements for table modifications // Note: DroppedTriggers are skipped here because they are already processed in the DROP phase // (see generateDropTriggersFromModifiedTables in trigger.go) diff --git a/internal/diff/view.go b/internal/diff/view.go index 464de616..cd7e5252 100644 --- a/internal/diff/view.go +++ b/internal/diff/view.go @@ -2,7 +2,6 @@ package diff import ( "fmt" - "regexp" "sort" "strings" @@ -806,36 +805,6 @@ func viewDependsOnView(viewA *ir.View, viewBName string) bool { return containsIdentifier(viewA.Definition, viewBName) } -// containsIdentifier checks if the given SQL text contains the identifier as a whole word. -// This uses word boundary matching to avoid false positives (e.g., "user" matching "users"). -func containsIdentifier(sqlText, identifier string) bool { - if sqlText == "" || identifier == "" { - return false - } - - // Build a regex pattern that matches the identifier as a whole word. - // Word boundaries in SQL are: start/end of string, whitespace, punctuation, operators. - // We use a pattern that matches the identifier not preceded/followed by word characters. - // - // For schema-qualified identifiers (containing a dot), treat '.' as part of the word - // to avoid matching inside longer qualified paths like "other.schema.name". - // Use [^a-zA-Z0-9_] to exclude both upper and lowercase letters as word boundaries. - var pattern string - if strings.Contains(identifier, ".") { - pattern = `(?i)(?:^|[^a-zA-Z0-9_.])` + regexp.QuoteMeta(identifier) + `(?:[^a-zA-Z0-9_.]|$)` - } else { - pattern = `(?i)(?:^|[^a-zA-Z0-9_])` + regexp.QuoteMeta(identifier) + `(?:[^a-zA-Z0-9_]|$)` - } - matched, err := regexp.MatchString(pattern, sqlText) - if err != nil { - // This should never happen since regexp.QuoteMeta ensures valid pattern, - // but log it rather than silently ignoring - fmt.Printf("containsIdentifier: regexp error for pattern %q: %v\n", pattern, err) - return false - } - return matched -} - // viewDependsOnTable checks if a view depends on a specific table // by checking if the table name appears in the view definition. // Uses whole-word identifier matching (not plain substring) so that a table @@ -851,7 +820,7 @@ func viewDependsOnTable(view *ir.View, tableSchema, tableName string) bool { } // Check for qualified table name (schema.table) - if containsIdentifier(view.Definition, tableSchema+"."+tableName) { + if containsQualifiedIdentifier(view.Definition, tableSchema, tableName) { return true } From eb21036337d996d67bb66e4bcafca88a330d6f6e Mon Sep 17 00:00:00 2001 From: tianzhou Date: Wed, 9 Sep 2026 21:02:39 -0700 Subject: [PATCH 06/11] fix: strip quoted schema qualifiers and keep dotted names whole in dependency checks (#591) Fifth round of review follow-ups: - StripSchemaPrefixFromBody also strips the quote_ident form of the schema ("My Schema".calc(a) -> calc(a)), which the deparsers render for schema names that need quoting. Without it the current state kept the qualifier while the desired state lost it, so a generated column in such a schema was rewritten on every plan. Index expressions and function bodies gain the same normalization. - containsIdentifier tries a dotted name as one identifier (a quoted "a.b") before falling back to schema.name matching, restoring the pre-refactor behaviour for such column names; dotted names never match inside a longer qualified path. - viewDependsOnRecreatedColumn iterates the table diffs instead of parsing a flattened schema.table key, so a schema name containing a dot works. Co-Authored-By: Claude Fable 5.1 --- internal/diff/diff.go | 11 +++++------ internal/diff/identifier_match.go | 25 ++++++++++++++++++------- internal/diff/identifier_match_test.go | 2 ++ ir/normalize.go | 13 ++++++++++++- ir/normalize_test.go | 23 ++++++++++++++++++++++- 5 files changed, 59 insertions(+), 15 deletions(-) diff --git a/internal/diff/diff.go b/internal/diff/diff.go index 624127ac..fb6928fd 100644 --- a/internal/diff/diff.go +++ b/internal/diff/diff.go @@ -948,7 +948,7 @@ func generateMigration(oldIR, newIR *ir.IR, targetSchema string, qualifySchema b // COLUMN) would block the DROP with SQLSTATE 2BP01, so it goes // through the pre-drop/recreate cycle even when unchanged (#591). // The live (old) definition is what holds the dependency. - dependsOnRecreated := viewDependsOnRecreatedColumn(oldView, recreatedColumnsByTable) + dependsOnRecreated := viewDependsOnRecreatedColumn(oldView, diff.modifiedTables) // Check if the view definition itself changed (excluding options). // This is used to decide if materialized views need DROP+CREATE: // option-only changes should use ALTER VIEW SET/RESET, not recreation. @@ -2420,13 +2420,12 @@ func collectRecreatedColumns(modifiedTables []*tableDiff) map[string]map[string] // column and created again afterwards. The column check is a textual match on // the view definition, so a same-named column of another table the view also // reads can cause a redundant recreation. (#591) -func viewDependsOnRecreatedColumn(view *ir.View, recreatedColumnsByTable map[string]map[string]bool) bool { - for tableKey, columns := range recreatedColumnsByTable { - schema, table, ok := strings.Cut(tableKey, ".") - if !ok { +func viewDependsOnRecreatedColumn(view *ir.View, modifiedTables []*tableDiff) bool { + for _, td := range modifiedTables { + if len(td.RecreatedColumns) == 0 { continue } - if viewDependsOnTable(view, schema, table) && exprReferencesAnyColumn(view.Definition, columns) { + if viewDependsOnTable(view, td.Table.Schema, td.Table.Name) && exprReferencesAnyColumn(view.Definition, td.RecreatedColumns) { return true } } diff --git a/internal/diff/identifier_match.go b/internal/diff/identifier_match.go index 9649bc1c..99d40870 100644 --- a/internal/diff/identifier_match.go +++ b/internal/diff/identifier_match.go @@ -60,9 +60,14 @@ func identifierRegexp(mode identifierMatchMode, parts ...string) *regexp.Regexp // Identifier characters never border a whole-identifier match, and a // quote cannot either, so a bare spelling never matches inside a quoted - // identifier. A qualified name must not sit inside a longer path. + // identifier. A qualified name (or a name that itself contains a dot) + // must not sit inside a longer path such as other.schema.name. + dotted := len(parts) > 1 + for _, part := range parts { + dotted = dotted || strings.Contains(part, ".") + } before, after := `[^\w$"]`, `[^\w$"]` - if len(parts) > 1 { + if dotted { before, after = `[^\w$".]`, `[^\w$".]` } if mode == columnMatch { @@ -84,17 +89,23 @@ func stripStringLiterals(sqlText string) string { } // containsIdentifier reports whether sqlText mentions identifier as a whole -// relation name, in bare or quoted form. A "schema.name" identifier is -// matched segment by segment, so "my schema"."a""b" is found for -// `my schema.a"b`, and "foo" does not match "foobar" or "other.foo.bar". +// relation name, in bare or quoted form; "foo" does not match "foobar". An +// identifier containing a dot is tried both as one name (a quoted "a.b") and +// as "schema.name" matched segment by segment, so "my schema"."a""b" is found +// for `my schema.a"b` and "other.foo.bar" does not match "foo.bar". Callers +// that hold schema and name separately should use containsQualifiedIdentifier. func containsIdentifier(sqlText, identifier string) bool { if sqlText == "" || identifier == "" { return false } + sqlText = stripStringLiterals(sqlText) + if identifierRegexp(relationMatch, identifier).MatchString(sqlText) { + return true + } if schema, name, ok := strings.Cut(identifier, "."); ok { - return containsQualifiedIdentifier(sqlText, schema, name) + return identifierRegexp(relationMatch, schema, name).MatchString(sqlText) } - return identifierRegexp(relationMatch, identifier).MatchString(stripStringLiterals(sqlText)) + return false } // containsQualifiedIdentifier reports whether sqlText mentions schema.name as diff --git a/internal/diff/identifier_match_test.go b/internal/diff/identifier_match_test.go index ba474c3a..40c5d40a 100644 --- a/internal/diff/identifier_match_test.go +++ b/internal/diff/identifier_match_test.go @@ -20,6 +20,8 @@ func TestContainsIdentifier(t *testing.T) { {`SELECT id FROM "a"b"`, `a"b`, false}, {`SELECT id FROM "my schema"."a""b"`, `my schema.a"b`, true}, {"SELECT id FROM foobar", "foo", false}, + {`SELECT "a.b" FROM t`, "a.b", true}, // dotted name is one identifier when quoted + {"SELECT a.b FROM t", "a.b", true}, {"SELECT id FROM foo_bar", "foo", false}, {"SELECT id FROM foo$bar", "foo", false}, {"SELECT 'users' FROM t", "users", false}, // string literal diff --git a/ir/normalize.go b/ir/normalize.go index e825e860..ad68130b 100644 --- a/ir/normalize.go +++ b/ir/normalize.go @@ -447,12 +447,23 @@ func normalizeFunctionDefinition(def string) string { // StripSchemaPrefixFromBody removes the current schema qualifier from identifiers // in a function or procedure body. For example, "public.users" becomes "users". // It skips single-quoted string literals to avoid modifying string constants. +// A schema name that needs quoting is recognized in the form the deparsers +// render it, e.g. "My Schema".calc(a) becomes calc(a). func StripSchemaPrefixFromBody(body, schema string) string { if body == "" || schema == "" { return body } - prefix := schema + "." + if quoted := QuoteIdentifier(schema); quoted != schema { + body = stripSchemaPrefixOccurrences(body, quoted+".") + } + return stripSchemaPrefixOccurrences(body, schema+".") +} + +// stripSchemaPrefixOccurrences removes every occurrence of prefix ("schema." +// in bare or quote_ident form) that starts an identifier reference outside +// string literals, quoting the remaining identifier when it is a reserved word. +func stripSchemaPrefixOccurrences(body, prefix string) string { prefixLen := len(prefix) // Fast path: if the prefix doesn't appear at all, return as-is diff --git a/ir/normalize_test.go b/ir/normalize_test.go index 09cf9838..7fbe8024 100644 --- a/ir/normalize_test.go +++ b/ir/normalize_test.go @@ -583,7 +583,7 @@ func TestStripRedundantTextCast(t *testing.T) { {"('new'::character varying)::text", "'new'::character varying"}, {"('new'::varchar)::text", "'new'::varchar"}, {"'new'::character varying::text", "'new'::character varying"}, - {"'new'::character varying", "'new'::character varying"}, // already clean + {"'new'::character varying", "'new'::character varying"}, // already clean {"'active'::public.status_type", "'active'::public.status_type"}, // custom type untouched {"42", "42"}, } @@ -669,3 +669,24 @@ func TestMarkSerialColumns(t *testing.T) { } } } + +// A schema name that needs quoting is rendered by the deparsers in quote_ident +// form; the qualifier must be stripped in that form too, or the current and +// desired states of a generated column expression never compare equal (#591). +func TestStripSchemaPrefixFromBody_QuotedSchema(t *testing.T) { + cases := []struct { + body, schema, want string + }{ + {`"My Schema".calc(a)`, "My Schema", `calc(a)`}, + {`("My Schema".calc(a) + "My Schema"."Other"(b))`, "My Schema", `(calc(a) + "Other"(b))`}, + {`'"My Schema".calc'`, "My Schema", `'"My Schema".calc'`}, + {`"my""s".calc(a)`, `my"s`, `calc(a)`}, + {`public.calc(a)`, "public", `calc(a)`}, + {`other.calc(a)`, "My Schema", `other.calc(a)`}, + } + for _, c := range cases { + if got := StripSchemaPrefixFromBody(c.body, c.schema); got != c.want { + t.Errorf("StripSchemaPrefixFromBody(%q, %q) = %q, want %q", c.body, c.schema, got, c.want) + } + } +} From 8d4ed7006d0c86dc574641da898f591e74a837e3 Mon Sep 17 00:00:00 2001 From: tianzhou Date: Wed, 9 Sep 2026 21:42:45 -0700 Subject: [PATCH 07/11] fix: cover expression EXCLUDE constraints, deferred FKs, and quoted identifiers around a replaced generated column (#591) Sixth round of review follow-ups: - An EXCLUDE constraint that references a re-created column only through an expression element (conkey records 0) is detected from its definition text, dropped explicitly ahead of the column, and added back afterwards. - A foreign key newly added to an existing table that targets a unique index rebuilt with a re-created column is deferred to the post-add step instead of being emitted (possibly inline) before the index exists again. - StripSchemaPrefixFromBody copies double-quoted identifiers verbatim unless the token is the quoted schema itself, so a column literally named "public.foo" is no longer rewritten to "foo". Co-Authored-By: Claude Fable 5.1 --- internal/diff/diff.go | 2 +- internal/diff/table.go | 50 +++++++++++++++++-- ir/normalize.go | 23 +++++++++ ir/normalize_test.go | 2 + .../diff.sql | 8 +++ .../new.sql | 9 +++- .../old.sql | 8 ++- .../plan.json | 34 ++++++++++++- .../plan.sql | 10 ++++ .../plan.txt | 24 +++++++-- 10 files changed, 158 insertions(+), 12 deletions(-) diff --git a/internal/diff/diff.go b/internal/diff/diff.go index fb6928fd..adfdf216 100644 --- a/internal/diff/diff.go +++ b/internal/diff/diff.go @@ -2247,7 +2247,7 @@ func (d *ddlDiff) generateModifySQL(targetSchema string, collector *diffCollecto generateDropRecreatedFKsSQL(d.fkPreDrops, targetSchema, collector) // Modify tables - generateModifyTablesSQL(d.modifiedTables, d.droppedTables, d.fkPreDrops, targetSchema, collector) + generateModifyTablesSQL(d.modifiedTables, d.droppedTables, d.fkPreDrops, d.suppressedInlineFKs, targetSchema, collector) // Attach OWNED BY for explicitly created sequences whose owning column was // created by this migration, either with a new table (create phase) or by diff --git a/internal/diff/table.go b/internal/diff/table.go index 6eb341bd..703e64c6 100644 --- a/internal/diff/table.go +++ b/internal/diff/table.go @@ -262,7 +262,11 @@ func diffTables(oldTable, newTable *ir.Table, targetSchema string, targetMajorVe // constraintDroppedWithColumns) also lets // planFKRecreationForReplacedConstraints re-bind foreign keys that // depend on a unique/PK constraint among them. (#591) - if constraintDroppedWithColumns(newConstraint, recreatedColumns) { + // An EXCLUDE constraint can reference the column only through an + // expression, which conkey records as 0, so its definition text is + // checked as well; its DROP is then emitted explicitly (before the + // column drop) because constraintDroppedWithColumns will not skip it. + if constraintDroppedWithColumns(newConstraint, recreatedColumns) || exclusionReferencesColumns(oldConstraint, recreatedColumns) { diff.DroppedConstraints = append(diff.DroppedConstraints, oldConstraint) diff.AddedConstraints = append(diff.AddedConstraints, newConstraint) continue @@ -624,7 +628,9 @@ func generateDeferredConstraintsSQL(deferred []*deferredConstraint, targetSchema } // generateModifyTablesSQL generates ALTER TABLE statements -func generateModifyTablesSQL(diffs []*tableDiff, droppedTables []*ir.Table, fkPreDrops []*ir.Constraint, targetSchema string, collector *diffCollector) { +// deferredFKs holds "schema.table.constraint" keys of added foreign keys that +// are emitted by the post-add step instead of with their table's changes. +func generateModifyTablesSQL(diffs []*tableDiff, droppedTables []*ir.Table, fkPreDrops []*ir.Constraint, deferredFKs map[string]bool, targetSchema string, collector *diffCollector) { // Build a set of tables being dropped (CASCADE will remove their dependent FK constraints) droppedTableSet := make(map[string]bool, len(droppedTables)) for _, t := range droppedTables { @@ -647,7 +653,7 @@ func generateModifyTablesSQL(diffs []*tableDiff, droppedTables []*ir.Table, fkPr } // Pass collector to generateAlterTableStatements to collect with proper context - diff.generateAlterTableStatements(targetSchema, collector, droppedTableSet, droppedColumnSet, preDroppedFKSet) + diff.generateAlterTableStatements(targetSchema, collector, droppedTableSet, droppedColumnSet, preDroppedFKSet, deferredFKs) } } @@ -779,6 +785,23 @@ func planFKRecreationForReplacedConstraints(modifiedTables []*tableDiff, addedTa } } + // FKs newly added to existing tables that target a unique index rebuilt + // with a re-created column: the ALTER TABLE ... ADD CONSTRAINT would run + // before the index is back (always for a self-reference, and depending + // on table order otherwise), so they are deferred the same way. (#591) + for _, td := range modifiedTables { + for _, fk := range td.AddedConstraints { + if fk.Type != ir.ConstraintTypeForeignKey { + continue + } + if !fkReferencesAnyUniqueIndex(fk, replacedUniqueIndexes[fkReferencedTableKey(fk)]) { + continue + } + suppressedInlineFKs[constraintPathKey(fk)] = true + postAdds = append(postAdds, &deferredConstraint{table: td.Table, constraint: fk}) + } + } + sort.Slice(preDrops, func(i, j int) bool { return constraintPathKey(preDrops[i]) < constraintPathKey(preDrops[j]) }) @@ -1131,6 +1154,14 @@ func (td *tableDiff) collectDropPolicy(policy *ir.RLSPolicy, targetSchema string collector.collect(context, sql) } +// exclusionReferencesColumns reports whether an EXCLUDE constraint's +// definition names any of the given columns inside an expression element, +// which pg_constraint.conkey does not record. (#591) +func exclusionReferencesColumns(constraint *ir.Constraint, columns map[string]bool) bool { + return constraint != nil && constraint.Type == ir.ConstraintTypeExclusion && + exprReferencesAnyColumn(constraint.ExclusionDefinition, columns) +} + // policyReferencesColumns reports whether a policy's USING or WITH CHECK // expression names any of the given columns. (#591) func policyReferencesColumns(policy *ir.RLSPolicy, columns map[string]bool) bool { @@ -1184,7 +1215,10 @@ func indexReferencesColumns(index *ir.Index, columns map[string]bool) bool { // preDroppedFKSet contains "schema.table.constraint" keys for FKs already dropped in the // pre-drop step because they were bound to a replaced unique/PK constraint; their drop // and modify entries are skipped since the pre-drop/post-add steps handle them. (#439) -func (td *tableDiff) generateAlterTableStatements(targetSchema string, collector *diffCollector, droppedTableSet map[string]bool, droppedColumnSet map[string]bool, preDroppedFKSet map[string]bool) { +// deferredFKSet contains "schema.table.constraint" keys of added FKs emitted by the +// post-add step (they target a key that is rebuilt in this migration); their normal +// and inline emission is skipped. (#591) +func (td *tableDiff) generateAlterTableStatements(targetSchema string, collector *diffCollector, droppedTableSet map[string]bool, droppedColumnSet map[string]bool, preDroppedFKSet map[string]bool, deferredFKSet map[string]bool) { // Persistence change (UNLOGGED to LOGGED or vice versa) should emit first // because PostgreSQL rewrites the heap so doing it before column/constraint // changes reduces data movement on subsequent steps @@ -1300,6 +1334,10 @@ func (td *tableDiff) generateAlterTableStatements(targetSchema string, collector // Check for single-column constraints that can be added inline var inlineConstraint string for _, constraint := range td.AddedConstraints { + // Deferred FKs are emitted by the post-add step. (#591) + if deferredFKSet[constraintPathKey(constraint)] { + continue + } // Only add inline for single-column constraints if len(constraint.Columns) == 1 && constraint.Columns[0].Name == column.Name { switch constraint.Type { @@ -1399,6 +1437,10 @@ func (td *tableDiff) generateAlterTableStatements(targetSchema string, collector if inlineConstraints[constraint.Name] { continue } + // Deferred FKs are emitted by the post-add step. (#591) + if deferredFKSet[constraintPathKey(constraint)] { + continue + } switch constraint.Type { case ir.ConstraintTypeUnique: diff --git a/ir/normalize.go b/ir/normalize.go index ad68130b..f4ccdfb8 100644 --- a/ir/normalize.go +++ b/ir/normalize.go @@ -495,6 +495,29 @@ func stripSchemaPrefixOccurrences(body, prefix string) string { continue } + // A double-quoted identifier is copied verbatim unless it is the quoted + // schema token itself (the prefix check below runs first for that), so + // a column literally named "public.foo" keeps its name. + if !inString && ch == '"' && !(i+prefixLen <= len(body) && body[i:i+prefixLen] == prefix) { + end := i + 1 + for end < len(body) { + if body[end] == '"' { + if end+1 < len(body) && body[end+1] == '"' { + end += 2 + continue + } + break + } + end++ + } + if end >= len(body) { + end = len(body) - 1 + } + result.WriteString(body[i : end+1]) + i = end + continue + } + // Only attempt replacement outside string literals if !inString && i+prefixLen <= len(body) && body[i:i+prefixLen] == prefix { // Ensure this is a schema qualifier, not part of a longer identifier diff --git a/ir/normalize_test.go b/ir/normalize_test.go index 7fbe8024..7d049bfd 100644 --- a/ir/normalize_test.go +++ b/ir/normalize_test.go @@ -683,6 +683,8 @@ func TestStripSchemaPrefixFromBody_QuotedSchema(t *testing.T) { {`"my""s".calc(a)`, `my"s`, `calc(a)`}, {`public.calc(a)`, "public", `calc(a)`}, {`other.calc(a)`, "My Schema", `other.calc(a)`}, + {`("public.foo" * 2)`, "public", `("public.foo" * 2)`}, // a column literally named public.foo + {`("My Schema".calc("My Schema.x"))`, "My Schema", `(calc("My Schema.x"))`}, // quoted schema token vs quoted column } for _, c := range cases { if got := StripSchemaPrefixFromBody(c.body, c.schema); got != c.want { diff --git a/testdata/diff/create_table/issue_591_recreate_generated_dependents/diff.sql b/testdata/diff/create_table/issue_591_recreate_generated_dependents/diff.sql index b442b22c..4faf8e59 100644 --- a/testdata/diff/create_table/issue_591_recreate_generated_dependents/diff.sql +++ b/testdata/diff/create_table/issue_591_recreate_generated_dependents/diff.sql @@ -10,6 +10,8 @@ ALTER TABLE shipments DROP CONSTRAINT shipments_order_code_fkey; DROP POLICY IF EXISTS orders_big ON orders; +ALTER TABLE orders DROP CONSTRAINT orders_total_excl; + ALTER TABLE orders DROP COLUMN total; ALTER TABLE orders DROP COLUMN code; @@ -18,6 +20,9 @@ ALTER TABLE orders ADD COLUMN total integer GENERATED ALWAYS AS ((qty * price)) ALTER TABLE orders ADD COLUMN code text GENERATED ALWAYS AS (('ORD-'::text || (id)::text)) STORED; +ALTER TABLE orders +ADD CONSTRAINT orders_total_excl EXCLUDE USING btree ((total + 0) WITH =); + CREATE OR REPLACE TRIGGER orders_total_trg AFTER UPDATE ON orders FOR EACH ROW @@ -34,6 +39,9 @@ DROP INDEX IF EXISTS orders_lookup_idx; CREATE INDEX IF NOT EXISTS orders_lookup_idx ON orders (total); +ALTER TABLE returns +ADD CONSTRAINT returns_order_code_fkey FOREIGN KEY (order_code) REFERENCES orders (code); + ALTER TABLE shipments ADD CONSTRAINT shipments_order_code_fkey FOREIGN KEY (order_code) REFERENCES orders (code); diff --git a/testdata/diff/create_table/issue_591_recreate_generated_dependents/new.sql b/testdata/diff/create_table/issue_591_recreate_generated_dependents/new.sql index 65a1b411..6e745db2 100644 --- a/testdata/diff/create_table/issue_591_recreate_generated_dependents/new.sql +++ b/testdata/diff/create_table/issue_591_recreate_generated_dependents/new.sql @@ -14,7 +14,8 @@ CREATE TABLE public.orders ( qty integer NOT NULL, price integer NOT NULL, total integer GENERATED ALWAYS AS (qty * price) STORED, - code text GENERATED ALWAYS AS ('ORD-' || id::text) STORED + code text GENERATED ALWAYS AS ('ORD-' || id::text) STORED, + CONSTRAINT orders_total_excl EXCLUDE USING btree ((total + 0) WITH =) ); CREATE UNIQUE INDEX orders_code_key ON public.orders (code); @@ -27,6 +28,12 @@ CREATE TABLE public.shipments ( CONSTRAINT shipments_order_code_fkey FOREIGN KEY (order_code) REFERENCES public.orders (code) ); +CREATE TABLE public.returns ( + id integer PRIMARY KEY, + order_code text, + CONSTRAINT returns_order_code_fkey FOREIGN KEY (order_code) REFERENCES public.orders (code) +); + CREATE VIEW public.order_totals AS SELECT id, total FROM public.orders; CREATE VIEW public.big_orders AS SELECT id FROM public.order_totals WHERE total > 100; diff --git a/testdata/diff/create_table/issue_591_recreate_generated_dependents/old.sql b/testdata/diff/create_table/issue_591_recreate_generated_dependents/old.sql index 459d4f1e..657fbbf1 100644 --- a/testdata/diff/create_table/issue_591_recreate_generated_dependents/old.sql +++ b/testdata/diff/create_table/issue_591_recreate_generated_dependents/old.sql @@ -14,7 +14,8 @@ CREATE TABLE public.orders ( qty integer NOT NULL, price integer NOT NULL, total integer, - code text + code text, + CONSTRAINT orders_total_excl EXCLUDE USING btree ((total + 0) WITH =) ); CREATE UNIQUE INDEX orders_code_key ON public.orders (code); @@ -27,6 +28,11 @@ CREATE TABLE public.shipments ( CONSTRAINT shipments_order_code_fkey FOREIGN KEY (order_code) REFERENCES public.orders (code) ); +CREATE TABLE public.returns ( + id integer PRIMARY KEY, + order_code text +); + CREATE VIEW public.order_totals AS SELECT id, total FROM public.orders; CREATE VIEW public.big_orders AS SELECT id FROM public.order_totals WHERE total > 100; diff --git a/testdata/diff/create_table/issue_591_recreate_generated_dependents/plan.json b/testdata/diff/create_table/issue_591_recreate_generated_dependents/plan.json index f9d6f948..e6384daf 100644 --- a/testdata/diff/create_table/issue_591_recreate_generated_dependents/plan.json +++ b/testdata/diff/create_table/issue_591_recreate_generated_dependents/plan.json @@ -3,7 +3,7 @@ "pgschema_version": "1.13.0", "created_at": "1970-01-01T00:00:00Z", "source_fingerprint": { - "hash": "d55cddab4cdd29708f6dd7af47adf0a45bcbf022f435f3c131b17a3b124a5f61" + "hash": "8c9711295d6cb9aae61a1a685ce7b7060849c2c394261f84b1ab57f92bdd0f21" }, "groups": [ { @@ -44,6 +44,12 @@ "operation": "drop", "path": "public.orders.orders_big" }, + { + "sql": "ALTER TABLE orders DROP CONSTRAINT orders_total_excl;", + "type": "table.constraint", + "operation": "drop", + "path": "public.orders.orders_total_excl" + }, { "sql": "ALTER TABLE orders DROP COLUMN total;", "type": "table.column", @@ -68,6 +74,12 @@ "operation": "create", "path": "public.orders.code" }, + { + "sql": "ALTER TABLE orders\nADD CONSTRAINT orders_total_excl EXCLUDE USING btree ((total + 0) WITH =);", + "type": "table.constraint", + "operation": "create", + "path": "public.orders.orders_total_excl" + }, { "sql": "CREATE OR REPLACE TRIGGER orders_total_trg\n AFTER UPDATE ON orders\n FOR EACH ROW\n WHEN (((NEW.total > 0)))\n EXECUTE FUNCTION orders_audit();", "type": "table.trigger", @@ -160,6 +172,26 @@ "operation": "alter", "path": "public.orders.orders_lookup_idx" }, + { + "sql": "ALTER TABLE returns\nADD CONSTRAINT returns_order_code_fkey FOREIGN KEY (order_code) REFERENCES orders (code) NOT VALID;", + "type": "table.constraint", + "operation": "create", + "path": "public.returns.returns_order_code_fkey" + } + ] + }, + { + "steps": [ + { + "sql": "ALTER TABLE returns VALIDATE CONSTRAINT returns_order_code_fkey;", + "type": "table.constraint", + "operation": "create", + "path": "public.returns.returns_order_code_fkey" + } + ] + }, + { + "steps": [ { "sql": "ALTER TABLE shipments\nADD CONSTRAINT shipments_order_code_fkey FOREIGN KEY (order_code) REFERENCES orders (code) NOT VALID;", "type": "table.constraint", diff --git a/testdata/diff/create_table/issue_591_recreate_generated_dependents/plan.sql b/testdata/diff/create_table/issue_591_recreate_generated_dependents/plan.sql index e3ebbdee..9dbc3dc2 100644 --- a/testdata/diff/create_table/issue_591_recreate_generated_dependents/plan.sql +++ b/testdata/diff/create_table/issue_591_recreate_generated_dependents/plan.sql @@ -10,6 +10,8 @@ ALTER TABLE shipments DROP CONSTRAINT shipments_order_code_fkey; DROP POLICY IF EXISTS orders_big ON orders; +ALTER TABLE orders DROP CONSTRAINT orders_total_excl; + ALTER TABLE orders DROP COLUMN total; ALTER TABLE orders DROP COLUMN code; @@ -18,6 +20,9 @@ ALTER TABLE orders ADD COLUMN total integer GENERATED ALWAYS AS ((qty * price)) ALTER TABLE orders ADD COLUMN code text GENERATED ALWAYS AS (('ORD-'::text || (id)::text)) STORED; +ALTER TABLE orders +ADD CONSTRAINT orders_total_excl EXCLUDE USING btree ((total + 0) WITH =); + CREATE OR REPLACE TRIGGER orders_total_trg AFTER UPDATE ON orders FOR EACH ROW @@ -62,6 +67,11 @@ DROP INDEX IF EXISTS orders_lookup_idx; ALTER INDEX orders_lookup_idx_pgschema_new RENAME TO orders_lookup_idx; +ALTER TABLE returns +ADD CONSTRAINT returns_order_code_fkey FOREIGN KEY (order_code) REFERENCES orders (code) NOT VALID; + +ALTER TABLE returns VALIDATE CONSTRAINT returns_order_code_fkey; + ALTER TABLE shipments ADD CONSTRAINT shipments_order_code_fkey FOREIGN KEY (order_code) REFERENCES orders (code) NOT VALID; diff --git a/testdata/diff/create_table/issue_591_recreate_generated_dependents/plan.txt b/testdata/diff/create_table/issue_591_recreate_generated_dependents/plan.txt index ef73c809..57f2162c 100644 --- a/testdata/diff/create_table/issue_591_recreate_generated_dependents/plan.txt +++ b/testdata/diff/create_table/issue_591_recreate_generated_dependents/plan.txt @@ -1,7 +1,7 @@ -Plan: 2 to add, 5 to modify. +Plan: 2 to add, 6 to modify. Summary by type: - tables: 2 to modify + tables: 3 to modify views: 3 to modify privileges: 1 to add column privileges: 1 to add @@ -12,12 +12,16 @@ Tables: + code (column) - total (column) + total (column) + - orders_total_excl (constraint) + + orders_total_excl (constraint) ~ orders_code_key (index - concurrent rebuild) ~ orders_lookup_idx (index - concurrent rebuild) - orders_big (policy) + orders_big (policy) - orders_total_trg (trigger) + orders_total_trg (trigger) + ~ returns + + returns_order_code_fkey (constraint) ~ shipments - shipments_order_code_fkey (constraint) + shipments_order_code_fkey (constraint) @@ -49,6 +53,8 @@ ALTER TABLE shipments DROP CONSTRAINT shipments_order_code_fkey; DROP POLICY IF EXISTS orders_big ON orders; +ALTER TABLE orders DROP CONSTRAINT orders_total_excl; + ALTER TABLE orders DROP COLUMN total; ALTER TABLE orders DROP COLUMN code; @@ -57,6 +63,9 @@ ALTER TABLE orders ADD COLUMN total integer GENERATED ALWAYS AS ((qty * price)) ALTER TABLE orders ADD COLUMN code text GENERATED ALWAYS AS (('ORD-'::text || (id)::text)) STORED; +ALTER TABLE orders +ADD CONSTRAINT orders_total_excl EXCLUDE USING btree ((total + 0) WITH =); + CREATE OR REPLACE TRIGGER orders_total_trg AFTER UPDATE ON orders FOR EACH ROW @@ -107,13 +116,20 @@ DROP INDEX IF EXISTS orders_lookup_idx; ALTER INDEX orders_lookup_idx_pgschema_new RENAME TO orders_lookup_idx; +ALTER TABLE returns +ADD CONSTRAINT returns_order_code_fkey FOREIGN KEY (order_code) REFERENCES orders (code) NOT VALID; + +-- Transaction Group #8 +ALTER TABLE returns VALIDATE CONSTRAINT returns_order_code_fkey; + +-- Transaction Group #9 ALTER TABLE shipments ADD CONSTRAINT shipments_order_code_fkey FOREIGN KEY (order_code) REFERENCES orders (code) NOT VALID; --- Transaction Group #8 +-- Transaction Group #10 ALTER TABLE shipments VALIDATE CONSTRAINT shipments_order_code_fkey; --- Transaction Group #9 +-- Transaction Group #11 CREATE OR REPLACE VIEW order_labels AS SELECT id, 'x'::text AS label From 601595b342d87de7dce0436ddd438d30cbe310ec Mon Sep 17 00:00:00 2001 From: tianzhou Date: Wed, 9 Sep 2026 23:24:07 -0700 Subject: [PATCH 08/11] fix: defer FKs on newly added unique indexes and re-grant column privileges on recreated views (#591) Seventh round of review follow-ups: - Foreign keys that target a standalone unique index created by this migration are deferred to the post-add step whether the index is rebuilt with a re-created column or entirely new: an FK newly added to an existing table, or an existing FK whose new definition targets such an index, would otherwise be emitted before the index exists (always for a self-reference). - Column grants on views the migration drops and creates again are re-issued after the views exist again, like the object-level grants already were. Co-Authored-By: Claude Fable 5.1 --- internal/diff/diff.go | 16 ++--- internal/diff/table.go | 16 +++-- .../diff.sql | 7 +++ .../new.sql | 8 ++- .../old.sql | 3 + .../plan.json | 58 ++++++++++++++++++- .../plan.sql | 21 +++++++ .../plan.txt | 41 +++++++++++-- 8 files changed, 148 insertions(+), 22 deletions(-) diff --git a/internal/diff/diff.go b/internal/diff/diff.go index adfdf216..fc55d161 100644 --- a/internal/diff/diff.go +++ b/internal/diff/diff.go @@ -1498,18 +1498,20 @@ func generateMigration(oldIR, newIR *ir.IR, targetSchema string, qualifySchema b } } - // Desired grants that touch a column this migration re-creates, keyed - // like newColPrivs. DROP COLUMN discards that column's ACL, so such a - // grant must be issued again after the column exists, even when the old - // and desired grants match (#591). The old grant stays in the old state - // so that removals on surviving columns of a grouped grant are still - // revoked by the normal comparison. + // Desired grants that touch a column this migration re-creates, or sit on + // a view it drops and creates again, keyed like newColPrivs. DROP COLUMN + // discards that column's ACL and DROP VIEW the whole relation's, so such + // a grant must be issued again afterwards even when the old and desired + // grants match (#591). The old grant stays in the old state so that + // removals on surviving columns of a grouped grant are still revoked by + // the normal comparison. newColPrivsOnRecreated := make(map[string]bool) for _, dbSchema := range newIR.Schemas { for _, cp := range dbSchema.ColumnPrivileges { key := cp.GetFullKey() newColPrivs[key] = cp - if columnPrivilegeTouchesColumns(cp, recreatedColumnsByTable[dbSchema.Name+"."+cp.TableName]) { + relationKey := dbSchema.Name + "." + cp.TableName + if recreatedViewKeys[relationKey] || columnPrivilegeTouchesColumns(cp, recreatedColumnsByTable[relationKey]) { newColPrivsOnRecreated[key] = true } } diff --git a/internal/diff/table.go b/internal/diff/table.go index 703e64c6..478406ac 100644 --- a/internal/diff/table.go +++ b/internal/diff/table.go @@ -751,7 +751,8 @@ func planFKRecreationForReplacedConstraints(modifiedTables []*tableDiff, addedTa // was bound elsewhere. newBound := newFK != nil && !constraintsEqual(fk, newFK) && (fkReferencesAnyConstraint(newFK, replaced[fkReferencedTableKey(newFK)]) || - fkReferencesAnyUniqueIndex(newFK, replacedUniqueIndexes[fkReferencedTableKey(newFK)])) + fkReferencesAnyUniqueIndex(newFK, replacedUniqueIndexes[fkReferencedTableKey(newFK)]) || + fkReferencesAnyUniqueIndex(newFK, addedUniqueIndexes[fkReferencedTableKey(newFK)])) if !oldBound && !newBound { continue } @@ -785,16 +786,19 @@ func planFKRecreationForReplacedConstraints(modifiedTables []*tableDiff, addedTa } } - // FKs newly added to existing tables that target a unique index rebuilt - // with a re-created column: the ALTER TABLE ... ADD CONSTRAINT would run - // before the index is back (always for a self-reference, and depending - // on table order otherwise), so they are deferred the same way. (#591) + // FKs newly added to existing tables that target a unique index this + // migration creates, whether rebuilt with a re-created column or new: the + // ALTER TABLE ... ADD CONSTRAINT would run before the index exists + // (always for a self-reference, and depending on table order otherwise), + // so they are deferred the same way. (#591, #506) for _, td := range modifiedTables { for _, fk := range td.AddedConstraints { if fk.Type != ir.ConstraintTypeForeignKey { continue } - if !fkReferencesAnyUniqueIndex(fk, replacedUniqueIndexes[fkReferencedTableKey(fk)]) { + refKey := fkReferencedTableKey(fk) + if !fkReferencesAnyUniqueIndex(fk, replacedUniqueIndexes[refKey]) && + !fkReferencesAnyUniqueIndex(fk, addedUniqueIndexes[refKey]) { continue } suppressedInlineFKs[constraintPathKey(fk)] = true diff --git a/testdata/diff/create_table/issue_591_recreate_generated_dependents/diff.sql b/testdata/diff/create_table/issue_591_recreate_generated_dependents/diff.sql index 4faf8e59..18d3977b 100644 --- a/testdata/diff/create_table/issue_591_recreate_generated_dependents/diff.sql +++ b/testdata/diff/create_table/issue_591_recreate_generated_dependents/diff.sql @@ -39,9 +39,14 @@ DROP INDEX IF EXISTS orders_lookup_idx; CREATE INDEX IF NOT EXISTS orders_lookup_idx ON orders (total); +CREATE UNIQUE INDEX IF NOT EXISTS orders_total_key ON orders (total); + ALTER TABLE returns ADD CONSTRAINT returns_order_code_fkey FOREIGN KEY (order_code) REFERENCES orders (code); +ALTER TABLE returns +ADD CONSTRAINT returns_order_total_fkey FOREIGN KEY (order_total) REFERENCES orders (total); + ALTER TABLE shipments ADD CONSTRAINT shipments_order_code_fkey FOREIGN KEY (order_code) REFERENCES orders (code); @@ -62,4 +67,6 @@ CREATE OR REPLACE VIEW big_orders AS GRANT SELECT ON TABLE order_totals TO app_reader; +GRANT SELECT (id) ON TABLE big_orders TO app_reader; + GRANT SELECT (id, total) ON TABLE orders TO app_reader; diff --git a/testdata/diff/create_table/issue_591_recreate_generated_dependents/new.sql b/testdata/diff/create_table/issue_591_recreate_generated_dependents/new.sql index 6e745db2..7e71b963 100644 --- a/testdata/diff/create_table/issue_591_recreate_generated_dependents/new.sql +++ b/testdata/diff/create_table/issue_591_recreate_generated_dependents/new.sql @@ -22,6 +22,8 @@ CREATE UNIQUE INDEX orders_code_key ON public.orders (code); CREATE INDEX orders_lookup_idx ON public.orders (total); +CREATE UNIQUE INDEX orders_total_key ON public.orders (total); + CREATE TABLE public.shipments ( id integer PRIMARY KEY, order_code text, @@ -30,8 +32,10 @@ CREATE TABLE public.shipments ( CREATE TABLE public.returns ( id integer PRIMARY KEY, + order_total integer, order_code text, - CONSTRAINT returns_order_code_fkey FOREIGN KEY (order_code) REFERENCES public.orders (code) + CONSTRAINT returns_order_code_fkey FOREIGN KEY (order_code) REFERENCES public.orders (code), + CONSTRAINT returns_order_total_fkey FOREIGN KEY (order_total) REFERENCES public.orders (total) ); CREATE VIEW public.order_totals AS SELECT id, total FROM public.orders; @@ -51,3 +55,5 @@ CREATE TRIGGER orders_total_trg AFTER UPDATE ON public.orders FOR EACH ROW WHEN GRANT SELECT (id, total) ON public.orders TO app_reader; GRANT SELECT ON public.order_totals TO app_reader; + +GRANT SELECT (id) ON public.big_orders TO app_reader; diff --git a/testdata/diff/create_table/issue_591_recreate_generated_dependents/old.sql b/testdata/diff/create_table/issue_591_recreate_generated_dependents/old.sql index 657fbbf1..d1b465c8 100644 --- a/testdata/diff/create_table/issue_591_recreate_generated_dependents/old.sql +++ b/testdata/diff/create_table/issue_591_recreate_generated_dependents/old.sql @@ -30,6 +30,7 @@ CREATE TABLE public.shipments ( CREATE TABLE public.returns ( id integer PRIMARY KEY, + order_total integer, order_code text ); @@ -50,3 +51,5 @@ CREATE TRIGGER orders_total_trg AFTER UPDATE ON public.orders FOR EACH ROW WHEN GRANT SELECT (id, total) ON public.orders TO app_reader; GRANT SELECT ON public.order_totals TO app_reader; + +GRANT SELECT (id) ON public.big_orders TO app_reader; diff --git a/testdata/diff/create_table/issue_591_recreate_generated_dependents/plan.json b/testdata/diff/create_table/issue_591_recreate_generated_dependents/plan.json index e6384daf..d6d4e665 100644 --- a/testdata/diff/create_table/issue_591_recreate_generated_dependents/plan.json +++ b/testdata/diff/create_table/issue_591_recreate_generated_dependents/plan.json @@ -3,7 +3,7 @@ "pgschema_version": "1.13.0", "created_at": "1970-01-01T00:00:00Z", "source_fingerprint": { - "hash": "8c9711295d6cb9aae61a1a685ce7b7060849c2c394261f84b1ab57f92bdd0f21" + "hash": "9b049588f0373a22af4f2a06007f3c35b1f0bd7dcc3827d2c73456ef55009af2" }, "groups": [ { @@ -171,7 +171,35 @@ "type": "table.index", "operation": "alter", "path": "public.orders.orders_lookup_idx" - }, + } + ] + }, + { + "steps": [ + { + "sql": "CREATE UNIQUE INDEX CONCURRENTLY IF NOT EXISTS orders_total_key ON orders (total);", + "type": "table.index", + "operation": "create", + "path": "public.orders.orders_total_key" + } + ] + }, + { + "steps": [ + { + "sql": "SELECT \n COALESCE(i.indisvalid, false) as done,\n CASE \n WHEN p.blocks_total > 0 THEN p.blocks_done * 100 / p.blocks_total\n ELSE 0\n END as progress\nFROM pg_class c\nLEFT JOIN pg_index i ON c.oid = i.indexrelid\nLEFT JOIN pg_stat_progress_create_index p ON c.oid = p.index_relid\nWHERE c.relname = 'orders_total_key';", + "directive": { + "type": "wait", + "message": "Creating index orders_total_key" + }, + "type": "table.index", + "operation": "create", + "path": "public.orders.orders_total_key" + } + ] + }, + { + "steps": [ { "sql": "ALTER TABLE returns\nADD CONSTRAINT returns_order_code_fkey FOREIGN KEY (order_code) REFERENCES orders (code) NOT VALID;", "type": "table.constraint", @@ -190,6 +218,26 @@ } ] }, + { + "steps": [ + { + "sql": "ALTER TABLE returns\nADD CONSTRAINT returns_order_total_fkey FOREIGN KEY (order_total) REFERENCES orders (total) NOT VALID;", + "type": "table.constraint", + "operation": "create", + "path": "public.returns.returns_order_total_fkey" + } + ] + }, + { + "steps": [ + { + "sql": "ALTER TABLE returns VALIDATE CONSTRAINT returns_order_total_fkey;", + "type": "table.constraint", + "operation": "create", + "path": "public.returns.returns_order_total_fkey" + } + ] + }, { "steps": [ { @@ -236,6 +284,12 @@ "operation": "create", "path": "privileges.VIEW.order_totals.app_reader" }, + { + "sql": "GRANT SELECT (id) ON TABLE big_orders TO app_reader;", + "type": "column_privilege", + "operation": "create", + "path": "column_privileges.TABLE.big_orders.id.app_reader" + }, { "sql": "GRANT SELECT (id, total) ON TABLE orders TO app_reader;", "type": "column_privilege", diff --git a/testdata/diff/create_table/issue_591_recreate_generated_dependents/plan.sql b/testdata/diff/create_table/issue_591_recreate_generated_dependents/plan.sql index 9dbc3dc2..12493f36 100644 --- a/testdata/diff/create_table/issue_591_recreate_generated_dependents/plan.sql +++ b/testdata/diff/create_table/issue_591_recreate_generated_dependents/plan.sql @@ -67,11 +67,30 @@ DROP INDEX IF EXISTS orders_lookup_idx; ALTER INDEX orders_lookup_idx_pgschema_new RENAME TO orders_lookup_idx; +CREATE UNIQUE INDEX CONCURRENTLY IF NOT EXISTS orders_total_key ON orders (total); + +-- pgschema:wait +SELECT + COALESCE(i.indisvalid, false) as done, + CASE + WHEN p.blocks_total > 0 THEN p.blocks_done * 100 / p.blocks_total + ELSE 0 + END as progress +FROM pg_class c +LEFT JOIN pg_index i ON c.oid = i.indexrelid +LEFT JOIN pg_stat_progress_create_index p ON c.oid = p.index_relid +WHERE c.relname = 'orders_total_key'; + ALTER TABLE returns ADD CONSTRAINT returns_order_code_fkey FOREIGN KEY (order_code) REFERENCES orders (code) NOT VALID; ALTER TABLE returns VALIDATE CONSTRAINT returns_order_code_fkey; +ALTER TABLE returns +ADD CONSTRAINT returns_order_total_fkey FOREIGN KEY (order_total) REFERENCES orders (total) NOT VALID; + +ALTER TABLE returns VALIDATE CONSTRAINT returns_order_total_fkey; + ALTER TABLE shipments ADD CONSTRAINT shipments_order_code_fkey FOREIGN KEY (order_code) REFERENCES orders (code) NOT VALID; @@ -94,4 +113,6 @@ CREATE OR REPLACE VIEW big_orders AS GRANT SELECT ON TABLE order_totals TO app_reader; +GRANT SELECT (id) ON TABLE big_orders TO app_reader; + GRANT SELECT (id, total) ON TABLE orders TO app_reader; diff --git a/testdata/diff/create_table/issue_591_recreate_generated_dependents/plan.txt b/testdata/diff/create_table/issue_591_recreate_generated_dependents/plan.txt index 57f2162c..9c0013ef 100644 --- a/testdata/diff/create_table/issue_591_recreate_generated_dependents/plan.txt +++ b/testdata/diff/create_table/issue_591_recreate_generated_dependents/plan.txt @@ -1,10 +1,10 @@ -Plan: 2 to add, 6 to modify. +Plan: 3 to add, 6 to modify. Summary by type: tables: 3 to modify views: 3 to modify privileges: 1 to add - column privileges: 1 to add + column privileges: 2 to add Tables: ~ orders @@ -16,12 +16,14 @@ Tables: + orders_total_excl (constraint) ~ orders_code_key (index - concurrent rebuild) ~ orders_lookup_idx (index - concurrent rebuild) + + orders_total_key (index) - orders_big (policy) + orders_big (policy) - orders_total_trg (trigger) + orders_total_trg (trigger) ~ returns + returns_order_code_fkey (constraint) + + returns_order_total_fkey (constraint) ~ shipments - shipments_order_code_fkey (constraint) + shipments_order_code_fkey (constraint) @@ -36,6 +38,7 @@ Privileges: Column privileges: + app_reader + + app_reader DDL to be executed: -------------------------------------------------- @@ -116,20 +119,44 @@ DROP INDEX IF EXISTS orders_lookup_idx; ALTER INDEX orders_lookup_idx_pgschema_new RENAME TO orders_lookup_idx; +-- Transaction Group #8 +CREATE UNIQUE INDEX CONCURRENTLY IF NOT EXISTS orders_total_key ON orders (total); + +-- Transaction Group #9 +-- pgschema:wait +SELECT + COALESCE(i.indisvalid, false) as done, + CASE + WHEN p.blocks_total > 0 THEN p.blocks_done * 100 / p.blocks_total + ELSE 0 + END as progress +FROM pg_class c +LEFT JOIN pg_index i ON c.oid = i.indexrelid +LEFT JOIN pg_stat_progress_create_index p ON c.oid = p.index_relid +WHERE c.relname = 'orders_total_key'; + +-- Transaction Group #10 ALTER TABLE returns ADD CONSTRAINT returns_order_code_fkey FOREIGN KEY (order_code) REFERENCES orders (code) NOT VALID; --- Transaction Group #8 +-- Transaction Group #11 ALTER TABLE returns VALIDATE CONSTRAINT returns_order_code_fkey; --- Transaction Group #9 +-- Transaction Group #12 +ALTER TABLE returns +ADD CONSTRAINT returns_order_total_fkey FOREIGN KEY (order_total) REFERENCES orders (total) NOT VALID; + +-- Transaction Group #13 +ALTER TABLE returns VALIDATE CONSTRAINT returns_order_total_fkey; + +-- Transaction Group #14 ALTER TABLE shipments ADD CONSTRAINT shipments_order_code_fkey FOREIGN KEY (order_code) REFERENCES orders (code) NOT VALID; --- Transaction Group #10 +-- Transaction Group #15 ALTER TABLE shipments VALIDATE CONSTRAINT shipments_order_code_fkey; --- Transaction Group #11 +-- Transaction Group #16 CREATE OR REPLACE VIEW order_labels AS SELECT id, 'x'::text AS label @@ -147,4 +174,6 @@ CREATE OR REPLACE VIEW big_orders AS GRANT SELECT ON TABLE order_totals TO app_reader; +GRANT SELECT (id) ON TABLE big_orders TO app_reader; + GRANT SELECT (id, total) ON TABLE orders TO app_reader; From eca9deca03a95480d28227962410d00d377e2b18 Mon Sep 17 00:00:00 2001 From: tianzhou Date: Thu, 10 Sep 2026 00:07:27 -0700 Subject: [PATCH 09/11] fix: match names that need quoting only in their quoted, case-sensitive form (#591) Deparsers render a name that needs quoting (mixed case, special characters, reserved word) only double-quoted, and quoted identifiers are case-sensitive. identifierSpellings now emits just an exact quoted branch for such names and keeps the case-insensitive bare branch for the rest, so a column named "select" no longer matches every SELECT keyword and foo no longer matches the distinct column "Foo". Such false positives only caused redundant drop and re-create operations, but on views and indexes those are not free. Co-Authored-By: Claude Fable 5.1 --- internal/diff/identifier_match.go | 18 +++++++++++++----- internal/diff/identifier_match_test.go | 12 ++++++++++-- 2 files changed, 23 insertions(+), 7 deletions(-) diff --git a/internal/diff/identifier_match.go b/internal/diff/identifier_match.go index 99d40870..8d19b9b5 100644 --- a/internal/diff/identifier_match.go +++ b/internal/diff/identifier_match.go @@ -4,6 +4,8 @@ import ( "regexp" "strings" "sync" + + "github.com/pgplex/pgschema/ir" ) // Textual dependency checks (does this view read that table, does this @@ -35,12 +37,18 @@ var sqlStringLiteralRegex = regexp.MustCompile(`'(?:[^']|'')*'`) // dependency checks run the same names over many definitions. var identifierRegexpCache sync.Map // map[string]*regexp.Regexp -// identifierSpellings renders the two ways a deparser can spell name: bare, or -// double-quoted with embedded quotes doubled. +// identifierSpellings renders the ways a deparser can spell name. A name that +// needs quoting (mixed case, special characters, reserved word) is only ever +// rendered double-quoted, with embedded quotes doubled, and quoted identifiers +// are case-sensitive, so it gets an exact quoted branch alone: a column named +// "select" must not match the SELECT keyword, nor foo the column "Foo". Any +// other name may appear bare (matched case-insensitively) or quoted. func identifierSpellings(name string) string { - bare := regexp.QuoteMeta(name) quoted := regexp.QuoteMeta(`"` + strings.ReplaceAll(name, `"`, `""`) + `"`) - return `(?:` + bare + `|` + quoted + `)` + if ir.NeedsQuoting(name) { + return quoted + } + return `(?:(?i:` + regexp.QuoteMeta(name) + `)|` + quoted + `)` } // identifierRegexp returns a pattern matching name as a whole identifier in @@ -74,7 +82,7 @@ func identifierRegexp(mode identifierMatchMode, parts ...string) *regexp.Regexp before = before[:len(before)-1] + `:]` after = after[:len(after)-1] + `(]` } - re := regexp.MustCompile(`(?i)(?:^|` + before + `)` + body + `(?:` + after + `|$)`) + re := regexp.MustCompile(`(?:^|` + before + `)` + body + `(?:` + after + `|$)`) identifierRegexpCache.Store(key, re) return re } diff --git a/internal/diff/identifier_match_test.go b/internal/diff/identifier_match_test.go index 40c5d40a..637e901b 100644 --- a/internal/diff/identifier_match_test.go +++ b/internal/diff/identifier_match_test.go @@ -24,7 +24,13 @@ func TestContainsIdentifier(t *testing.T) { {"SELECT a.b FROM t", "a.b", true}, {"SELECT id FROM foo_bar", "foo", false}, {"SELECT id FROM foo$bar", "foo", false}, - {"SELECT 'users' FROM t", "users", false}, // string literal + {"SELECT 'users' FROM t", "users", false}, // string literal + {`SELECT "select" FROM t`, "select", true}, // reserved word is only ever quoted + {"SELECT a FROM t", "select", false}, // ... so the keyword is not a match + {`SELECT "Foo" FROM t`, "foo", false}, // quoted identifiers are case-sensitive + {`SELECT "Foo" FROM t`, "Foo", true}, + {"SELECT foo FROM t", "Foo", false}, + {"SELECT FOO FROM t", "foo", true}, // bare spelling stays case-insensitive {"SELECT (row)::users FROM t", "users", true}, {"", "users", false}, } @@ -68,7 +74,9 @@ func TestExprReferencesAnyColumn(t *testing.T) { {`("a"b" + 1)`, false}, {"(b$x + 1)", false}, // $ is part of the identifier {"(x$b + 1)", false}, - {"((a)::b)", false}, // type cast, not a column + {"((a)::b)", false}, // type cast, not a column + {"(B + 1)", true}, // bare spelling is case-insensitive + {`("B" + 1)`, false}, // a quoted "B" is a different column {"", false}, } for _, c := range cases { From eed96f8e0d339ac83c6d45a2530602c5bfa423c7 Mon Sep 17 00:00:00 2001 From: tianzhou Date: Thu, 10 Sep 2026 00:31:19 -0700 Subject: [PATCH 10/11] docs: state what an unknown target version means for diff vs rewrites (#591) Co-Authored-By: Claude Fable 5.1 --- cmd/plan/plan.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/cmd/plan/plan.go b/cmd/plan/plan.go index f234353b..0c66970e 100644 --- a/cmd/plan/plan.go +++ b/cmd/plan/plan.go @@ -396,8 +396,10 @@ func GeneratePlan(config *PlanConfig, provider postgres.DesiredStateProvider) (* } // Extract the target database's major version (e.g. "PostgreSQL 18.1" -> 18) - // to gate version-specific DDL and rewrites. Zero (unknown) falls back to - // the version-portable patterns. + // to gate version-specific DDL and rewrites. Zero (unknown) is handled + // differently by the two consumers: the diff assumes a current server and + // may emit DDL that older servers reject (e.g. SET EXPRESSION AS, PG17+), + // while the plan rewrites fall back to their version-portable patterns. targetMajorVersion := 0 if v, ok := strings.CutPrefix(currentStateIR.Metadata.DatabaseVersion, "PostgreSQL "); ok { fmt.Sscanf(v, "%d", &targetMajorVersion) From cd59747e671203453eaf59921339fb701d3e0041 Mon Sep 17 00:00:00 2001 From: tianzhou Date: Thu, 10 Sep 2026 00:35:38 -0700 Subject: [PATCH 11/11] test: fold the issue 591 cases into one apply cycle The STORED, VIRTUAL, and dependent-object scenarios now live in create_table/issue_591_alter_generated_column, grouped by scenario with comments; one embedded-postgres cycle instead of three. Co-Authored-By: Claude Fable 5.1 --- .../issue_591_alter_generated_column/diff.sql | 83 +++++ .../issue_591_alter_generated_column/new.sql | 72 ++++ .../issue_591_alter_generated_column/old.sql | 68 ++++ .../plan.json | 324 +++++++++++++++++- .../issue_591_alter_generated_column/plan.sql | 129 +++++++ .../issue_591_alter_generated_column/plan.txt | 192 ++++++++++- .../diff.sql | 9 - .../issue_591_alter_generated_virtual/new.sql | 6 - .../issue_591_alter_generated_virtual/old.sql | 6 - .../plan.json | 44 --- .../plan.sql | 9 - .../plan.txt | 25 -- .../diff.sql | 72 ---- .../new.sql | 59 ---- .../old.sql | 55 --- .../plan.json | 302 ---------------- .../plan.sql | 118 ------- .../plan.txt | 179 ---------- 18 files changed, 864 insertions(+), 888 deletions(-) delete mode 100644 testdata/diff/create_table/issue_591_alter_generated_virtual/diff.sql delete mode 100644 testdata/diff/create_table/issue_591_alter_generated_virtual/new.sql delete mode 100644 testdata/diff/create_table/issue_591_alter_generated_virtual/old.sql delete mode 100644 testdata/diff/create_table/issue_591_alter_generated_virtual/plan.json delete mode 100644 testdata/diff/create_table/issue_591_alter_generated_virtual/plan.sql delete mode 100644 testdata/diff/create_table/issue_591_alter_generated_virtual/plan.txt delete mode 100644 testdata/diff/create_table/issue_591_recreate_generated_dependents/diff.sql delete mode 100644 testdata/diff/create_table/issue_591_recreate_generated_dependents/new.sql delete mode 100644 testdata/diff/create_table/issue_591_recreate_generated_dependents/old.sql delete mode 100644 testdata/diff/create_table/issue_591_recreate_generated_dependents/plan.json delete mode 100644 testdata/diff/create_table/issue_591_recreate_generated_dependents/plan.sql delete mode 100644 testdata/diff/create_table/issue_591_recreate_generated_dependents/plan.txt diff --git a/testdata/diff/create_table/issue_591_alter_generated_column/diff.sql b/testdata/diff/create_table/issue_591_alter_generated_column/diff.sql index 4489a5d3..ef296505 100644 --- a/testdata/diff/create_table/issue_591_alter_generated_column/diff.sql +++ b/testdata/diff/create_table/issue_591_alter_generated_column/diff.sql @@ -1,5 +1,15 @@ +DROP VIEW IF EXISTS big_orders RESTRICT; + +DROP VIEW IF EXISTS order_totals RESTRICT; + +DROP VIEW IF EXISTS order_labels RESTRICT; + +DROP TRIGGER IF EXISTS orders_total_trg ON orders; + ALTER TABLE metric_refs DROP CONSTRAINT metric_refs_tripled_fkey; +ALTER TABLE shipments DROP CONSTRAINT shipments_order_code_fkey; + ALTER TABLE metrics DROP COLUMN tripled; ALTER TABLE metrics @@ -18,5 +28,78 @@ DROP INDEX IF EXISTS metrics_tripled_idx; CREATE INDEX IF NOT EXISTS metrics_tripled_idx ON metrics ((tripled + 1)) WHERE (tripled > 10); +DROP POLICY IF EXISTS orders_big ON orders; + +ALTER TABLE orders DROP CONSTRAINT orders_total_excl; + +ALTER TABLE orders DROP COLUMN total; + +ALTER TABLE orders DROP COLUMN code; + +ALTER TABLE orders ADD COLUMN total integer GENERATED ALWAYS AS ((qty * price)) STORED; + +ALTER TABLE orders ADD COLUMN code text GENERATED ALWAYS AS (('ORD-'::text || (id)::text)) STORED; + +ALTER TABLE orders +ADD CONSTRAINT orders_total_excl EXCLUDE USING btree ((total + 0) WITH =); + +CREATE OR REPLACE TRIGGER orders_total_trg + AFTER UPDATE ON orders + FOR EACH ROW + WHEN (((NEW.total > 0))) + EXECUTE FUNCTION orders_audit(); + +CREATE POLICY orders_big ON orders TO PUBLIC USING (total > 100); + +DROP INDEX IF EXISTS orders_code_key; + +CREATE UNIQUE INDEX IF NOT EXISTS orders_code_key ON orders (code); + +DROP INDEX IF EXISTS orders_lookup_idx; + +CREATE INDEX IF NOT EXISTS orders_lookup_idx ON orders (total); + +CREATE UNIQUE INDEX IF NOT EXISTS orders_total_key ON orders (total); + +ALTER TABLE vt DROP COLUMN v2; + +ALTER TABLE vt DROP COLUMN s1; + +ALTER TABLE vt ADD COLUMN v2 integer; + +ALTER TABLE vt ADD COLUMN s1 integer GENERATED ALWAYS AS ((a * 2)) VIRTUAL; + +ALTER TABLE vt ALTER COLUMN v1 SET EXPRESSION AS ((a * 2)); + ALTER TABLE metric_refs ADD CONSTRAINT metric_refs_tripled_fkey FOREIGN KEY (tripled) REFERENCES metrics (tripled); + +ALTER TABLE returns +ADD CONSTRAINT returns_order_code_fkey FOREIGN KEY (order_code) REFERENCES orders (code); + +ALTER TABLE returns +ADD CONSTRAINT returns_order_total_fkey FOREIGN KEY (order_total) REFERENCES orders (total); + +ALTER TABLE shipments +ADD CONSTRAINT shipments_order_code_fkey FOREIGN KEY (order_code) REFERENCES orders (code); + +CREATE OR REPLACE VIEW order_labels AS + SELECT id, + 'x'::text AS label + FROM orders; + +CREATE OR REPLACE VIEW order_totals AS + SELECT id, + total + FROM orders; + +CREATE OR REPLACE VIEW big_orders AS + SELECT id + FROM order_totals + WHERE total > 100; + +GRANT SELECT ON TABLE order_totals TO app_reader; + +GRANT SELECT (id) ON TABLE big_orders TO app_reader; + +GRANT SELECT (id, total) ON TABLE orders TO app_reader; diff --git a/testdata/diff/create_table/issue_591_alter_generated_column/new.sql b/testdata/diff/create_table/issue_591_alter_generated_column/new.sql index b19da6cf..f112c873 100644 --- a/testdata/diff/create_table/issue_591_alter_generated_column/new.sql +++ b/testdata/diff/create_table/issue_591_alter_generated_column/new.sql @@ -1,3 +1,67 @@ +-- Re-created column with dependents: views, policy, trigger, grants, +-- expression EXCLUDE, standalone unique index bound by FKs, moved index +DO $$ +BEGIN + IF NOT EXISTS (SELECT 1 FROM pg_roles WHERE rolname = 'app_reader') THEN + CREATE ROLE app_reader; + END IF; +END $$; + +CREATE FUNCTION public.orders_audit() RETURNS trigger + LANGUAGE plpgsql + AS $$ BEGIN RETURN NEW; END $$; + +CREATE TABLE public.orders ( + id integer PRIMARY KEY, + qty integer NOT NULL, + price integer NOT NULL, + total integer GENERATED ALWAYS AS (qty * price) STORED, + code text GENERATED ALWAYS AS ('ORD-' || id::text) STORED, + CONSTRAINT orders_total_excl EXCLUDE USING btree ((total + 0) WITH =) +); + +CREATE UNIQUE INDEX orders_code_key ON public.orders (code); + +CREATE INDEX orders_lookup_idx ON public.orders (total); + +CREATE UNIQUE INDEX orders_total_key ON public.orders (total); + +CREATE TABLE public.shipments ( + id integer PRIMARY KEY, + order_code text, + CONSTRAINT shipments_order_code_fkey FOREIGN KEY (order_code) REFERENCES public.orders (code) +); + +CREATE TABLE public.returns ( + id integer PRIMARY KEY, + order_total integer, + order_code text, + CONSTRAINT returns_order_code_fkey FOREIGN KEY (order_code) REFERENCES public.orders (code), + CONSTRAINT returns_order_total_fkey FOREIGN KEY (order_total) REFERENCES public.orders (total) +); + +CREATE VIEW public.order_totals AS SELECT id, total FROM public.orders; + +CREATE VIEW public.big_orders AS SELECT id FROM public.order_totals WHERE total > 100; + +CREATE VIEW public.order_prices AS SELECT id, price FROM public.orders; + +CREATE VIEW public.order_labels AS SELECT id, 'x'::text AS label FROM public.orders; + +ALTER TABLE public.orders ENABLE ROW LEVEL SECURITY; + +CREATE POLICY orders_big ON public.orders USING (total > 100); + +CREATE TRIGGER orders_total_trg AFTER UPDATE ON public.orders FOR EACH ROW WHEN (NEW.total > 0) EXECUTE FUNCTION public.orders_audit(); + +GRANT SELECT (id, total) ON public.orders TO app_reader; + +GRANT SELECT ON public.order_totals TO app_reader; + +GRANT SELECT (id) ON public.big_orders TO app_reader; + +-- STORED expression change, STORED -> plain, plain -> STORED with a +-- check constraint, unique constraint bound by an FK, and an expression index CREATE TABLE public.metrics ( id integer PRIMARY KEY, a integer NOT NULL, @@ -16,3 +80,11 @@ CREATE TABLE public.metric_refs ( tripled integer, CONSTRAINT metric_refs_tripled_fkey FOREIGN KEY (tripled) REFERENCES public.metrics (tripled) ); + +-- VIRTUAL expression change, VIRTUAL -> plain, STORED -> VIRTUAL (PG18+) +CREATE TABLE public.vt ( + a integer NOT NULL, + v1 integer GENERATED ALWAYS AS (a * 2) VIRTUAL, + v2 integer, + s1 integer GENERATED ALWAYS AS (a * 2) VIRTUAL +); diff --git a/testdata/diff/create_table/issue_591_alter_generated_column/old.sql b/testdata/diff/create_table/issue_591_alter_generated_column/old.sql index 79a0c34d..6ae837af 100644 --- a/testdata/diff/create_table/issue_591_alter_generated_column/old.sql +++ b/testdata/diff/create_table/issue_591_alter_generated_column/old.sql @@ -1,3 +1,63 @@ +-- Re-created column with dependents: views, policy, trigger, grants, +-- expression EXCLUDE, standalone unique index bound by FKs, moved index +DO $$ +BEGIN + IF NOT EXISTS (SELECT 1 FROM pg_roles WHERE rolname = 'app_reader') THEN + CREATE ROLE app_reader; + END IF; +END $$; + +CREATE FUNCTION public.orders_audit() RETURNS trigger + LANGUAGE plpgsql + AS $$ BEGIN RETURN NEW; END $$; + +CREATE TABLE public.orders ( + id integer PRIMARY KEY, + qty integer NOT NULL, + price integer NOT NULL, + total integer, + code text, + CONSTRAINT orders_total_excl EXCLUDE USING btree ((total + 0) WITH =) +); + +CREATE UNIQUE INDEX orders_code_key ON public.orders (code); + +CREATE INDEX orders_lookup_idx ON public.orders (price); + +CREATE TABLE public.shipments ( + id integer PRIMARY KEY, + order_code text, + CONSTRAINT shipments_order_code_fkey FOREIGN KEY (order_code) REFERENCES public.orders (code) +); + +CREATE TABLE public.returns ( + id integer PRIMARY KEY, + order_total integer, + order_code text +); + +CREATE VIEW public.order_totals AS SELECT id, total FROM public.orders; + +CREATE VIEW public.big_orders AS SELECT id FROM public.order_totals WHERE total > 100; + +CREATE VIEW public.order_prices AS SELECT id, price FROM public.orders; + +CREATE VIEW public.order_labels AS SELECT id, code AS label FROM public.orders; + +ALTER TABLE public.orders ENABLE ROW LEVEL SECURITY; + +CREATE POLICY orders_big ON public.orders USING (total > 100); + +CREATE TRIGGER orders_total_trg AFTER UPDATE ON public.orders FOR EACH ROW WHEN (NEW.total > 0) EXECUTE FUNCTION public.orders_audit(); + +GRANT SELECT (id, total) ON public.orders TO app_reader; + +GRANT SELECT ON public.order_totals TO app_reader; + +GRANT SELECT (id) ON public.big_orders TO app_reader; + +-- STORED expression change, STORED -> plain, plain -> STORED with a +-- check constraint, unique constraint bound by an FK, and an expression index CREATE TABLE public.metrics ( id integer PRIMARY KEY, a integer NOT NULL, @@ -16,3 +76,11 @@ CREATE TABLE public.metric_refs ( tripled integer, CONSTRAINT metric_refs_tripled_fkey FOREIGN KEY (tripled) REFERENCES public.metrics (tripled) ); + +-- VIRTUAL expression change, VIRTUAL -> plain, STORED -> VIRTUAL (PG18+) +CREATE TABLE public.vt ( + a integer NOT NULL, + v1 integer GENERATED ALWAYS AS (a * 3) VIRTUAL, + v2 integer GENERATED ALWAYS AS (a * 2) VIRTUAL, + s1 integer GENERATED ALWAYS AS (a * 2) STORED +); diff --git a/testdata/diff/create_table/issue_591_alter_generated_column/plan.json b/testdata/diff/create_table/issue_591_alter_generated_column/plan.json index 9532bd31..82d44211 100644 --- a/testdata/diff/create_table/issue_591_alter_generated_column/plan.json +++ b/testdata/diff/create_table/issue_591_alter_generated_column/plan.json @@ -3,17 +3,47 @@ "pgschema_version": "1.13.0", "created_at": "1970-01-01T00:00:00Z", "source_fingerprint": { - "hash": "5de493c70ef78fca50074be3dd6d594c731b27ee3810f598460fdea5bb75c106" + "hash": "09ffd86ab854259a56fd900c98b031b50b0f4222351115dab779a7bddf3fe0ac" }, "groups": [ { "steps": [ + { + "sql": "DROP VIEW IF EXISTS big_orders RESTRICT;", + "type": "view", + "operation": "recreate", + "path": "public.big_orders" + }, + { + "sql": "DROP VIEW IF EXISTS order_totals RESTRICT;", + "type": "view", + "operation": "recreate", + "path": "public.order_totals" + }, + { + "sql": "DROP VIEW IF EXISTS order_labels RESTRICT;", + "type": "view", + "operation": "recreate", + "path": "public.order_labels" + }, + { + "sql": "DROP TRIGGER IF EXISTS orders_total_trg ON orders;", + "type": "table.trigger", + "operation": "drop", + "path": "public.orders.orders_total_trg" + }, { "sql": "ALTER TABLE metric_refs DROP CONSTRAINT metric_refs_tripled_fkey;", "type": "table.constraint", "operation": "drop", "path": "public.metric_refs.metric_refs_tripled_fkey" }, + { + "sql": "ALTER TABLE shipments DROP CONSTRAINT shipments_order_code_fkey;", + "type": "table.constraint", + "operation": "drop", + "path": "public.shipments.shipments_order_code_fkey" + }, { "sql": "ALTER TABLE metrics DROP COLUMN tripled;", "type": "table.column", @@ -100,6 +130,198 @@ "operation": "alter", "path": "public.metrics.metrics_tripled_idx" }, + { + "sql": "DROP POLICY IF EXISTS orders_big ON orders;", + "type": "table.policy", + "operation": "drop", + "path": "public.orders.orders_big" + }, + { + "sql": "ALTER TABLE orders DROP CONSTRAINT orders_total_excl;", + "type": "table.constraint", + "operation": "drop", + "path": "public.orders.orders_total_excl" + }, + { + "sql": "ALTER TABLE orders DROP COLUMN total;", + "type": "table.column", + "operation": "drop", + "path": "public.orders.total" + }, + { + "sql": "ALTER TABLE orders DROP COLUMN code;", + "type": "table.column", + "operation": "drop", + "path": "public.orders.code" + }, + { + "sql": "ALTER TABLE orders ADD COLUMN total integer GENERATED ALWAYS AS ((qty * price)) STORED;", + "type": "table.column", + "operation": "create", + "path": "public.orders.total" + }, + { + "sql": "ALTER TABLE orders ADD COLUMN code text GENERATED ALWAYS AS (('ORD-'::text || (id)::text)) STORED;", + "type": "table.column", + "operation": "create", + "path": "public.orders.code" + }, + { + "sql": "ALTER TABLE orders\nADD CONSTRAINT orders_total_excl EXCLUDE USING btree ((total + 0) WITH =);", + "type": "table.constraint", + "operation": "create", + "path": "public.orders.orders_total_excl" + }, + { + "sql": "CREATE OR REPLACE TRIGGER orders_total_trg\n AFTER UPDATE ON orders\n FOR EACH ROW\n WHEN (((NEW.total > 0)))\n EXECUTE FUNCTION orders_audit();", + "type": "table.trigger", + "operation": "create", + "path": "public.orders.orders_total_trg" + }, + { + "sql": "CREATE POLICY orders_big ON orders TO PUBLIC USING (total > 100);", + "type": "table.policy", + "operation": "create", + "path": "public.orders.orders_big" + } + ] + }, + { + "steps": [ + { + "sql": "CREATE UNIQUE INDEX CONCURRENTLY IF NOT EXISTS orders_code_key_pgschema_new ON orders (code);", + "type": "table.index", + "operation": "alter", + "path": "public.orders.orders_code_key" + } + ] + }, + { + "steps": [ + { + "sql": "SELECT \n COALESCE(i.indisvalid, false) as done,\n CASE \n WHEN p.blocks_total > 0 THEN p.blocks_done * 100 / p.blocks_total\n ELSE 0\n END as progress\nFROM pg_class c\nLEFT JOIN pg_index i ON c.oid = i.indexrelid\nLEFT JOIN pg_stat_progress_create_index p ON c.oid = p.index_relid\nWHERE c.relname = 'orders_code_key_pgschema_new';", + "directive": { + "type": "wait", + "message": "Creating index orders_code_key_pgschema_new" + }, + "type": "table.index", + "operation": "alter", + "path": "public.orders.orders_code_key" + } + ] + }, + { + "steps": [ + { + "sql": "DROP INDEX IF EXISTS orders_code_key;", + "type": "table.index", + "operation": "alter", + "path": "public.orders.orders_code_key" + }, + { + "sql": "ALTER INDEX orders_code_key_pgschema_new RENAME TO orders_code_key;", + "type": "table.index", + "operation": "alter", + "path": "public.orders.orders_code_key" + } + ] + }, + { + "steps": [ + { + "sql": "CREATE INDEX CONCURRENTLY IF NOT EXISTS orders_lookup_idx_pgschema_new ON orders (total);", + "type": "table.index", + "operation": "alter", + "path": "public.orders.orders_lookup_idx" + } + ] + }, + { + "steps": [ + { + "sql": "SELECT \n COALESCE(i.indisvalid, false) as done,\n CASE \n WHEN p.blocks_total > 0 THEN p.blocks_done * 100 / p.blocks_total\n ELSE 0\n END as progress\nFROM pg_class c\nLEFT JOIN pg_index i ON c.oid = i.indexrelid\nLEFT JOIN pg_stat_progress_create_index p ON c.oid = p.index_relid\nWHERE c.relname = 'orders_lookup_idx_pgschema_new';", + "directive": { + "type": "wait", + "message": "Creating index orders_lookup_idx_pgschema_new" + }, + "type": "table.index", + "operation": "alter", + "path": "public.orders.orders_lookup_idx" + } + ] + }, + { + "steps": [ + { + "sql": "DROP INDEX IF EXISTS orders_lookup_idx;", + "type": "table.index", + "operation": "alter", + "path": "public.orders.orders_lookup_idx" + }, + { + "sql": "ALTER INDEX orders_lookup_idx_pgschema_new RENAME TO orders_lookup_idx;", + "type": "table.index", + "operation": "alter", + "path": "public.orders.orders_lookup_idx" + } + ] + }, + { + "steps": [ + { + "sql": "CREATE UNIQUE INDEX CONCURRENTLY IF NOT EXISTS orders_total_key ON orders (total);", + "type": "table.index", + "operation": "create", + "path": "public.orders.orders_total_key" + } + ] + }, + { + "steps": [ + { + "sql": "SELECT \n COALESCE(i.indisvalid, false) as done,\n CASE \n WHEN p.blocks_total > 0 THEN p.blocks_done * 100 / p.blocks_total\n ELSE 0\n END as progress\nFROM pg_class c\nLEFT JOIN pg_index i ON c.oid = i.indexrelid\nLEFT JOIN pg_stat_progress_create_index p ON c.oid = p.index_relid\nWHERE c.relname = 'orders_total_key';", + "directive": { + "type": "wait", + "message": "Creating index orders_total_key" + }, + "type": "table.index", + "operation": "create", + "path": "public.orders.orders_total_key" + } + ] + }, + { + "steps": [ + { + "sql": "ALTER TABLE vt DROP COLUMN v2;", + "type": "table.column", + "operation": "drop", + "path": "public.vt.v2" + }, + { + "sql": "ALTER TABLE vt DROP COLUMN s1;", + "type": "table.column", + "operation": "drop", + "path": "public.vt.s1" + }, + { + "sql": "ALTER TABLE vt ADD COLUMN v2 integer;", + "type": "table.column", + "operation": "create", + "path": "public.vt.v2" + }, + { + "sql": "ALTER TABLE vt ADD COLUMN s1 integer GENERATED ALWAYS AS ((a * 2)) VIRTUAL;", + "type": "table.column", + "operation": "create", + "path": "public.vt.s1" + }, + { + "sql": "ALTER TABLE vt ALTER COLUMN v1 SET EXPRESSION AS ((a * 2));", + "type": "table.column", + "operation": "alter", + "path": "public.vt.v1" + }, { "sql": "ALTER TABLE metric_refs\nADD CONSTRAINT metric_refs_tripled_fkey FOREIGN KEY (tripled) REFERENCES metrics (tripled) NOT VALID;", "type": "table.constraint", @@ -117,6 +339,106 @@ "path": "public.metric_refs.metric_refs_tripled_fkey" } ] + }, + { + "steps": [ + { + "sql": "ALTER TABLE returns\nADD CONSTRAINT returns_order_code_fkey FOREIGN KEY (order_code) REFERENCES orders (code) NOT VALID;", + "type": "table.constraint", + "operation": "create", + "path": "public.returns.returns_order_code_fkey" + } + ] + }, + { + "steps": [ + { + "sql": "ALTER TABLE returns VALIDATE CONSTRAINT returns_order_code_fkey;", + "type": "table.constraint", + "operation": "create", + "path": "public.returns.returns_order_code_fkey" + } + ] + }, + { + "steps": [ + { + "sql": "ALTER TABLE returns\nADD CONSTRAINT returns_order_total_fkey FOREIGN KEY (order_total) REFERENCES orders (total) NOT VALID;", + "type": "table.constraint", + "operation": "create", + "path": "public.returns.returns_order_total_fkey" + } + ] + }, + { + "steps": [ + { + "sql": "ALTER TABLE returns VALIDATE CONSTRAINT returns_order_total_fkey;", + "type": "table.constraint", + "operation": "create", + "path": "public.returns.returns_order_total_fkey" + } + ] + }, + { + "steps": [ + { + "sql": "ALTER TABLE shipments\nADD CONSTRAINT shipments_order_code_fkey FOREIGN KEY (order_code) REFERENCES orders (code) NOT VALID;", + "type": "table.constraint", + "operation": "create", + "path": "public.shipments.shipments_order_code_fkey" + } + ] + }, + { + "steps": [ + { + "sql": "ALTER TABLE shipments VALIDATE CONSTRAINT shipments_order_code_fkey;", + "type": "table.constraint", + "operation": "create", + "path": "public.shipments.shipments_order_code_fkey" + } + ] + }, + { + "steps": [ + { + "sql": "CREATE OR REPLACE VIEW order_labels AS\n SELECT id,\n 'x'::text AS label\n FROM orders;", + "type": "view", + "operation": "create", + "path": "public.order_labels" + }, + { + "sql": "CREATE OR REPLACE VIEW order_totals AS\n SELECT id,\n total\n FROM orders;", + "type": "view", + "operation": "create", + "path": "public.order_totals" + }, + { + "sql": "CREATE OR REPLACE VIEW big_orders AS\n SELECT id\n FROM order_totals\n WHERE total > 100;", + "type": "view", + "operation": "recreate", + "path": "public.big_orders" + }, + { + "sql": "GRANT SELECT ON TABLE order_totals TO app_reader;", + "type": "privilege", + "operation": "create", + "path": "privileges.VIEW.order_totals.app_reader" + }, + { + "sql": "GRANT SELECT (id) ON TABLE big_orders TO app_reader;", + "type": "column_privilege", + "operation": "create", + "path": "column_privileges.TABLE.big_orders.id.app_reader" + }, + { + "sql": "GRANT SELECT (id, total) ON TABLE orders TO app_reader;", + "type": "column_privilege", + "operation": "create", + "path": "column_privileges.TABLE.orders.id,total.app_reader" + } + ] } ] } diff --git a/testdata/diff/create_table/issue_591_alter_generated_column/plan.sql b/testdata/diff/create_table/issue_591_alter_generated_column/plan.sql index 29089b23..11011322 100644 --- a/testdata/diff/create_table/issue_591_alter_generated_column/plan.sql +++ b/testdata/diff/create_table/issue_591_alter_generated_column/plan.sql @@ -1,5 +1,15 @@ +DROP VIEW IF EXISTS big_orders RESTRICT; + +DROP VIEW IF EXISTS order_totals RESTRICT; + +DROP VIEW IF EXISTS order_labels RESTRICT; + +DROP TRIGGER IF EXISTS orders_total_trg ON orders; + ALTER TABLE metric_refs DROP CONSTRAINT metric_refs_tripled_fkey; +ALTER TABLE shipments DROP CONSTRAINT shipments_order_code_fkey; + ALTER TABLE metrics DROP COLUMN tripled; ALTER TABLE metrics @@ -34,7 +44,126 @@ DROP INDEX IF EXISTS metrics_tripled_idx; ALTER INDEX metrics_tripled_idx_pgschema_new RENAME TO metrics_tripled_idx; +DROP POLICY IF EXISTS orders_big ON orders; + +ALTER TABLE orders DROP CONSTRAINT orders_total_excl; + +ALTER TABLE orders DROP COLUMN total; + +ALTER TABLE orders DROP COLUMN code; + +ALTER TABLE orders ADD COLUMN total integer GENERATED ALWAYS AS ((qty * price)) STORED; + +ALTER TABLE orders ADD COLUMN code text GENERATED ALWAYS AS (('ORD-'::text || (id)::text)) STORED; + +ALTER TABLE orders +ADD CONSTRAINT orders_total_excl EXCLUDE USING btree ((total + 0) WITH =); + +CREATE OR REPLACE TRIGGER orders_total_trg + AFTER UPDATE ON orders + FOR EACH ROW + WHEN (((NEW.total > 0))) + EXECUTE FUNCTION orders_audit(); + +CREATE POLICY orders_big ON orders TO PUBLIC USING (total > 100); + +CREATE UNIQUE INDEX CONCURRENTLY IF NOT EXISTS orders_code_key_pgschema_new ON orders (code); + +-- pgschema:wait +SELECT + COALESCE(i.indisvalid, false) as done, + CASE + WHEN p.blocks_total > 0 THEN p.blocks_done * 100 / p.blocks_total + ELSE 0 + END as progress +FROM pg_class c +LEFT JOIN pg_index i ON c.oid = i.indexrelid +LEFT JOIN pg_stat_progress_create_index p ON c.oid = p.index_relid +WHERE c.relname = 'orders_code_key_pgschema_new'; + +DROP INDEX IF EXISTS orders_code_key; + +ALTER INDEX orders_code_key_pgschema_new RENAME TO orders_code_key; + +CREATE INDEX CONCURRENTLY IF NOT EXISTS orders_lookup_idx_pgschema_new ON orders (total); + +-- pgschema:wait +SELECT + COALESCE(i.indisvalid, false) as done, + CASE + WHEN p.blocks_total > 0 THEN p.blocks_done * 100 / p.blocks_total + ELSE 0 + END as progress +FROM pg_class c +LEFT JOIN pg_index i ON c.oid = i.indexrelid +LEFT JOIN pg_stat_progress_create_index p ON c.oid = p.index_relid +WHERE c.relname = 'orders_lookup_idx_pgschema_new'; + +DROP INDEX IF EXISTS orders_lookup_idx; + +ALTER INDEX orders_lookup_idx_pgschema_new RENAME TO orders_lookup_idx; + +CREATE UNIQUE INDEX CONCURRENTLY IF NOT EXISTS orders_total_key ON orders (total); + +-- pgschema:wait +SELECT + COALESCE(i.indisvalid, false) as done, + CASE + WHEN p.blocks_total > 0 THEN p.blocks_done * 100 / p.blocks_total + ELSE 0 + END as progress +FROM pg_class c +LEFT JOIN pg_index i ON c.oid = i.indexrelid +LEFT JOIN pg_stat_progress_create_index p ON c.oid = p.index_relid +WHERE c.relname = 'orders_total_key'; + +ALTER TABLE vt DROP COLUMN v2; + +ALTER TABLE vt DROP COLUMN s1; + +ALTER TABLE vt ADD COLUMN v2 integer; + +ALTER TABLE vt ADD COLUMN s1 integer GENERATED ALWAYS AS ((a * 2)) VIRTUAL; + +ALTER TABLE vt ALTER COLUMN v1 SET EXPRESSION AS ((a * 2)); + ALTER TABLE metric_refs ADD CONSTRAINT metric_refs_tripled_fkey FOREIGN KEY (tripled) REFERENCES metrics (tripled) NOT VALID; ALTER TABLE metric_refs VALIDATE CONSTRAINT metric_refs_tripled_fkey; + +ALTER TABLE returns +ADD CONSTRAINT returns_order_code_fkey FOREIGN KEY (order_code) REFERENCES orders (code) NOT VALID; + +ALTER TABLE returns VALIDATE CONSTRAINT returns_order_code_fkey; + +ALTER TABLE returns +ADD CONSTRAINT returns_order_total_fkey FOREIGN KEY (order_total) REFERENCES orders (total) NOT VALID; + +ALTER TABLE returns VALIDATE CONSTRAINT returns_order_total_fkey; + +ALTER TABLE shipments +ADD CONSTRAINT shipments_order_code_fkey FOREIGN KEY (order_code) REFERENCES orders (code) NOT VALID; + +ALTER TABLE shipments VALIDATE CONSTRAINT shipments_order_code_fkey; + +CREATE OR REPLACE VIEW order_labels AS + SELECT id, + 'x'::text AS label + FROM orders; + +CREATE OR REPLACE VIEW order_totals AS + SELECT id, + total + FROM orders; + +CREATE OR REPLACE VIEW big_orders AS + SELECT id + FROM order_totals + WHERE total > 100; + +GRANT SELECT ON TABLE order_totals TO app_reader; + +GRANT SELECT (id) ON TABLE big_orders TO app_reader; + +GRANT SELECT (id, total) ON TABLE orders TO app_reader; diff --git a/testdata/diff/create_table/issue_591_alter_generated_column/plan.txt b/testdata/diff/create_table/issue_591_alter_generated_column/plan.txt index 00cdf329..7f34b683 100644 --- a/testdata/diff/create_table/issue_591_alter_generated_column/plan.txt +++ b/testdata/diff/create_table/issue_591_alter_generated_column/plan.txt @@ -1,7 +1,10 @@ -Plan: 2 to modify. +Plan: 3 to add, 9 to modify. Summary by type: - tables: 2 to modify + tables: 6 to modify + views: 3 to modify + privileges: 1 to add + column privileges: 2 to add Tables: ~ metric_refs @@ -14,13 +17,61 @@ Tables: + tripled (column) + metrics_tripled_check (constraint) ~ metrics_tripled_idx (index - concurrent rebuild) + ~ orders + - code (column) + + code (column) + - total (column) + + total (column) + - orders_total_excl (constraint) + + orders_total_excl (constraint) + ~ orders_code_key (index - concurrent rebuild) + ~ orders_lookup_idx (index - concurrent rebuild) + + orders_total_key (index) + - orders_big (policy) + + orders_big (policy) + - orders_total_trg (trigger) + + orders_total_trg (trigger) + ~ returns + + returns_order_code_fkey (constraint) + + returns_order_total_fkey (constraint) + ~ shipments + - shipments_order_code_fkey (constraint) + + shipments_order_code_fkey (constraint) + ~ vt + - s1 (column) + + s1 (column) + ~ v1 (column) + - v2 (column) + + v2 (column) + +Views: + ~ big_orders + ~ order_labels + ~ order_totals + +Privileges: + + app_reader + +Column privileges: + + app_reader + + app_reader DDL to be executed: -------------------------------------------------- -- Transaction Group #1 +DROP VIEW IF EXISTS big_orders RESTRICT; + +DROP VIEW IF EXISTS order_totals RESTRICT; + +DROP VIEW IF EXISTS order_labels RESTRICT; + +DROP TRIGGER IF EXISTS orders_total_trg ON orders; + ALTER TABLE metric_refs DROP CONSTRAINT metric_refs_tripled_fkey; +ALTER TABLE shipments DROP CONSTRAINT shipments_order_code_fkey; + ALTER TABLE metrics DROP COLUMN tripled; ALTER TABLE metrics @@ -59,8 +110,143 @@ DROP INDEX IF EXISTS metrics_tripled_idx; ALTER INDEX metrics_tripled_idx_pgschema_new RENAME TO metrics_tripled_idx; +DROP POLICY IF EXISTS orders_big ON orders; + +ALTER TABLE orders DROP CONSTRAINT orders_total_excl; + +ALTER TABLE orders DROP COLUMN total; + +ALTER TABLE orders DROP COLUMN code; + +ALTER TABLE orders ADD COLUMN total integer GENERATED ALWAYS AS ((qty * price)) STORED; + +ALTER TABLE orders ADD COLUMN code text GENERATED ALWAYS AS (('ORD-'::text || (id)::text)) STORED; + +ALTER TABLE orders +ADD CONSTRAINT orders_total_excl EXCLUDE USING btree ((total + 0) WITH =); + +CREATE OR REPLACE TRIGGER orders_total_trg + AFTER UPDATE ON orders + FOR EACH ROW + WHEN (((NEW.total > 0))) + EXECUTE FUNCTION orders_audit(); + +CREATE POLICY orders_big ON orders TO PUBLIC USING (total > 100); + +-- Transaction Group #6 +CREATE UNIQUE INDEX CONCURRENTLY IF NOT EXISTS orders_code_key_pgschema_new ON orders (code); + +-- Transaction Group #7 +-- pgschema:wait +SELECT + COALESCE(i.indisvalid, false) as done, + CASE + WHEN p.blocks_total > 0 THEN p.blocks_done * 100 / p.blocks_total + ELSE 0 + END as progress +FROM pg_class c +LEFT JOIN pg_index i ON c.oid = i.indexrelid +LEFT JOIN pg_stat_progress_create_index p ON c.oid = p.index_relid +WHERE c.relname = 'orders_code_key_pgschema_new'; + +-- Transaction Group #8 +DROP INDEX IF EXISTS orders_code_key; + +ALTER INDEX orders_code_key_pgschema_new RENAME TO orders_code_key; + +-- Transaction Group #9 +CREATE INDEX CONCURRENTLY IF NOT EXISTS orders_lookup_idx_pgschema_new ON orders (total); + +-- Transaction Group #10 +-- pgschema:wait +SELECT + COALESCE(i.indisvalid, false) as done, + CASE + WHEN p.blocks_total > 0 THEN p.blocks_done * 100 / p.blocks_total + ELSE 0 + END as progress +FROM pg_class c +LEFT JOIN pg_index i ON c.oid = i.indexrelid +LEFT JOIN pg_stat_progress_create_index p ON c.oid = p.index_relid +WHERE c.relname = 'orders_lookup_idx_pgschema_new'; + +-- Transaction Group #11 +DROP INDEX IF EXISTS orders_lookup_idx; + +ALTER INDEX orders_lookup_idx_pgschema_new RENAME TO orders_lookup_idx; + +-- Transaction Group #12 +CREATE UNIQUE INDEX CONCURRENTLY IF NOT EXISTS orders_total_key ON orders (total); + +-- Transaction Group #13 +-- pgschema:wait +SELECT + COALESCE(i.indisvalid, false) as done, + CASE + WHEN p.blocks_total > 0 THEN p.blocks_done * 100 / p.blocks_total + ELSE 0 + END as progress +FROM pg_class c +LEFT JOIN pg_index i ON c.oid = i.indexrelid +LEFT JOIN pg_stat_progress_create_index p ON c.oid = p.index_relid +WHERE c.relname = 'orders_total_key'; + +-- Transaction Group #14 +ALTER TABLE vt DROP COLUMN v2; + +ALTER TABLE vt DROP COLUMN s1; + +ALTER TABLE vt ADD COLUMN v2 integer; + +ALTER TABLE vt ADD COLUMN s1 integer GENERATED ALWAYS AS ((a * 2)) VIRTUAL; + +ALTER TABLE vt ALTER COLUMN v1 SET EXPRESSION AS ((a * 2)); + ALTER TABLE metric_refs ADD CONSTRAINT metric_refs_tripled_fkey FOREIGN KEY (tripled) REFERENCES metrics (tripled) NOT VALID; --- Transaction Group #6 +-- Transaction Group #15 ALTER TABLE metric_refs VALIDATE CONSTRAINT metric_refs_tripled_fkey; + +-- Transaction Group #16 +ALTER TABLE returns +ADD CONSTRAINT returns_order_code_fkey FOREIGN KEY (order_code) REFERENCES orders (code) NOT VALID; + +-- Transaction Group #17 +ALTER TABLE returns VALIDATE CONSTRAINT returns_order_code_fkey; + +-- Transaction Group #18 +ALTER TABLE returns +ADD CONSTRAINT returns_order_total_fkey FOREIGN KEY (order_total) REFERENCES orders (total) NOT VALID; + +-- Transaction Group #19 +ALTER TABLE returns VALIDATE CONSTRAINT returns_order_total_fkey; + +-- Transaction Group #20 +ALTER TABLE shipments +ADD CONSTRAINT shipments_order_code_fkey FOREIGN KEY (order_code) REFERENCES orders (code) NOT VALID; + +-- Transaction Group #21 +ALTER TABLE shipments VALIDATE CONSTRAINT shipments_order_code_fkey; + +-- Transaction Group #22 +CREATE OR REPLACE VIEW order_labels AS + SELECT id, + 'x'::text AS label + FROM orders; + +CREATE OR REPLACE VIEW order_totals AS + SELECT id, + total + FROM orders; + +CREATE OR REPLACE VIEW big_orders AS + SELECT id + FROM order_totals + WHERE total > 100; + +GRANT SELECT ON TABLE order_totals TO app_reader; + +GRANT SELECT (id) ON TABLE big_orders TO app_reader; + +GRANT SELECT (id, total) ON TABLE orders TO app_reader; diff --git a/testdata/diff/create_table/issue_591_alter_generated_virtual/diff.sql b/testdata/diff/create_table/issue_591_alter_generated_virtual/diff.sql deleted file mode 100644 index 9fd07a75..00000000 --- a/testdata/diff/create_table/issue_591_alter_generated_virtual/diff.sql +++ /dev/null @@ -1,9 +0,0 @@ -ALTER TABLE vt DROP COLUMN v2; - -ALTER TABLE vt DROP COLUMN s1; - -ALTER TABLE vt ADD COLUMN v2 integer; - -ALTER TABLE vt ADD COLUMN s1 integer GENERATED ALWAYS AS ((a * 2)) VIRTUAL; - -ALTER TABLE vt ALTER COLUMN v1 SET EXPRESSION AS ((a * 2)); diff --git a/testdata/diff/create_table/issue_591_alter_generated_virtual/new.sql b/testdata/diff/create_table/issue_591_alter_generated_virtual/new.sql deleted file mode 100644 index 2859e390..00000000 --- a/testdata/diff/create_table/issue_591_alter_generated_virtual/new.sql +++ /dev/null @@ -1,6 +0,0 @@ -CREATE TABLE public.vt ( - a integer NOT NULL, - v1 integer GENERATED ALWAYS AS (a * 2) VIRTUAL, - v2 integer, - s1 integer GENERATED ALWAYS AS (a * 2) VIRTUAL -); diff --git a/testdata/diff/create_table/issue_591_alter_generated_virtual/old.sql b/testdata/diff/create_table/issue_591_alter_generated_virtual/old.sql deleted file mode 100644 index 97615873..00000000 --- a/testdata/diff/create_table/issue_591_alter_generated_virtual/old.sql +++ /dev/null @@ -1,6 +0,0 @@ -CREATE TABLE public.vt ( - a integer NOT NULL, - v1 integer GENERATED ALWAYS AS (a * 3) VIRTUAL, - v2 integer GENERATED ALWAYS AS (a * 2) VIRTUAL, - s1 integer GENERATED ALWAYS AS (a * 2) STORED -); diff --git a/testdata/diff/create_table/issue_591_alter_generated_virtual/plan.json b/testdata/diff/create_table/issue_591_alter_generated_virtual/plan.json deleted file mode 100644 index 45736538..00000000 --- a/testdata/diff/create_table/issue_591_alter_generated_virtual/plan.json +++ /dev/null @@ -1,44 +0,0 @@ -{ - "version": "1.0.0", - "pgschema_version": "1.13.0", - "created_at": "1970-01-01T00:00:00Z", - "source_fingerprint": { - "hash": "78438bf9ab91cda45c838d3d085890bbc5d757a8d67a01f5585451e56f7521e4" - }, - "groups": [ - { - "steps": [ - { - "sql": "ALTER TABLE vt DROP COLUMN v2;", - "type": "table.column", - "operation": "drop", - "path": "public.vt.v2" - }, - { - "sql": "ALTER TABLE vt DROP COLUMN s1;", - "type": "table.column", - "operation": "drop", - "path": "public.vt.s1" - }, - { - "sql": "ALTER TABLE vt ADD COLUMN v2 integer;", - "type": "table.column", - "operation": "create", - "path": "public.vt.v2" - }, - { - "sql": "ALTER TABLE vt ADD COLUMN s1 integer GENERATED ALWAYS AS ((a * 2)) VIRTUAL;", - "type": "table.column", - "operation": "create", - "path": "public.vt.s1" - }, - { - "sql": "ALTER TABLE vt ALTER COLUMN v1 SET EXPRESSION AS ((a * 2));", - "type": "table.column", - "operation": "alter", - "path": "public.vt.v1" - } - ] - } - ] -} diff --git a/testdata/diff/create_table/issue_591_alter_generated_virtual/plan.sql b/testdata/diff/create_table/issue_591_alter_generated_virtual/plan.sql deleted file mode 100644 index 9fd07a75..00000000 --- a/testdata/diff/create_table/issue_591_alter_generated_virtual/plan.sql +++ /dev/null @@ -1,9 +0,0 @@ -ALTER TABLE vt DROP COLUMN v2; - -ALTER TABLE vt DROP COLUMN s1; - -ALTER TABLE vt ADD COLUMN v2 integer; - -ALTER TABLE vt ADD COLUMN s1 integer GENERATED ALWAYS AS ((a * 2)) VIRTUAL; - -ALTER TABLE vt ALTER COLUMN v1 SET EXPRESSION AS ((a * 2)); diff --git a/testdata/diff/create_table/issue_591_alter_generated_virtual/plan.txt b/testdata/diff/create_table/issue_591_alter_generated_virtual/plan.txt deleted file mode 100644 index 5539125a..00000000 --- a/testdata/diff/create_table/issue_591_alter_generated_virtual/plan.txt +++ /dev/null @@ -1,25 +0,0 @@ -Plan: 1 to modify. - -Summary by type: - tables: 1 to modify - -Tables: - ~ vt - - s1 (column) - + s1 (column) - ~ v1 (column) - - v2 (column) - + v2 (column) - -DDL to be executed: --------------------------------------------------- - -ALTER TABLE vt DROP COLUMN v2; - -ALTER TABLE vt DROP COLUMN s1; - -ALTER TABLE vt ADD COLUMN v2 integer; - -ALTER TABLE vt ADD COLUMN s1 integer GENERATED ALWAYS AS ((a * 2)) VIRTUAL; - -ALTER TABLE vt ALTER COLUMN v1 SET EXPRESSION AS ((a * 2)); diff --git a/testdata/diff/create_table/issue_591_recreate_generated_dependents/diff.sql b/testdata/diff/create_table/issue_591_recreate_generated_dependents/diff.sql deleted file mode 100644 index 18d3977b..00000000 --- a/testdata/diff/create_table/issue_591_recreate_generated_dependents/diff.sql +++ /dev/null @@ -1,72 +0,0 @@ -DROP VIEW IF EXISTS big_orders RESTRICT; - -DROP VIEW IF EXISTS order_totals RESTRICT; - -DROP VIEW IF EXISTS order_labels RESTRICT; - -DROP TRIGGER IF EXISTS orders_total_trg ON orders; - -ALTER TABLE shipments DROP CONSTRAINT shipments_order_code_fkey; - -DROP POLICY IF EXISTS orders_big ON orders; - -ALTER TABLE orders DROP CONSTRAINT orders_total_excl; - -ALTER TABLE orders DROP COLUMN total; - -ALTER TABLE orders DROP COLUMN code; - -ALTER TABLE orders ADD COLUMN total integer GENERATED ALWAYS AS ((qty * price)) STORED; - -ALTER TABLE orders ADD COLUMN code text GENERATED ALWAYS AS (('ORD-'::text || (id)::text)) STORED; - -ALTER TABLE orders -ADD CONSTRAINT orders_total_excl EXCLUDE USING btree ((total + 0) WITH =); - -CREATE OR REPLACE TRIGGER orders_total_trg - AFTER UPDATE ON orders - FOR EACH ROW - WHEN (((NEW.total > 0))) - EXECUTE FUNCTION orders_audit(); - -CREATE POLICY orders_big ON orders TO PUBLIC USING (total > 100); - -DROP INDEX IF EXISTS orders_code_key; - -CREATE UNIQUE INDEX IF NOT EXISTS orders_code_key ON orders (code); - -DROP INDEX IF EXISTS orders_lookup_idx; - -CREATE INDEX IF NOT EXISTS orders_lookup_idx ON orders (total); - -CREATE UNIQUE INDEX IF NOT EXISTS orders_total_key ON orders (total); - -ALTER TABLE returns -ADD CONSTRAINT returns_order_code_fkey FOREIGN KEY (order_code) REFERENCES orders (code); - -ALTER TABLE returns -ADD CONSTRAINT returns_order_total_fkey FOREIGN KEY (order_total) REFERENCES orders (total); - -ALTER TABLE shipments -ADD CONSTRAINT shipments_order_code_fkey FOREIGN KEY (order_code) REFERENCES orders (code); - -CREATE OR REPLACE VIEW order_labels AS - SELECT id, - 'x'::text AS label - FROM orders; - -CREATE OR REPLACE VIEW order_totals AS - SELECT id, - total - FROM orders; - -CREATE OR REPLACE VIEW big_orders AS - SELECT id - FROM order_totals - WHERE total > 100; - -GRANT SELECT ON TABLE order_totals TO app_reader; - -GRANT SELECT (id) ON TABLE big_orders TO app_reader; - -GRANT SELECT (id, total) ON TABLE orders TO app_reader; diff --git a/testdata/diff/create_table/issue_591_recreate_generated_dependents/new.sql b/testdata/diff/create_table/issue_591_recreate_generated_dependents/new.sql deleted file mode 100644 index 7e71b963..00000000 --- a/testdata/diff/create_table/issue_591_recreate_generated_dependents/new.sql +++ /dev/null @@ -1,59 +0,0 @@ -DO $$ -BEGIN - IF NOT EXISTS (SELECT 1 FROM pg_roles WHERE rolname = 'app_reader') THEN - CREATE ROLE app_reader; - END IF; -END $$; - -CREATE FUNCTION public.orders_audit() RETURNS trigger - LANGUAGE plpgsql - AS $$ BEGIN RETURN NEW; END $$; - -CREATE TABLE public.orders ( - id integer PRIMARY KEY, - qty integer NOT NULL, - price integer NOT NULL, - total integer GENERATED ALWAYS AS (qty * price) STORED, - code text GENERATED ALWAYS AS ('ORD-' || id::text) STORED, - CONSTRAINT orders_total_excl EXCLUDE USING btree ((total + 0) WITH =) -); - -CREATE UNIQUE INDEX orders_code_key ON public.orders (code); - -CREATE INDEX orders_lookup_idx ON public.orders (total); - -CREATE UNIQUE INDEX orders_total_key ON public.orders (total); - -CREATE TABLE public.shipments ( - id integer PRIMARY KEY, - order_code text, - CONSTRAINT shipments_order_code_fkey FOREIGN KEY (order_code) REFERENCES public.orders (code) -); - -CREATE TABLE public.returns ( - id integer PRIMARY KEY, - order_total integer, - order_code text, - CONSTRAINT returns_order_code_fkey FOREIGN KEY (order_code) REFERENCES public.orders (code), - CONSTRAINT returns_order_total_fkey FOREIGN KEY (order_total) REFERENCES public.orders (total) -); - -CREATE VIEW public.order_totals AS SELECT id, total FROM public.orders; - -CREATE VIEW public.big_orders AS SELECT id FROM public.order_totals WHERE total > 100; - -CREATE VIEW public.order_prices AS SELECT id, price FROM public.orders; - -CREATE VIEW public.order_labels AS SELECT id, 'x'::text AS label FROM public.orders; - -ALTER TABLE public.orders ENABLE ROW LEVEL SECURITY; - -CREATE POLICY orders_big ON public.orders USING (total > 100); - -CREATE TRIGGER orders_total_trg AFTER UPDATE ON public.orders FOR EACH ROW WHEN (NEW.total > 0) EXECUTE FUNCTION public.orders_audit(); - -GRANT SELECT (id, total) ON public.orders TO app_reader; - -GRANT SELECT ON public.order_totals TO app_reader; - -GRANT SELECT (id) ON public.big_orders TO app_reader; diff --git a/testdata/diff/create_table/issue_591_recreate_generated_dependents/old.sql b/testdata/diff/create_table/issue_591_recreate_generated_dependents/old.sql deleted file mode 100644 index d1b465c8..00000000 --- a/testdata/diff/create_table/issue_591_recreate_generated_dependents/old.sql +++ /dev/null @@ -1,55 +0,0 @@ -DO $$ -BEGIN - IF NOT EXISTS (SELECT 1 FROM pg_roles WHERE rolname = 'app_reader') THEN - CREATE ROLE app_reader; - END IF; -END $$; - -CREATE FUNCTION public.orders_audit() RETURNS trigger - LANGUAGE plpgsql - AS $$ BEGIN RETURN NEW; END $$; - -CREATE TABLE public.orders ( - id integer PRIMARY KEY, - qty integer NOT NULL, - price integer NOT NULL, - total integer, - code text, - CONSTRAINT orders_total_excl EXCLUDE USING btree ((total + 0) WITH =) -); - -CREATE UNIQUE INDEX orders_code_key ON public.orders (code); - -CREATE INDEX orders_lookup_idx ON public.orders (price); - -CREATE TABLE public.shipments ( - id integer PRIMARY KEY, - order_code text, - CONSTRAINT shipments_order_code_fkey FOREIGN KEY (order_code) REFERENCES public.orders (code) -); - -CREATE TABLE public.returns ( - id integer PRIMARY KEY, - order_total integer, - order_code text -); - -CREATE VIEW public.order_totals AS SELECT id, total FROM public.orders; - -CREATE VIEW public.big_orders AS SELECT id FROM public.order_totals WHERE total > 100; - -CREATE VIEW public.order_prices AS SELECT id, price FROM public.orders; - -CREATE VIEW public.order_labels AS SELECT id, code AS label FROM public.orders; - -ALTER TABLE public.orders ENABLE ROW LEVEL SECURITY; - -CREATE POLICY orders_big ON public.orders USING (total > 100); - -CREATE TRIGGER orders_total_trg AFTER UPDATE ON public.orders FOR EACH ROW WHEN (NEW.total > 0) EXECUTE FUNCTION public.orders_audit(); - -GRANT SELECT (id, total) ON public.orders TO app_reader; - -GRANT SELECT ON public.order_totals TO app_reader; - -GRANT SELECT (id) ON public.big_orders TO app_reader; diff --git a/testdata/diff/create_table/issue_591_recreate_generated_dependents/plan.json b/testdata/diff/create_table/issue_591_recreate_generated_dependents/plan.json deleted file mode 100644 index d6d4e665..00000000 --- a/testdata/diff/create_table/issue_591_recreate_generated_dependents/plan.json +++ /dev/null @@ -1,302 +0,0 @@ -{ - "version": "1.0.0", - "pgschema_version": "1.13.0", - "created_at": "1970-01-01T00:00:00Z", - "source_fingerprint": { - "hash": "9b049588f0373a22af4f2a06007f3c35b1f0bd7dcc3827d2c73456ef55009af2" - }, - "groups": [ - { - "steps": [ - { - "sql": "DROP VIEW IF EXISTS big_orders RESTRICT;", - "type": "view", - "operation": "recreate", - "path": "public.big_orders" - }, - { - "sql": "DROP VIEW IF EXISTS order_totals RESTRICT;", - "type": "view", - "operation": "recreate", - "path": "public.order_totals" - }, - { - "sql": "DROP VIEW IF EXISTS order_labels RESTRICT;", - "type": "view", - "operation": "recreate", - "path": "public.order_labels" - }, - { - "sql": "DROP TRIGGER IF EXISTS orders_total_trg ON orders;", - "type": "table.trigger", - "operation": "drop", - "path": "public.orders.orders_total_trg" - }, - { - "sql": "ALTER TABLE shipments DROP CONSTRAINT shipments_order_code_fkey;", - "type": "table.constraint", - "operation": "drop", - "path": "public.shipments.shipments_order_code_fkey" - }, - { - "sql": "DROP POLICY IF EXISTS orders_big ON orders;", - "type": "table.policy", - "operation": "drop", - "path": "public.orders.orders_big" - }, - { - "sql": "ALTER TABLE orders DROP CONSTRAINT orders_total_excl;", - "type": "table.constraint", - "operation": "drop", - "path": "public.orders.orders_total_excl" - }, - { - "sql": "ALTER TABLE orders DROP COLUMN total;", - "type": "table.column", - "operation": "drop", - "path": "public.orders.total" - }, - { - "sql": "ALTER TABLE orders DROP COLUMN code;", - "type": "table.column", - "operation": "drop", - "path": "public.orders.code" - }, - { - "sql": "ALTER TABLE orders ADD COLUMN total integer GENERATED ALWAYS AS ((qty * price)) STORED;", - "type": "table.column", - "operation": "create", - "path": "public.orders.total" - }, - { - "sql": "ALTER TABLE orders ADD COLUMN code text GENERATED ALWAYS AS (('ORD-'::text || (id)::text)) STORED;", - "type": "table.column", - "operation": "create", - "path": "public.orders.code" - }, - { - "sql": "ALTER TABLE orders\nADD CONSTRAINT orders_total_excl EXCLUDE USING btree ((total + 0) WITH =);", - "type": "table.constraint", - "operation": "create", - "path": "public.orders.orders_total_excl" - }, - { - "sql": "CREATE OR REPLACE TRIGGER orders_total_trg\n AFTER UPDATE ON orders\n FOR EACH ROW\n WHEN (((NEW.total > 0)))\n EXECUTE FUNCTION orders_audit();", - "type": "table.trigger", - "operation": "create", - "path": "public.orders.orders_total_trg" - }, - { - "sql": "CREATE POLICY orders_big ON orders TO PUBLIC USING (total > 100);", - "type": "table.policy", - "operation": "create", - "path": "public.orders.orders_big" - } - ] - }, - { - "steps": [ - { - "sql": "CREATE UNIQUE INDEX CONCURRENTLY IF NOT EXISTS orders_code_key_pgschema_new ON orders (code);", - "type": "table.index", - "operation": "alter", - "path": "public.orders.orders_code_key" - } - ] - }, - { - "steps": [ - { - "sql": "SELECT \n COALESCE(i.indisvalid, false) as done,\n CASE \n WHEN p.blocks_total > 0 THEN p.blocks_done * 100 / p.blocks_total\n ELSE 0\n END as progress\nFROM pg_class c\nLEFT JOIN pg_index i ON c.oid = i.indexrelid\nLEFT JOIN pg_stat_progress_create_index p ON c.oid = p.index_relid\nWHERE c.relname = 'orders_code_key_pgschema_new';", - "directive": { - "type": "wait", - "message": "Creating index orders_code_key_pgschema_new" - }, - "type": "table.index", - "operation": "alter", - "path": "public.orders.orders_code_key" - } - ] - }, - { - "steps": [ - { - "sql": "DROP INDEX IF EXISTS orders_code_key;", - "type": "table.index", - "operation": "alter", - "path": "public.orders.orders_code_key" - }, - { - "sql": "ALTER INDEX orders_code_key_pgschema_new RENAME TO orders_code_key;", - "type": "table.index", - "operation": "alter", - "path": "public.orders.orders_code_key" - } - ] - }, - { - "steps": [ - { - "sql": "CREATE INDEX CONCURRENTLY IF NOT EXISTS orders_lookup_idx_pgschema_new ON orders (total);", - "type": "table.index", - "operation": "alter", - "path": "public.orders.orders_lookup_idx" - } - ] - }, - { - "steps": [ - { - "sql": "SELECT \n COALESCE(i.indisvalid, false) as done,\n CASE \n WHEN p.blocks_total > 0 THEN p.blocks_done * 100 / p.blocks_total\n ELSE 0\n END as progress\nFROM pg_class c\nLEFT JOIN pg_index i ON c.oid = i.indexrelid\nLEFT JOIN pg_stat_progress_create_index p ON c.oid = p.index_relid\nWHERE c.relname = 'orders_lookup_idx_pgschema_new';", - "directive": { - "type": "wait", - "message": "Creating index orders_lookup_idx_pgschema_new" - }, - "type": "table.index", - "operation": "alter", - "path": "public.orders.orders_lookup_idx" - } - ] - }, - { - "steps": [ - { - "sql": "DROP INDEX IF EXISTS orders_lookup_idx;", - "type": "table.index", - "operation": "alter", - "path": "public.orders.orders_lookup_idx" - }, - { - "sql": "ALTER INDEX orders_lookup_idx_pgschema_new RENAME TO orders_lookup_idx;", - "type": "table.index", - "operation": "alter", - "path": "public.orders.orders_lookup_idx" - } - ] - }, - { - "steps": [ - { - "sql": "CREATE UNIQUE INDEX CONCURRENTLY IF NOT EXISTS orders_total_key ON orders (total);", - "type": "table.index", - "operation": "create", - "path": "public.orders.orders_total_key" - } - ] - }, - { - "steps": [ - { - "sql": "SELECT \n COALESCE(i.indisvalid, false) as done,\n CASE \n WHEN p.blocks_total > 0 THEN p.blocks_done * 100 / p.blocks_total\n ELSE 0\n END as progress\nFROM pg_class c\nLEFT JOIN pg_index i ON c.oid = i.indexrelid\nLEFT JOIN pg_stat_progress_create_index p ON c.oid = p.index_relid\nWHERE c.relname = 'orders_total_key';", - "directive": { - "type": "wait", - "message": "Creating index orders_total_key" - }, - "type": "table.index", - "operation": "create", - "path": "public.orders.orders_total_key" - } - ] - }, - { - "steps": [ - { - "sql": "ALTER TABLE returns\nADD CONSTRAINT returns_order_code_fkey FOREIGN KEY (order_code) REFERENCES orders (code) NOT VALID;", - "type": "table.constraint", - "operation": "create", - "path": "public.returns.returns_order_code_fkey" - } - ] - }, - { - "steps": [ - { - "sql": "ALTER TABLE returns VALIDATE CONSTRAINT returns_order_code_fkey;", - "type": "table.constraint", - "operation": "create", - "path": "public.returns.returns_order_code_fkey" - } - ] - }, - { - "steps": [ - { - "sql": "ALTER TABLE returns\nADD CONSTRAINT returns_order_total_fkey FOREIGN KEY (order_total) REFERENCES orders (total) NOT VALID;", - "type": "table.constraint", - "operation": "create", - "path": "public.returns.returns_order_total_fkey" - } - ] - }, - { - "steps": [ - { - "sql": "ALTER TABLE returns VALIDATE CONSTRAINT returns_order_total_fkey;", - "type": "table.constraint", - "operation": "create", - "path": "public.returns.returns_order_total_fkey" - } - ] - }, - { - "steps": [ - { - "sql": "ALTER TABLE shipments\nADD CONSTRAINT shipments_order_code_fkey FOREIGN KEY (order_code) REFERENCES orders (code) NOT VALID;", - "type": "table.constraint", - "operation": "create", - "path": "public.shipments.shipments_order_code_fkey" - } - ] - }, - { - "steps": [ - { - "sql": "ALTER TABLE shipments VALIDATE CONSTRAINT shipments_order_code_fkey;", - "type": "table.constraint", - "operation": "create", - "path": "public.shipments.shipments_order_code_fkey" - } - ] - }, - { - "steps": [ - { - "sql": "CREATE OR REPLACE VIEW order_labels AS\n SELECT id,\n 'x'::text AS label\n FROM orders;", - "type": "view", - "operation": "create", - "path": "public.order_labels" - }, - { - "sql": "CREATE OR REPLACE VIEW order_totals AS\n SELECT id,\n total\n FROM orders;", - "type": "view", - "operation": "create", - "path": "public.order_totals" - }, - { - "sql": "CREATE OR REPLACE VIEW big_orders AS\n SELECT id\n FROM order_totals\n WHERE total > 100;", - "type": "view", - "operation": "recreate", - "path": "public.big_orders" - }, - { - "sql": "GRANT SELECT ON TABLE order_totals TO app_reader;", - "type": "privilege", - "operation": "create", - "path": "privileges.VIEW.order_totals.app_reader" - }, - { - "sql": "GRANT SELECT (id) ON TABLE big_orders TO app_reader;", - "type": "column_privilege", - "operation": "create", - "path": "column_privileges.TABLE.big_orders.id.app_reader" - }, - { - "sql": "GRANT SELECT (id, total) ON TABLE orders TO app_reader;", - "type": "column_privilege", - "operation": "create", - "path": "column_privileges.TABLE.orders.id,total.app_reader" - } - ] - } - ] -} diff --git a/testdata/diff/create_table/issue_591_recreate_generated_dependents/plan.sql b/testdata/diff/create_table/issue_591_recreate_generated_dependents/plan.sql deleted file mode 100644 index 12493f36..00000000 --- a/testdata/diff/create_table/issue_591_recreate_generated_dependents/plan.sql +++ /dev/null @@ -1,118 +0,0 @@ -DROP VIEW IF EXISTS big_orders RESTRICT; - -DROP VIEW IF EXISTS order_totals RESTRICT; - -DROP VIEW IF EXISTS order_labels RESTRICT; - -DROP TRIGGER IF EXISTS orders_total_trg ON orders; - -ALTER TABLE shipments DROP CONSTRAINT shipments_order_code_fkey; - -DROP POLICY IF EXISTS orders_big ON orders; - -ALTER TABLE orders DROP CONSTRAINT orders_total_excl; - -ALTER TABLE orders DROP COLUMN total; - -ALTER TABLE orders DROP COLUMN code; - -ALTER TABLE orders ADD COLUMN total integer GENERATED ALWAYS AS ((qty * price)) STORED; - -ALTER TABLE orders ADD COLUMN code text GENERATED ALWAYS AS (('ORD-'::text || (id)::text)) STORED; - -ALTER TABLE orders -ADD CONSTRAINT orders_total_excl EXCLUDE USING btree ((total + 0) WITH =); - -CREATE OR REPLACE TRIGGER orders_total_trg - AFTER UPDATE ON orders - FOR EACH ROW - WHEN (((NEW.total > 0))) - EXECUTE FUNCTION orders_audit(); - -CREATE POLICY orders_big ON orders TO PUBLIC USING (total > 100); - -CREATE UNIQUE INDEX CONCURRENTLY IF NOT EXISTS orders_code_key_pgschema_new ON orders (code); - --- pgschema:wait -SELECT - COALESCE(i.indisvalid, false) as done, - CASE - WHEN p.blocks_total > 0 THEN p.blocks_done * 100 / p.blocks_total - ELSE 0 - END as progress -FROM pg_class c -LEFT JOIN pg_index i ON c.oid = i.indexrelid -LEFT JOIN pg_stat_progress_create_index p ON c.oid = p.index_relid -WHERE c.relname = 'orders_code_key_pgschema_new'; - -DROP INDEX IF EXISTS orders_code_key; - -ALTER INDEX orders_code_key_pgschema_new RENAME TO orders_code_key; - -CREATE INDEX CONCURRENTLY IF NOT EXISTS orders_lookup_idx_pgschema_new ON orders (total); - --- pgschema:wait -SELECT - COALESCE(i.indisvalid, false) as done, - CASE - WHEN p.blocks_total > 0 THEN p.blocks_done * 100 / p.blocks_total - ELSE 0 - END as progress -FROM pg_class c -LEFT JOIN pg_index i ON c.oid = i.indexrelid -LEFT JOIN pg_stat_progress_create_index p ON c.oid = p.index_relid -WHERE c.relname = 'orders_lookup_idx_pgschema_new'; - -DROP INDEX IF EXISTS orders_lookup_idx; - -ALTER INDEX orders_lookup_idx_pgschema_new RENAME TO orders_lookup_idx; - -CREATE UNIQUE INDEX CONCURRENTLY IF NOT EXISTS orders_total_key ON orders (total); - --- pgschema:wait -SELECT - COALESCE(i.indisvalid, false) as done, - CASE - WHEN p.blocks_total > 0 THEN p.blocks_done * 100 / p.blocks_total - ELSE 0 - END as progress -FROM pg_class c -LEFT JOIN pg_index i ON c.oid = i.indexrelid -LEFT JOIN pg_stat_progress_create_index p ON c.oid = p.index_relid -WHERE c.relname = 'orders_total_key'; - -ALTER TABLE returns -ADD CONSTRAINT returns_order_code_fkey FOREIGN KEY (order_code) REFERENCES orders (code) NOT VALID; - -ALTER TABLE returns VALIDATE CONSTRAINT returns_order_code_fkey; - -ALTER TABLE returns -ADD CONSTRAINT returns_order_total_fkey FOREIGN KEY (order_total) REFERENCES orders (total) NOT VALID; - -ALTER TABLE returns VALIDATE CONSTRAINT returns_order_total_fkey; - -ALTER TABLE shipments -ADD CONSTRAINT shipments_order_code_fkey FOREIGN KEY (order_code) REFERENCES orders (code) NOT VALID; - -ALTER TABLE shipments VALIDATE CONSTRAINT shipments_order_code_fkey; - -CREATE OR REPLACE VIEW order_labels AS - SELECT id, - 'x'::text AS label - FROM orders; - -CREATE OR REPLACE VIEW order_totals AS - SELECT id, - total - FROM orders; - -CREATE OR REPLACE VIEW big_orders AS - SELECT id - FROM order_totals - WHERE total > 100; - -GRANT SELECT ON TABLE order_totals TO app_reader; - -GRANT SELECT (id) ON TABLE big_orders TO app_reader; - -GRANT SELECT (id, total) ON TABLE orders TO app_reader; diff --git a/testdata/diff/create_table/issue_591_recreate_generated_dependents/plan.txt b/testdata/diff/create_table/issue_591_recreate_generated_dependents/plan.txt deleted file mode 100644 index 9c0013ef..00000000 --- a/testdata/diff/create_table/issue_591_recreate_generated_dependents/plan.txt +++ /dev/null @@ -1,179 +0,0 @@ -Plan: 3 to add, 6 to modify. - -Summary by type: - tables: 3 to modify - views: 3 to modify - privileges: 1 to add - column privileges: 2 to add - -Tables: - ~ orders - - code (column) - + code (column) - - total (column) - + total (column) - - orders_total_excl (constraint) - + orders_total_excl (constraint) - ~ orders_code_key (index - concurrent rebuild) - ~ orders_lookup_idx (index - concurrent rebuild) - + orders_total_key (index) - - orders_big (policy) - + orders_big (policy) - - orders_total_trg (trigger) - + orders_total_trg (trigger) - ~ returns - + returns_order_code_fkey (constraint) - + returns_order_total_fkey (constraint) - ~ shipments - - shipments_order_code_fkey (constraint) - + shipments_order_code_fkey (constraint) - -Views: - ~ big_orders - ~ order_labels - ~ order_totals - -Privileges: - + app_reader - -Column privileges: - + app_reader - + app_reader - -DDL to be executed: --------------------------------------------------- - --- Transaction Group #1 -DROP VIEW IF EXISTS big_orders RESTRICT; - -DROP VIEW IF EXISTS order_totals RESTRICT; - -DROP VIEW IF EXISTS order_labels RESTRICT; - -DROP TRIGGER IF EXISTS orders_total_trg ON orders; - -ALTER TABLE shipments DROP CONSTRAINT shipments_order_code_fkey; - -DROP POLICY IF EXISTS orders_big ON orders; - -ALTER TABLE orders DROP CONSTRAINT orders_total_excl; - -ALTER TABLE orders DROP COLUMN total; - -ALTER TABLE orders DROP COLUMN code; - -ALTER TABLE orders ADD COLUMN total integer GENERATED ALWAYS AS ((qty * price)) STORED; - -ALTER TABLE orders ADD COLUMN code text GENERATED ALWAYS AS (('ORD-'::text || (id)::text)) STORED; - -ALTER TABLE orders -ADD CONSTRAINT orders_total_excl EXCLUDE USING btree ((total + 0) WITH =); - -CREATE OR REPLACE TRIGGER orders_total_trg - AFTER UPDATE ON orders - FOR EACH ROW - WHEN (((NEW.total > 0))) - EXECUTE FUNCTION orders_audit(); - -CREATE POLICY orders_big ON orders TO PUBLIC USING (total > 100); - --- Transaction Group #2 -CREATE UNIQUE INDEX CONCURRENTLY IF NOT EXISTS orders_code_key_pgschema_new ON orders (code); - --- Transaction Group #3 --- pgschema:wait -SELECT - COALESCE(i.indisvalid, false) as done, - CASE - WHEN p.blocks_total > 0 THEN p.blocks_done * 100 / p.blocks_total - ELSE 0 - END as progress -FROM pg_class c -LEFT JOIN pg_index i ON c.oid = i.indexrelid -LEFT JOIN pg_stat_progress_create_index p ON c.oid = p.index_relid -WHERE c.relname = 'orders_code_key_pgschema_new'; - --- Transaction Group #4 -DROP INDEX IF EXISTS orders_code_key; - -ALTER INDEX orders_code_key_pgschema_new RENAME TO orders_code_key; - --- Transaction Group #5 -CREATE INDEX CONCURRENTLY IF NOT EXISTS orders_lookup_idx_pgschema_new ON orders (total); - --- Transaction Group #6 --- pgschema:wait -SELECT - COALESCE(i.indisvalid, false) as done, - CASE - WHEN p.blocks_total > 0 THEN p.blocks_done * 100 / p.blocks_total - ELSE 0 - END as progress -FROM pg_class c -LEFT JOIN pg_index i ON c.oid = i.indexrelid -LEFT JOIN pg_stat_progress_create_index p ON c.oid = p.index_relid -WHERE c.relname = 'orders_lookup_idx_pgschema_new'; - --- Transaction Group #7 -DROP INDEX IF EXISTS orders_lookup_idx; - -ALTER INDEX orders_lookup_idx_pgschema_new RENAME TO orders_lookup_idx; - --- Transaction Group #8 -CREATE UNIQUE INDEX CONCURRENTLY IF NOT EXISTS orders_total_key ON orders (total); - --- Transaction Group #9 --- pgschema:wait -SELECT - COALESCE(i.indisvalid, false) as done, - CASE - WHEN p.blocks_total > 0 THEN p.blocks_done * 100 / p.blocks_total - ELSE 0 - END as progress -FROM pg_class c -LEFT JOIN pg_index i ON c.oid = i.indexrelid -LEFT JOIN pg_stat_progress_create_index p ON c.oid = p.index_relid -WHERE c.relname = 'orders_total_key'; - --- Transaction Group #10 -ALTER TABLE returns -ADD CONSTRAINT returns_order_code_fkey FOREIGN KEY (order_code) REFERENCES orders (code) NOT VALID; - --- Transaction Group #11 -ALTER TABLE returns VALIDATE CONSTRAINT returns_order_code_fkey; - --- Transaction Group #12 -ALTER TABLE returns -ADD CONSTRAINT returns_order_total_fkey FOREIGN KEY (order_total) REFERENCES orders (total) NOT VALID; - --- Transaction Group #13 -ALTER TABLE returns VALIDATE CONSTRAINT returns_order_total_fkey; - --- Transaction Group #14 -ALTER TABLE shipments -ADD CONSTRAINT shipments_order_code_fkey FOREIGN KEY (order_code) REFERENCES orders (code) NOT VALID; - --- Transaction Group #15 -ALTER TABLE shipments VALIDATE CONSTRAINT shipments_order_code_fkey; - --- Transaction Group #16 -CREATE OR REPLACE VIEW order_labels AS - SELECT id, - 'x'::text AS label - FROM orders; - -CREATE OR REPLACE VIEW order_totals AS - SELECT id, - total - FROM orders; - -CREATE OR REPLACE VIEW big_orders AS - SELECT id - FROM order_totals - WHERE total > 100; - -GRANT SELECT ON TABLE order_totals TO app_reader; - -GRANT SELECT (id) ON TABLE big_orders TO app_reader; - -GRANT SELECT (id, total) ON TABLE orders TO app_reader;