From 592ce642d93776c0110eff4ffa0d6b1143ef75c6 Mon Sep 17 00:00:00 2001 From: tianzhou Date: Wed, 10 Jun 2026 03:32:56 -0700 Subject: [PATCH 1/5] fix: recreate dependent FKs when a referenced unique/PK constraint is replaced (#439) Replacing a UNIQUE constraint with a PRIMARY KEY (or otherwise dropping or recreating a unique/PK constraint) failed with SQLSTATE 2BP01 when a foreign key in another table was bound to that constraint: the plan emitted a bare ALTER TABLE ... DROP CONSTRAINT with no handling of the dependent FK. Detect desired-state foreign keys whose referenced column set matches a unique/PK constraint being dropped or recreated, and when the FK itself is unchanged, drop it before the table modifications and recreate it afterwards. The final state still matches the schema files exactly; changed or dropped FKs remain handled by their own table diff. Co-Authored-By: Claude Fable 5 --- internal/diff/diff.go | 15 ++ internal/diff/table.go | 130 ++++++++++++++++++ .../diff.sql | 14 ++ .../new.sql | 14 ++ .../old.sql | 15 ++ .../plan.json | 62 +++++++++ .../plan.sql | 18 +++ .../plan.txt | 37 +++++ 8 files changed, 305 insertions(+) create mode 100644 testdata/diff/dependency/issue_439_unique_to_pk_fk_dependent/diff.sql create mode 100644 testdata/diff/dependency/issue_439_unique_to_pk_fk_dependent/new.sql create mode 100644 testdata/diff/dependency/issue_439_unique_to_pk_fk_dependent/old.sql create mode 100644 testdata/diff/dependency/issue_439_unique_to_pk_fk_dependent/plan.json create mode 100644 testdata/diff/dependency/issue_439_unique_to_pk_fk_dependent/plan.sql create mode 100644 testdata/diff/dependency/issue_439_unique_to_pk_fk_dependent/plan.txt diff --git a/internal/diff/diff.go b/internal/diff/diff.go index fe00209f..51fa5310 100644 --- a/internal/diff/diff.go +++ b/internal/diff/diff.go @@ -309,6 +309,10 @@ type ddlDiff struct { // exist when the view body is parsed (issue #414). deferredAddedViews []*ir.View functionsAwaitingDeferredViews []*ir.Function + // Unchanged foreign keys that depend on a unique/PK constraint being dropped + // or recreated by this migration. Dropped before the table modifications and + // recreated afterwards (issue #439). + recreatedFKConstraints []*ir.Constraint } // schemaDiff represents changes to a schema @@ -1459,6 +1463,10 @@ func GenerateMigration(oldIR, newIR *ir.IR, targetSchema string) []Diff { // Sort individual table objects (indexes, triggers, policies, constraints) within each table sortTableObjects(diff.modifiedTables) + // Detect unchanged foreign keys bound to unique/PK constraints being replaced; + // they must be dropped before and recreated after the replacement (issue #439) + diff.recreatedFKConstraints = findFKsDependingOnReplacedConstraints(diff.modifiedTables, oldTables, newTables) + // Create a diffCollector and generate SQL collector := newDiffCollector() diff.collectMigrationSQL(targetSchema, collector) @@ -1912,9 +1920,16 @@ func (d *ddlDiff) generateModifySQL(targetSchema string, collector *diffCollecto // Modify sequences generateModifySequencesSQL(d.modifiedSequences, targetSchema, collector) + // Drop foreign keys bound to unique/PK constraints being replaced, so the + // constraint drops below succeed; they are recreated right after (issue #439) + generateDropRecreatedFKsSQL(d.recreatedFKConstraints, targetSchema, collector) + // Modify tables generateModifyTablesSQL(d.modifiedTables, d.droppedTables, targetSchema, collector) + // Recreate the dropped foreign keys now that the replacement constraints exist + generateAddRecreatedFKsSQL(d.recreatedFKConstraints, targetSchema, collector) + // Create views deferred from generateCreateSQL — their bodies reference // columns just added by ALTER TABLE above (issue #414). Likewise, emit // any functions whose view dependency was on those deferred views. diff --git a/internal/diff/table.go b/internal/diff/table.go index bb5578ed..29a9c884 100644 --- a/internal/diff/table.go +++ b/internal/diff/table.go @@ -561,6 +561,136 @@ func generateModifyTablesSQL(diffs []*tableDiff, droppedTables []*ir.Table, targ } } +// findFKsDependingOnReplacedConstraints returns desired-state foreign keys that +// are bound to a unique/primary-key constraint this migration drops or recreates. +// Postgres ties a foreign key to the specific unique constraint it was created +// against, so the constraint cannot be dropped while the FK exists (SQLSTATE +// 2BP01). FKs that are otherwise unchanged must be dropped before the table +// modifications and recreated after the replacement constraint exists. Changed +// or dropped FKs are already handled by their own table diff. (#439) +func findFKsDependingOnReplacedConstraints(modifiedTables []*tableDiff, oldTables, newTables map[string]*ir.Table) []*ir.Constraint { + // Unique/PK constraints removed by this migration, keyed by their table + replaced := make(map[string][]*ir.Constraint) + for _, td := range modifiedTables { + key := td.Table.Schema + "." + td.Table.Name + for _, c := range td.DroppedConstraints { + if c.Type == ir.ConstraintTypeUnique || c.Type == ir.ConstraintTypePrimaryKey { + replaced[key] = append(replaced[key], c) + } + } + // Modified unique/PK constraints are recreated via DROP + ADD + for _, cd := range td.ModifiedConstraints { + if cd.Old.Type == ir.ConstraintTypeUnique || cd.Old.Type == ir.ConstraintTypePrimaryKey { + replaced[key] = append(replaced[key], cd.Old) + } + } + } + if len(replaced) == 0 { + return nil + } + + var fks []*ir.Constraint + for _, tableKey := range sortedKeys(oldTables) { + oldTable := oldTables[tableKey] + newTable := newTables[tableKey] + if newTable == nil { + continue // table is dropped; its FKs go away with it + } + for _, name := range sortedKeys(oldTable.Constraints) { + fk := oldTable.Constraints[name] + if fk.Type != ir.ConstraintTypeForeignKey { + continue + } + refSchema := fk.ReferencedSchema + if refSchema == "" { + refSchema = fk.Schema + } + if !fkReferencesAnyConstraint(fk, replaced[refSchema+"."+fk.ReferencedTable]) { + continue + } + newFK := newTable.Constraints[name] + if newFK == nil || !constraintsEqual(fk, newFK) { + continue + } + fks = append(fks, newFK) + } + } + return fks +} + +// fkReferencesAnyConstraint reports whether the FK's referenced columns match +// the column set of any of the given unique/PK constraints. +func fkReferencesAnyConstraint(fk *ir.Constraint, candidates []*ir.Constraint) bool { + refCols := make(map[string]bool, len(fk.ReferencedColumns)) + for _, col := range fk.ReferencedColumns { + refCols[col.Name] = true + } + for _, c := range candidates { + if len(c.Columns) != len(refCols) { + continue + } + matched := true + for _, col := range c.Columns { + if !refCols[col.Name] { + matched = false + break + } + } + if matched { + return true + } + } + return false +} + +// generateDropRecreatedFKsSQL drops foreign keys bound to unique/PK constraints +// being replaced. Must run before the table modifications. (#439) +func generateDropRecreatedFKsSQL(fks []*ir.Constraint, targetSchema string, collector *diffCollector) { + for _, fk := range fks { + tableName := getTableNameWithSchema(fk.Schema, fk.Table, targetSchema) + sql := fmt.Sprintf("ALTER TABLE %s DROP CONSTRAINT %s;", tableName, ir.QuoteIdentifier(fk.Name)) + + context := &diffContext{ + Type: DiffTypeTableConstraint, + Operation: DiffOperationDrop, + Path: fmt.Sprintf("%s.%s.%s", fk.Schema, fk.Table, fk.Name), + Source: fk, + CanRunInTransaction: true, + } + collector.collect(context, sql) + } +} + +// generateAddRecreatedFKsSQL recreates the foreign keys dropped by +// generateDropRecreatedFKsSQL, after the replacement constraints exist. (#439) +func generateAddRecreatedFKsSQL(fks []*ir.Constraint, targetSchema string, collector *diffCollector) { + for _, fk := range fks { + columns := sortConstraintColumnsByPosition(fk.Columns) + var columnNames []string + for _, col := range columns { + columnNames = append(columnNames, ir.QuoteIdentifier(col.Name)) + } + if fk.IsTemporal && len(columnNames) > 0 { + columnNames[len(columnNames)-1] = "PERIOD " + columnNames[len(columnNames)-1] + } + + tableName := getTableNameWithSchema(fk.Schema, fk.Table, targetSchema) + sql := fmt.Sprintf("ALTER TABLE %s\nADD CONSTRAINT %s FOREIGN KEY (%s) %s;", + tableName, ir.QuoteIdentifier(fk.Name), + strings.Join(columnNames, ", "), + generateForeignKeyClause(fk, targetSchema, false)) + + context := &diffContext{ + Type: DiffTypeTableConstraint, + Operation: DiffOperationCreate, + Path: fmt.Sprintf("%s.%s.%s", fk.Schema, fk.Table, fk.Name), + Source: fk, + CanRunInTransaction: true, + } + collector.collect(context, sql) + } +} + // generateDropTablesSQL generates DROP TABLE statements // Tables are assumed to be pre-sorted in reverse topological order for dependency-aware dropping func generateDropTablesSQL(tables []*ir.Table, targetSchema string, collector *diffCollector) { diff --git a/testdata/diff/dependency/issue_439_unique_to_pk_fk_dependent/diff.sql b/testdata/diff/dependency/issue_439_unique_to_pk_fk_dependent/diff.sql new file mode 100644 index 00000000..e38b0bdf --- /dev/null +++ b/testdata/diff/dependency/issue_439_unique_to_pk_fk_dependent/diff.sql @@ -0,0 +1,14 @@ +ALTER TABLE case_notes DROP CONSTRAINT case_notes_case_id_fkey; + +ALTER TABLE case_tags DROP CONSTRAINT case_tags_case_id_fkey; + +ALTER TABLE cases DROP CONSTRAINT cases_id_key; + +ALTER TABLE cases +ADD CONSTRAINT cases_pkey PRIMARY KEY (id); + +ALTER TABLE case_notes +ADD CONSTRAINT case_notes_case_id_fkey FOREIGN KEY (case_id) REFERENCES cases (id); + +ALTER TABLE case_tags +ADD CONSTRAINT case_tags_case_id_fkey FOREIGN KEY (case_id) REFERENCES cases (id) ON DELETE CASCADE; diff --git a/testdata/diff/dependency/issue_439_unique_to_pk_fk_dependent/new.sql b/testdata/diff/dependency/issue_439_unique_to_pk_fk_dependent/new.sql new file mode 100644 index 00000000..fbc8bccb --- /dev/null +++ b/testdata/diff/dependency/issue_439_unique_to_pk_fk_dependent/new.sql @@ -0,0 +1,14 @@ +CREATE TABLE cases ( + id integer PRIMARY KEY, + name text +); + +CREATE TABLE case_notes ( + id integer PRIMARY KEY, + case_id integer CONSTRAINT case_notes_case_id_fkey REFERENCES cases (id) +); + +CREATE TABLE case_tags ( + id integer PRIMARY KEY, + case_id integer CONSTRAINT case_tags_case_id_fkey REFERENCES cases (id) ON DELETE CASCADE +); diff --git a/testdata/diff/dependency/issue_439_unique_to_pk_fk_dependent/old.sql b/testdata/diff/dependency/issue_439_unique_to_pk_fk_dependent/old.sql new file mode 100644 index 00000000..d1428781 --- /dev/null +++ b/testdata/diff/dependency/issue_439_unique_to_pk_fk_dependent/old.sql @@ -0,0 +1,15 @@ +CREATE TABLE cases ( + id integer NOT NULL, + name text, + CONSTRAINT cases_id_key UNIQUE (id) +); + +CREATE TABLE case_notes ( + id integer PRIMARY KEY, + case_id integer CONSTRAINT case_notes_case_id_fkey REFERENCES cases (id) +); + +CREATE TABLE case_tags ( + id integer PRIMARY KEY, + case_id integer CONSTRAINT case_tags_case_id_fkey REFERENCES cases (id) ON DELETE CASCADE +); diff --git a/testdata/diff/dependency/issue_439_unique_to_pk_fk_dependent/plan.json b/testdata/diff/dependency/issue_439_unique_to_pk_fk_dependent/plan.json new file mode 100644 index 00000000..3a128b1d --- /dev/null +++ b/testdata/diff/dependency/issue_439_unique_to_pk_fk_dependent/plan.json @@ -0,0 +1,62 @@ +{ + "version": "1.0.0", + "pgschema_version": "1.11.0", + "created_at": "1970-01-01T00:00:00Z", + "source_fingerprint": { + "hash": "67b8ade4e7f5a944045178db3d613964042ecb7e1deb5f497db25c5251d3c430" + }, + "groups": [ + { + "steps": [ + { + "sql": "ALTER TABLE case_notes DROP CONSTRAINT case_notes_case_id_fkey;", + "type": "table.constraint", + "operation": "drop", + "path": "public.case_notes.case_notes_case_id_fkey" + }, + { + "sql": "ALTER TABLE case_tags DROP CONSTRAINT case_tags_case_id_fkey;", + "type": "table.constraint", + "operation": "drop", + "path": "public.case_tags.case_tags_case_id_fkey" + }, + { + "sql": "ALTER TABLE cases DROP CONSTRAINT cases_id_key;", + "type": "table.constraint", + "operation": "drop", + "path": "public.cases.cases_id_key" + }, + { + "sql": "ALTER TABLE cases\nADD CONSTRAINT cases_pkey PRIMARY KEY (id);", + "type": "table.constraint", + "operation": "create", + "path": "public.cases.cases_pkey" + }, + { + "sql": "ALTER TABLE case_notes\nADD CONSTRAINT case_notes_case_id_fkey FOREIGN KEY (case_id) REFERENCES cases (id) NOT VALID;", + "type": "table.constraint", + "operation": "create", + "path": "public.case_notes.case_notes_case_id_fkey" + }, + { + "sql": "ALTER TABLE case_notes VALIDATE CONSTRAINT case_notes_case_id_fkey;", + "type": "table.constraint", + "operation": "create", + "path": "public.case_notes.case_notes_case_id_fkey" + }, + { + "sql": "ALTER TABLE case_tags\nADD CONSTRAINT case_tags_case_id_fkey FOREIGN KEY (case_id) REFERENCES cases (id) ON DELETE CASCADE NOT VALID;", + "type": "table.constraint", + "operation": "create", + "path": "public.case_tags.case_tags_case_id_fkey" + }, + { + "sql": "ALTER TABLE case_tags VALIDATE CONSTRAINT case_tags_case_id_fkey;", + "type": "table.constraint", + "operation": "create", + "path": "public.case_tags.case_tags_case_id_fkey" + } + ] + } + ] +} diff --git a/testdata/diff/dependency/issue_439_unique_to_pk_fk_dependent/plan.sql b/testdata/diff/dependency/issue_439_unique_to_pk_fk_dependent/plan.sql new file mode 100644 index 00000000..8ebbff84 --- /dev/null +++ b/testdata/diff/dependency/issue_439_unique_to_pk_fk_dependent/plan.sql @@ -0,0 +1,18 @@ +ALTER TABLE case_notes DROP CONSTRAINT case_notes_case_id_fkey; + +ALTER TABLE case_tags DROP CONSTRAINT case_tags_case_id_fkey; + +ALTER TABLE cases DROP CONSTRAINT cases_id_key; + +ALTER TABLE cases +ADD CONSTRAINT cases_pkey PRIMARY KEY (id); + +ALTER TABLE case_notes +ADD CONSTRAINT case_notes_case_id_fkey FOREIGN KEY (case_id) REFERENCES cases (id) NOT VALID; + +ALTER TABLE case_notes VALIDATE CONSTRAINT case_notes_case_id_fkey; + +ALTER TABLE case_tags +ADD CONSTRAINT case_tags_case_id_fkey FOREIGN KEY (case_id) REFERENCES cases (id) ON DELETE CASCADE NOT VALID; + +ALTER TABLE case_tags VALIDATE CONSTRAINT case_tags_case_id_fkey; diff --git a/testdata/diff/dependency/issue_439_unique_to_pk_fk_dependent/plan.txt b/testdata/diff/dependency/issue_439_unique_to_pk_fk_dependent/plan.txt new file mode 100644 index 00000000..42808842 --- /dev/null +++ b/testdata/diff/dependency/issue_439_unique_to_pk_fk_dependent/plan.txt @@ -0,0 +1,37 @@ +Plan: 3 to modify. + +Summary by type: + tables: 3 to modify + +Tables: + ~ case_notes + - case_notes_case_id_fkey (constraint) + + case_notes_case_id_fkey (constraint) + ~ case_tags + - case_tags_case_id_fkey (constraint) + + case_tags_case_id_fkey (constraint) + ~ cases + - cases_id_key (constraint) + + cases_pkey (constraint) + +DDL to be executed: +-------------------------------------------------- + +ALTER TABLE case_notes DROP CONSTRAINT case_notes_case_id_fkey; + +ALTER TABLE case_tags DROP CONSTRAINT case_tags_case_id_fkey; + +ALTER TABLE cases DROP CONSTRAINT cases_id_key; + +ALTER TABLE cases +ADD CONSTRAINT cases_pkey PRIMARY KEY (id); + +ALTER TABLE case_notes +ADD CONSTRAINT case_notes_case_id_fkey FOREIGN KEY (case_id) REFERENCES cases (id) NOT VALID; + +ALTER TABLE case_notes VALIDATE CONSTRAINT case_notes_case_id_fkey; + +ALTER TABLE case_tags +ADD CONSTRAINT case_tags_case_id_fkey FOREIGN KEY (case_id) REFERENCES cases (id) ON DELETE CASCADE NOT VALID; + +ALTER TABLE case_tags VALIDATE CONSTRAINT case_tags_case_id_fkey; From 4b68e99be1cc815ec51e359674f3e312f7d758a4 Mon Sep 17 00:00:00 2001 From: tianzhou Date: Wed, 10 Jun 2026 04:00:27 -0700 Subject: [PATCH 2/5] fix: handle changed FKs and new-table FKs bound to replaced unique/PK constraints (#439) Extends the dependent-FK handling to two scenarios flagged in review: - An FK that itself changes in the same migration as the unique->PK swap was left on the normal table-diff path, where its recreate could run before the swap and bind to the old constraint. FKs whose old or new definition targets a replaced constraint are now routed through the pre-drop/post-add path and removed from their own table diff. - An FK on a newly added table referencing the replaced constraint was emitted inline in CREATE TABLE during the create phase, binding to the old constraint before the modify-phase swap. Such FKs are now kept out of CREATE TABLE and created after the replacement constraint exists. Verified against live PostgreSQL that FK-to-unique matching is by column set, not ordered list (FOREIGN KEY (x, y) REFERENCES t (b, a) is valid against UNIQUE (a, b)), so the order-insensitive matching is kept. Co-Authored-By: Claude Fable 5 --- internal/diff/diff.go | 29 ++-- internal/diff/table.go | 136 +++++++++++++++--- .../diff.sql | 9 ++ .../new.sql | 7 + .../old.sql | 8 ++ .../plan.json | 44 ++++++ .../plan.sql | 11 ++ .../plan.txt | 27 ++++ .../diff.sql | 13 ++ .../new.sql | 7 + .../old.sql | 4 + .../plan.json | 38 +++++ .../plan.sql | 13 ++ .../plan.txt | 28 ++++ 14 files changed, 341 insertions(+), 33 deletions(-) create mode 100644 testdata/diff/dependency/issue_439_replaced_unique_changed_fk/diff.sql create mode 100644 testdata/diff/dependency/issue_439_replaced_unique_changed_fk/new.sql create mode 100644 testdata/diff/dependency/issue_439_replaced_unique_changed_fk/old.sql create mode 100644 testdata/diff/dependency/issue_439_replaced_unique_changed_fk/plan.json create mode 100644 testdata/diff/dependency/issue_439_replaced_unique_changed_fk/plan.sql create mode 100644 testdata/diff/dependency/issue_439_replaced_unique_changed_fk/plan.txt create mode 100644 testdata/diff/dependency/issue_439_replaced_unique_new_table_fk/diff.sql create mode 100644 testdata/diff/dependency/issue_439_replaced_unique_new_table_fk/new.sql create mode 100644 testdata/diff/dependency/issue_439_replaced_unique_new_table_fk/old.sql create mode 100644 testdata/diff/dependency/issue_439_replaced_unique_new_table_fk/plan.json create mode 100644 testdata/diff/dependency/issue_439_replaced_unique_new_table_fk/plan.sql create mode 100644 testdata/diff/dependency/issue_439_replaced_unique_new_table_fk/plan.txt diff --git a/internal/diff/diff.go b/internal/diff/diff.go index 51fa5310..5b1b2dad 100644 --- a/internal/diff/diff.go +++ b/internal/diff/diff.go @@ -309,10 +309,15 @@ type ddlDiff struct { // exist when the view body is parsed (issue #414). deferredAddedViews []*ir.View functionsAwaitingDeferredViews []*ir.Function - // Unchanged foreign keys that depend on a unique/PK constraint being dropped - // or recreated by this migration. Dropped before the table modifications and - // recreated afterwards (issue #439). - recreatedFKConstraints []*ir.Constraint + // Foreign keys that depend on a unique/PK constraint being dropped or + // recreated by this migration: existing ones are dropped before the table + // modifications (fkPreDrops) and desired-state ones are (re)created + // afterwards (fkPostAdds). FKs on newly added tables that would bind to the + // old constraint are kept out of CREATE TABLE (suppressedInlineFKs) and + // created via fkPostAdds instead (issue #439). + fkPreDrops []*ir.Constraint + fkPostAdds []*ir.Constraint + suppressedInlineFKs map[string]bool } // schemaDiff represents changes to a schema @@ -1463,9 +1468,9 @@ func GenerateMigration(oldIR, newIR *ir.IR, targetSchema string) []Diff { // Sort individual table objects (indexes, triggers, policies, constraints) within each table sortTableObjects(diff.modifiedTables) - // Detect unchanged foreign keys bound to unique/PK constraints being replaced; - // they must be dropped before and recreated after the replacement (issue #439) - diff.recreatedFKConstraints = findFKsDependingOnReplacedConstraints(diff.modifiedTables, oldTables, newTables) + // Detect foreign keys bound to unique/PK constraints being replaced; they + // must be dropped before and recreated after the replacement (issue #439) + diff.fkPreDrops, diff.fkPostAdds, diff.suppressedInlineFKs = planFKRecreationForReplacedConstraints(diff.modifiedTables, diff.addedTables, oldTables, newTables) // Create a diffCollector and generate SQL collector := newDiffCollector() @@ -1780,7 +1785,7 @@ func (d *ddlDiff) generateCreateSQL(targetSchema string, collector *diffCollecto } // Create tables WITHOUT function/domain dependencies first (functions may reference these) - deferredPolicies1, deferredConstraints1 := generateCreateTablesSQL(tablesWithoutDeps, targetSchema, collector, existingTables, shouldDeferPolicy) + deferredPolicies1, deferredConstraints1 := generateCreateTablesSQL(tablesWithoutDeps, targetSchema, collector, existingTables, shouldDeferPolicy, d.suppressedInlineFKs) // Build view lookup - needed for detecting functions that depend on views newViewLookup := buildViewLookup(d.addedViews) @@ -1811,7 +1816,7 @@ func (d *ddlDiff) generateCreateSQL(targetSchema string, collector *diffCollecto generateCreateProceduresSQL(d.addedProcedures, targetSchema, collector) // Create tables WITH function/domain dependencies (now that functions and deferred domains exist) - deferredPolicies2, deferredConstraints2 := generateCreateTablesSQL(tablesWithDeps, targetSchema, collector, existingTables, shouldDeferPolicy) + deferredPolicies2, deferredConstraints2 := generateCreateTablesSQL(tablesWithDeps, targetSchema, collector, existingTables, shouldDeferPolicy, d.suppressedInlineFKs) // Add deferred foreign key constraints from BOTH batches AFTER all tables are created // This ensures FK references to tables in the second batch (function-dependent tables) work correctly @@ -1922,13 +1927,13 @@ func (d *ddlDiff) generateModifySQL(targetSchema string, collector *diffCollecto // Drop foreign keys bound to unique/PK constraints being replaced, so the // constraint drops below succeed; they are recreated right after (issue #439) - generateDropRecreatedFKsSQL(d.recreatedFKConstraints, targetSchema, collector) + generateDropRecreatedFKsSQL(d.fkPreDrops, targetSchema, collector) // Modify tables generateModifyTablesSQL(d.modifiedTables, d.droppedTables, targetSchema, collector) - // Recreate the dropped foreign keys now that the replacement constraints exist - generateAddRecreatedFKsSQL(d.recreatedFKConstraints, targetSchema, collector) + // (Re)create the dependent foreign keys now that the replacement constraints exist + generateAddRecreatedFKsSQL(d.fkPostAdds, targetSchema, collector) // Create views deferred from generateCreateSQL — their bodies reference // columns just added by ALTER TABLE above (issue #414). Likewise, emit diff --git a/internal/diff/table.go b/internal/diff/table.go index 29a9c884..c1771431 100644 --- a/internal/diff/table.go +++ b/internal/diff/table.go @@ -400,6 +400,7 @@ func generateCreateTablesSQL( collector *diffCollector, existingTables map[string]bool, shouldDeferPolicy func(*ir.RLSPolicy) bool, + suppressedInlineFKs map[string]bool, ) ([]*ir.RLSPolicy, []*deferredConstraint) { var deferredPolicies []*ir.RLSPolicy var deferredConstraints []*deferredConstraint @@ -408,7 +409,7 @@ func generateCreateTablesSQL( // Process tables in the provided order (already topologically sorted) for _, table := range tables { // Create the table, deferring FK constraints that reference not-yet-created tables - sql, tableDeferred := generateTableSQL(table, targetSchema, createdTables, existingTables) + sql, tableDeferred := generateTableSQL(table, targetSchema, createdTables, existingTables, suppressedInlineFKs) deferredConstraints = append(deferredConstraints, tableDeferred...) // Create context for this statement @@ -561,18 +562,33 @@ func generateModifyTablesSQL(diffs []*tableDiff, droppedTables []*ir.Table, targ } } -// findFKsDependingOnReplacedConstraints returns desired-state foreign keys that -// are bound to a unique/primary-key constraint this migration drops or recreates. -// Postgres ties a foreign key to the specific unique constraint it was created -// against, so the constraint cannot be dropped while the FK exists (SQLSTATE -// 2BP01). FKs that are otherwise unchanged must be dropped before the table -// modifications and recreated after the replacement constraint exists. Changed -// or dropped FKs are already handled by their own table diff. (#439) -func findFKsDependingOnReplacedConstraints(modifiedTables []*tableDiff, oldTables, newTables map[string]*ir.Table) []*ir.Constraint { +// planFKRecreationForReplacedConstraints handles foreign keys that depend on a +// unique/primary-key constraint this migration drops or recreates. Postgres +// ties a foreign key to the specific unique constraint it was created against, +// so the constraint cannot be dropped while the FK exists (SQLSTATE 2BP01), +// and an FK created while the old constraint still exists binds to it. (#439) +// +// It returns: +// - preDrops: old-state FKs bound to a replaced constraint, to drop before +// the table modifications. Their drop/modify entries are removed from +// their own table diff so they are not dropped twice. +// - postAdds: desired-state FKs to (re)create after the table modifications, +// once the replacement constraint exists. +// - suppressedInlineFKs: FKs on newly added tables (keyed schema.table.name) +// that must be left out of CREATE TABLE — emitted inline they would bind +// to the old constraint before the replacement runs. They are recreated +// via postAdds instead. +// +// Note: Postgres matches an FK's referenced columns to a unique constraint as +// a column set, not an ordered list — FOREIGN KEY (x, y) REFERENCES t (b, a) +// is valid against UNIQUE (a, b) — so matching here is order-insensitive. +func planFKRecreationForReplacedConstraints(modifiedTables []*tableDiff, addedTables []*ir.Table, oldTables, newTables map[string]*ir.Table) (preDrops, postAdds []*ir.Constraint, suppressedInlineFKs map[string]bool) { // Unique/PK constraints removed by this migration, keyed by their table replaced := make(map[string][]*ir.Constraint) + tableDiffByKey := make(map[string]*tableDiff, len(modifiedTables)) for _, td := range modifiedTables { key := td.Table.Schema + "." + td.Table.Name + tableDiffByKey[key] = td for _, c := range td.DroppedConstraints { if c.Type == ir.ConstraintTypeUnique || c.Type == ir.ConstraintTypePrimaryKey { replaced[key] = append(replaced[key], c) @@ -586,36 +602,76 @@ func findFKsDependingOnReplacedConstraints(modifiedTables []*tableDiff, oldTable } } if len(replaced) == 0 { - return nil + return nil, nil, nil } - var fks []*ir.Constraint + // Existing FKs bound to a replaced constraint for _, tableKey := range sortedKeys(oldTables) { oldTable := oldTables[tableKey] newTable := newTables[tableKey] if newTable == nil { - continue // table is dropped; its FKs go away with it + continue // table is dropped before the modify phase; its FKs go with it } + td := tableDiffByKey[tableKey] for _, name := range sortedKeys(oldTable.Constraints) { fk := oldTable.Constraints[name] if fk.Type != ir.ConstraintTypeForeignKey { continue } - refSchema := fk.ReferencedSchema - if refSchema == "" { - refSchema = fk.Schema + newFK := newTable.Constraints[name] + oldBound := fkReferencesAnyConstraint(fk, replaced[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)]) + if !oldBound && !newBound { + continue } - if !fkReferencesAnyConstraint(fk, replaced[refSchema+"."+fk.ReferencedTable]) { + + preDrops = append(preDrops, fk) + if td != nil { + // The FK is now dropped in the pre-drop step; remove it from its + // own table diff so it is not dropped twice or re-added too early. + td.DroppedConstraints = removeConstraintByName(td.DroppedConstraints, name) + td.ModifiedConstraints = removeConstraintDiffByName(td.ModifiedConstraints, name) + } + if newFK != nil { + postAdds = append(postAdds, newFK) + } + } + } + + // FKs on newly added tables that target a replaced constraint: keep them + // out of CREATE TABLE (create phase runs before the modify-phase swap) and + // add them with the other recreated FKs instead. + suppressedInlineFKs = make(map[string]bool) + for _, table := range addedTables { + for _, name := range sortedKeys(table.Constraints) { + fk := table.Constraints[name] + if fk.Type != ir.ConstraintTypeForeignKey { continue } - newFK := newTable.Constraints[name] - if newFK == nil || !constraintsEqual(fk, newFK) { + if !fkReferencesAnyConstraint(fk, replaced[fkReferencedTableKey(fk)]) { continue } - fks = append(fks, newFK) + suppressedInlineFKs[table.Schema+"."+table.Name+"."+name] = true + postAdds = append(postAdds, fk) } } - return fks + + sortConstraintsByPath(preDrops) + sortConstraintsByPath(postAdds) + return preDrops, postAdds, suppressedInlineFKs +} + +// fkReferencedTableKey returns the schema-qualified key of the table an FK references. +func fkReferencedTableKey(fk *ir.Constraint) string { + refSchema := fk.ReferencedSchema + if refSchema == "" { + refSchema = fk.Schema + } + return refSchema + "." + fk.ReferencedTable } // fkReferencesAnyConstraint reports whether the FK's referenced columns match @@ -643,6 +699,39 @@ func fkReferencesAnyConstraint(fk *ir.Constraint, candidates []*ir.Constraint) b return false } +func removeConstraintByName(list []*ir.Constraint, name string) []*ir.Constraint { + result := list[:0] + for _, c := range list { + if c.Name != name { + result = append(result, c) + } + } + return result +} + +func removeConstraintDiffByName(list []*ConstraintDiff, name string) []*ConstraintDiff { + result := list[:0] + for _, cd := range list { + if cd.Old.Name != name { + result = append(result, cd) + } + } + return result +} + +func sortConstraintsByPath(constraints []*ir.Constraint) { + sort.Slice(constraints, func(i, j int) bool { + a, b := constraints[i], constraints[j] + if a.Schema != b.Schema { + return a.Schema < b.Schema + } + if a.Table != b.Table { + return a.Table < b.Table + } + return a.Name < b.Name + }) +} + // generateDropRecreatedFKsSQL drops foreign keys bound to unique/PK constraints // being replaced. Must run before the table modifications. (#439) func generateDropRecreatedFKsSQL(fks []*ir.Constraint, targetSchema string, collector *diffCollector) { @@ -713,7 +802,7 @@ func generateDropTablesSQL(tables []*ir.Table, targetSchema string, collector *d } // generateTableSQL generates CREATE TABLE statement and returns any deferred FK constraints -func generateTableSQL(table *ir.Table, targetSchema string, createdTables map[string]bool, existingTables map[string]bool) (string, []*deferredConstraint) { +func generateTableSQL(table *ir.Table, targetSchema string, createdTables map[string]bool, existingTables map[string]bool, suppressedInlineFKs map[string]bool) (string, []*deferredConstraint) { // Only include table name without schema if it's in the target schema tableName := ir.QualifyEntityNameWithQuotes(table.Schema, table.Name, targetSchema) @@ -748,6 +837,11 @@ func generateTableSQL(table *ir.Table, targetSchema string, createdTables map[st var deferred []*deferredConstraint currentKey := fmt.Sprintf("%s.%s", table.Schema, table.Name) for _, constraint := range inlineConstraints { + // FKs bound to a unique/PK constraint being replaced by this migration + // are created after the replacement instead (issue #439) + if suppressedInlineFKs[currentKey+"."+constraint.Name] { + continue + } if shouldDeferConstraint(table, constraint, currentKey, createdTables, existingTables) { deferred = append(deferred, &deferredConstraint{ table: table, diff --git a/testdata/diff/dependency/issue_439_replaced_unique_changed_fk/diff.sql b/testdata/diff/dependency/issue_439_replaced_unique_changed_fk/diff.sql new file mode 100644 index 00000000..768eead3 --- /dev/null +++ b/testdata/diff/dependency/issue_439_replaced_unique_changed_fk/diff.sql @@ -0,0 +1,9 @@ +ALTER TABLE case_notes DROP CONSTRAINT case_notes_case_id_fkey; + +ALTER TABLE cases DROP CONSTRAINT cases_id_key; + +ALTER TABLE cases +ADD CONSTRAINT cases_pkey PRIMARY KEY (id); + +ALTER TABLE case_notes +ADD CONSTRAINT case_notes_case_id_fkey FOREIGN KEY (case_id) REFERENCES cases (id) ON DELETE CASCADE; diff --git a/testdata/diff/dependency/issue_439_replaced_unique_changed_fk/new.sql b/testdata/diff/dependency/issue_439_replaced_unique_changed_fk/new.sql new file mode 100644 index 00000000..f7476089 --- /dev/null +++ b/testdata/diff/dependency/issue_439_replaced_unique_changed_fk/new.sql @@ -0,0 +1,7 @@ +CREATE TABLE cases ( + id integer PRIMARY KEY +); +CREATE TABLE case_notes ( + id integer PRIMARY KEY, + case_id integer CONSTRAINT case_notes_case_id_fkey REFERENCES cases (id) ON DELETE CASCADE +); diff --git a/testdata/diff/dependency/issue_439_replaced_unique_changed_fk/old.sql b/testdata/diff/dependency/issue_439_replaced_unique_changed_fk/old.sql new file mode 100644 index 00000000..499ede0d --- /dev/null +++ b/testdata/diff/dependency/issue_439_replaced_unique_changed_fk/old.sql @@ -0,0 +1,8 @@ +CREATE TABLE cases ( + id integer NOT NULL, + CONSTRAINT cases_id_key UNIQUE (id) +); +CREATE TABLE case_notes ( + id integer PRIMARY KEY, + case_id integer CONSTRAINT case_notes_case_id_fkey REFERENCES cases (id) +); diff --git a/testdata/diff/dependency/issue_439_replaced_unique_changed_fk/plan.json b/testdata/diff/dependency/issue_439_replaced_unique_changed_fk/plan.json new file mode 100644 index 00000000..19890826 --- /dev/null +++ b/testdata/diff/dependency/issue_439_replaced_unique_changed_fk/plan.json @@ -0,0 +1,44 @@ +{ + "version": "1.0.0", + "pgschema_version": "1.11.0", + "created_at": "1970-01-01T00:00:00Z", + "source_fingerprint": { + "hash": "aef1cfad9933c06a8c7e330d6c6bbcf10cd4255648eb2dcd3719758b8c373cb1" + }, + "groups": [ + { + "steps": [ + { + "sql": "ALTER TABLE case_notes DROP CONSTRAINT case_notes_case_id_fkey;", + "type": "table.constraint", + "operation": "drop", + "path": "public.case_notes.case_notes_case_id_fkey" + }, + { + "sql": "ALTER TABLE cases DROP CONSTRAINT cases_id_key;", + "type": "table.constraint", + "operation": "drop", + "path": "public.cases.cases_id_key" + }, + { + "sql": "ALTER TABLE cases\nADD CONSTRAINT cases_pkey PRIMARY KEY (id);", + "type": "table.constraint", + "operation": "create", + "path": "public.cases.cases_pkey" + }, + { + "sql": "ALTER TABLE case_notes\nADD CONSTRAINT case_notes_case_id_fkey FOREIGN KEY (case_id) REFERENCES cases (id) ON DELETE CASCADE NOT VALID;", + "type": "table.constraint", + "operation": "create", + "path": "public.case_notes.case_notes_case_id_fkey" + }, + { + "sql": "ALTER TABLE case_notes VALIDATE CONSTRAINT case_notes_case_id_fkey;", + "type": "table.constraint", + "operation": "create", + "path": "public.case_notes.case_notes_case_id_fkey" + } + ] + } + ] +} diff --git a/testdata/diff/dependency/issue_439_replaced_unique_changed_fk/plan.sql b/testdata/diff/dependency/issue_439_replaced_unique_changed_fk/plan.sql new file mode 100644 index 00000000..321f380d --- /dev/null +++ b/testdata/diff/dependency/issue_439_replaced_unique_changed_fk/plan.sql @@ -0,0 +1,11 @@ +ALTER TABLE case_notes DROP CONSTRAINT case_notes_case_id_fkey; + +ALTER TABLE cases DROP CONSTRAINT cases_id_key; + +ALTER TABLE cases +ADD CONSTRAINT cases_pkey PRIMARY KEY (id); + +ALTER TABLE case_notes +ADD CONSTRAINT case_notes_case_id_fkey FOREIGN KEY (case_id) REFERENCES cases (id) ON DELETE CASCADE NOT VALID; + +ALTER TABLE case_notes VALIDATE CONSTRAINT case_notes_case_id_fkey; diff --git a/testdata/diff/dependency/issue_439_replaced_unique_changed_fk/plan.txt b/testdata/diff/dependency/issue_439_replaced_unique_changed_fk/plan.txt new file mode 100644 index 00000000..f122c148 --- /dev/null +++ b/testdata/diff/dependency/issue_439_replaced_unique_changed_fk/plan.txt @@ -0,0 +1,27 @@ +Plan: 2 to modify. + +Summary by type: + tables: 2 to modify + +Tables: + ~ case_notes + - case_notes_case_id_fkey (constraint) + + case_notes_case_id_fkey (constraint) + ~ cases + - cases_id_key (constraint) + + cases_pkey (constraint) + +DDL to be executed: +-------------------------------------------------- + +ALTER TABLE case_notes DROP CONSTRAINT case_notes_case_id_fkey; + +ALTER TABLE cases DROP CONSTRAINT cases_id_key; + +ALTER TABLE cases +ADD CONSTRAINT cases_pkey PRIMARY KEY (id); + +ALTER TABLE case_notes +ADD CONSTRAINT case_notes_case_id_fkey FOREIGN KEY (case_id) REFERENCES cases (id) ON DELETE CASCADE NOT VALID; + +ALTER TABLE case_notes VALIDATE CONSTRAINT case_notes_case_id_fkey; diff --git a/testdata/diff/dependency/issue_439_replaced_unique_new_table_fk/diff.sql b/testdata/diff/dependency/issue_439_replaced_unique_new_table_fk/diff.sql new file mode 100644 index 00000000..e24549f1 --- /dev/null +++ b/testdata/diff/dependency/issue_439_replaced_unique_new_table_fk/diff.sql @@ -0,0 +1,13 @@ +CREATE TABLE IF NOT EXISTS case_notes ( + id integer, + case_id integer, + CONSTRAINT case_notes_pkey PRIMARY KEY (id) +); + +ALTER TABLE cases DROP CONSTRAINT cases_id_key; + +ALTER TABLE cases +ADD CONSTRAINT cases_pkey PRIMARY KEY (id); + +ALTER TABLE case_notes +ADD CONSTRAINT case_notes_case_id_fkey FOREIGN KEY (case_id) REFERENCES cases (id); diff --git a/testdata/diff/dependency/issue_439_replaced_unique_new_table_fk/new.sql b/testdata/diff/dependency/issue_439_replaced_unique_new_table_fk/new.sql new file mode 100644 index 00000000..14532094 --- /dev/null +++ b/testdata/diff/dependency/issue_439_replaced_unique_new_table_fk/new.sql @@ -0,0 +1,7 @@ +CREATE TABLE cases ( + id integer PRIMARY KEY +); +CREATE TABLE case_notes ( + id integer PRIMARY KEY, + case_id integer CONSTRAINT case_notes_case_id_fkey REFERENCES cases (id) +); diff --git a/testdata/diff/dependency/issue_439_replaced_unique_new_table_fk/old.sql b/testdata/diff/dependency/issue_439_replaced_unique_new_table_fk/old.sql new file mode 100644 index 00000000..622336e9 --- /dev/null +++ b/testdata/diff/dependency/issue_439_replaced_unique_new_table_fk/old.sql @@ -0,0 +1,4 @@ +CREATE TABLE cases ( + id integer NOT NULL, + CONSTRAINT cases_id_key UNIQUE (id) +); diff --git a/testdata/diff/dependency/issue_439_replaced_unique_new_table_fk/plan.json b/testdata/diff/dependency/issue_439_replaced_unique_new_table_fk/plan.json new file mode 100644 index 00000000..fde23a5c --- /dev/null +++ b/testdata/diff/dependency/issue_439_replaced_unique_new_table_fk/plan.json @@ -0,0 +1,38 @@ +{ + "version": "1.0.0", + "pgschema_version": "1.11.0", + "created_at": "1970-01-01T00:00:00Z", + "source_fingerprint": { + "hash": "15b7d3045f1565d067e24c418da5dbd8cf9abc4d8e7b643d004048481282fe26" + }, + "groups": [ + { + "steps": [ + { + "sql": "CREATE TABLE IF NOT EXISTS case_notes (\n id integer,\n case_id integer,\n CONSTRAINT case_notes_pkey PRIMARY KEY (id)\n);", + "type": "table", + "operation": "create", + "path": "public.case_notes" + }, + { + "sql": "ALTER TABLE cases DROP CONSTRAINT cases_id_key;", + "type": "table.constraint", + "operation": "drop", + "path": "public.cases.cases_id_key" + }, + { + "sql": "ALTER TABLE cases\nADD CONSTRAINT cases_pkey PRIMARY KEY (id);", + "type": "table.constraint", + "operation": "create", + "path": "public.cases.cases_pkey" + }, + { + "sql": "ALTER TABLE case_notes\nADD CONSTRAINT case_notes_case_id_fkey FOREIGN KEY (case_id) REFERENCES cases (id);", + "type": "table.constraint", + "operation": "create", + "path": "public.case_notes.case_notes_case_id_fkey" + } + ] + } + ] +} diff --git a/testdata/diff/dependency/issue_439_replaced_unique_new_table_fk/plan.sql b/testdata/diff/dependency/issue_439_replaced_unique_new_table_fk/plan.sql new file mode 100644 index 00000000..e24549f1 --- /dev/null +++ b/testdata/diff/dependency/issue_439_replaced_unique_new_table_fk/plan.sql @@ -0,0 +1,13 @@ +CREATE TABLE IF NOT EXISTS case_notes ( + id integer, + case_id integer, + CONSTRAINT case_notes_pkey PRIMARY KEY (id) +); + +ALTER TABLE cases DROP CONSTRAINT cases_id_key; + +ALTER TABLE cases +ADD CONSTRAINT cases_pkey PRIMARY KEY (id); + +ALTER TABLE case_notes +ADD CONSTRAINT case_notes_case_id_fkey FOREIGN KEY (case_id) REFERENCES cases (id); diff --git a/testdata/diff/dependency/issue_439_replaced_unique_new_table_fk/plan.txt b/testdata/diff/dependency/issue_439_replaced_unique_new_table_fk/plan.txt new file mode 100644 index 00000000..cf4010b1 --- /dev/null +++ b/testdata/diff/dependency/issue_439_replaced_unique_new_table_fk/plan.txt @@ -0,0 +1,28 @@ +Plan: 1 to add, 1 to modify. + +Summary by type: + tables: 1 to add, 1 to modify + +Tables: + + case_notes + + case_notes_case_id_fkey (constraint) + ~ cases + - cases_id_key (constraint) + + cases_pkey (constraint) + +DDL to be executed: +-------------------------------------------------- + +CREATE TABLE IF NOT EXISTS case_notes ( + id integer, + case_id integer, + CONSTRAINT case_notes_pkey PRIMARY KEY (id) +); + +ALTER TABLE cases DROP CONSTRAINT cases_id_key; + +ALTER TABLE cases +ADD CONSTRAINT cases_pkey PRIMARY KEY (id); + +ALTER TABLE case_notes +ADD CONSTRAINT case_notes_case_id_fkey FOREIGN KEY (case_id) REFERENCES cases (id); From 282722431a41ae6abed63f165631ad6351994832 Mon Sep 17 00:00:00 2001 From: tianzhou Date: Wed, 10 Jun 2026 04:07:16 -0700 Subject: [PATCH 3/5] test: merge issue 439 fixtures into a single test case Fold the changed-FK and new-table-FK scenarios into the existing issue_439_unique_to_pk_fk_dependent fixture: one UNIQUE->PK swap with four referencing tables covering an unchanged FK, an unchanged FK with ON DELETE CASCADE, an FK changed in the same migration, and a newly added table whose FK targets the replaced constraint. Co-Authored-By: Claude Fable 5 --- .../diff.sql | 9 ---- .../new.sql | 7 --- .../old.sql | 8 ---- .../plan.json | 44 ------------------- .../plan.sql | 11 ----- .../plan.txt | 27 ------------ .../diff.sql | 13 ------ .../new.sql | 7 --- .../old.sql | 4 -- .../plan.json | 38 ---------------- .../plan.sql | 13 ------ .../plan.txt | 28 ------------ .../diff.sql | 14 ++++++ .../new.sql | 14 ++++++ .../old.sql | 8 ++++ .../plan.json | 32 +++++++++++++- .../plan.sql | 16 +++++++ .../plan.txt | 25 ++++++++++- 18 files changed, 106 insertions(+), 212 deletions(-) delete mode 100644 testdata/diff/dependency/issue_439_replaced_unique_changed_fk/diff.sql delete mode 100644 testdata/diff/dependency/issue_439_replaced_unique_changed_fk/new.sql delete mode 100644 testdata/diff/dependency/issue_439_replaced_unique_changed_fk/old.sql delete mode 100644 testdata/diff/dependency/issue_439_replaced_unique_changed_fk/plan.json delete mode 100644 testdata/diff/dependency/issue_439_replaced_unique_changed_fk/plan.sql delete mode 100644 testdata/diff/dependency/issue_439_replaced_unique_changed_fk/plan.txt delete mode 100644 testdata/diff/dependency/issue_439_replaced_unique_new_table_fk/diff.sql delete mode 100644 testdata/diff/dependency/issue_439_replaced_unique_new_table_fk/new.sql delete mode 100644 testdata/diff/dependency/issue_439_replaced_unique_new_table_fk/old.sql delete mode 100644 testdata/diff/dependency/issue_439_replaced_unique_new_table_fk/plan.json delete mode 100644 testdata/diff/dependency/issue_439_replaced_unique_new_table_fk/plan.sql delete mode 100644 testdata/diff/dependency/issue_439_replaced_unique_new_table_fk/plan.txt diff --git a/testdata/diff/dependency/issue_439_replaced_unique_changed_fk/diff.sql b/testdata/diff/dependency/issue_439_replaced_unique_changed_fk/diff.sql deleted file mode 100644 index 768eead3..00000000 --- a/testdata/diff/dependency/issue_439_replaced_unique_changed_fk/diff.sql +++ /dev/null @@ -1,9 +0,0 @@ -ALTER TABLE case_notes DROP CONSTRAINT case_notes_case_id_fkey; - -ALTER TABLE cases DROP CONSTRAINT cases_id_key; - -ALTER TABLE cases -ADD CONSTRAINT cases_pkey PRIMARY KEY (id); - -ALTER TABLE case_notes -ADD CONSTRAINT case_notes_case_id_fkey FOREIGN KEY (case_id) REFERENCES cases (id) ON DELETE CASCADE; diff --git a/testdata/diff/dependency/issue_439_replaced_unique_changed_fk/new.sql b/testdata/diff/dependency/issue_439_replaced_unique_changed_fk/new.sql deleted file mode 100644 index f7476089..00000000 --- a/testdata/diff/dependency/issue_439_replaced_unique_changed_fk/new.sql +++ /dev/null @@ -1,7 +0,0 @@ -CREATE TABLE cases ( - id integer PRIMARY KEY -); -CREATE TABLE case_notes ( - id integer PRIMARY KEY, - case_id integer CONSTRAINT case_notes_case_id_fkey REFERENCES cases (id) ON DELETE CASCADE -); diff --git a/testdata/diff/dependency/issue_439_replaced_unique_changed_fk/old.sql b/testdata/diff/dependency/issue_439_replaced_unique_changed_fk/old.sql deleted file mode 100644 index 499ede0d..00000000 --- a/testdata/diff/dependency/issue_439_replaced_unique_changed_fk/old.sql +++ /dev/null @@ -1,8 +0,0 @@ -CREATE TABLE cases ( - id integer NOT NULL, - CONSTRAINT cases_id_key UNIQUE (id) -); -CREATE TABLE case_notes ( - id integer PRIMARY KEY, - case_id integer CONSTRAINT case_notes_case_id_fkey REFERENCES cases (id) -); diff --git a/testdata/diff/dependency/issue_439_replaced_unique_changed_fk/plan.json b/testdata/diff/dependency/issue_439_replaced_unique_changed_fk/plan.json deleted file mode 100644 index 19890826..00000000 --- a/testdata/diff/dependency/issue_439_replaced_unique_changed_fk/plan.json +++ /dev/null @@ -1,44 +0,0 @@ -{ - "version": "1.0.0", - "pgschema_version": "1.11.0", - "created_at": "1970-01-01T00:00:00Z", - "source_fingerprint": { - "hash": "aef1cfad9933c06a8c7e330d6c6bbcf10cd4255648eb2dcd3719758b8c373cb1" - }, - "groups": [ - { - "steps": [ - { - "sql": "ALTER TABLE case_notes DROP CONSTRAINT case_notes_case_id_fkey;", - "type": "table.constraint", - "operation": "drop", - "path": "public.case_notes.case_notes_case_id_fkey" - }, - { - "sql": "ALTER TABLE cases DROP CONSTRAINT cases_id_key;", - "type": "table.constraint", - "operation": "drop", - "path": "public.cases.cases_id_key" - }, - { - "sql": "ALTER TABLE cases\nADD CONSTRAINT cases_pkey PRIMARY KEY (id);", - "type": "table.constraint", - "operation": "create", - "path": "public.cases.cases_pkey" - }, - { - "sql": "ALTER TABLE case_notes\nADD CONSTRAINT case_notes_case_id_fkey FOREIGN KEY (case_id) REFERENCES cases (id) ON DELETE CASCADE NOT VALID;", - "type": "table.constraint", - "operation": "create", - "path": "public.case_notes.case_notes_case_id_fkey" - }, - { - "sql": "ALTER TABLE case_notes VALIDATE CONSTRAINT case_notes_case_id_fkey;", - "type": "table.constraint", - "operation": "create", - "path": "public.case_notes.case_notes_case_id_fkey" - } - ] - } - ] -} diff --git a/testdata/diff/dependency/issue_439_replaced_unique_changed_fk/plan.sql b/testdata/diff/dependency/issue_439_replaced_unique_changed_fk/plan.sql deleted file mode 100644 index 321f380d..00000000 --- a/testdata/diff/dependency/issue_439_replaced_unique_changed_fk/plan.sql +++ /dev/null @@ -1,11 +0,0 @@ -ALTER TABLE case_notes DROP CONSTRAINT case_notes_case_id_fkey; - -ALTER TABLE cases DROP CONSTRAINT cases_id_key; - -ALTER TABLE cases -ADD CONSTRAINT cases_pkey PRIMARY KEY (id); - -ALTER TABLE case_notes -ADD CONSTRAINT case_notes_case_id_fkey FOREIGN KEY (case_id) REFERENCES cases (id) ON DELETE CASCADE NOT VALID; - -ALTER TABLE case_notes VALIDATE CONSTRAINT case_notes_case_id_fkey; diff --git a/testdata/diff/dependency/issue_439_replaced_unique_changed_fk/plan.txt b/testdata/diff/dependency/issue_439_replaced_unique_changed_fk/plan.txt deleted file mode 100644 index f122c148..00000000 --- a/testdata/diff/dependency/issue_439_replaced_unique_changed_fk/plan.txt +++ /dev/null @@ -1,27 +0,0 @@ -Plan: 2 to modify. - -Summary by type: - tables: 2 to modify - -Tables: - ~ case_notes - - case_notes_case_id_fkey (constraint) - + case_notes_case_id_fkey (constraint) - ~ cases - - cases_id_key (constraint) - + cases_pkey (constraint) - -DDL to be executed: --------------------------------------------------- - -ALTER TABLE case_notes DROP CONSTRAINT case_notes_case_id_fkey; - -ALTER TABLE cases DROP CONSTRAINT cases_id_key; - -ALTER TABLE cases -ADD CONSTRAINT cases_pkey PRIMARY KEY (id); - -ALTER TABLE case_notes -ADD CONSTRAINT case_notes_case_id_fkey FOREIGN KEY (case_id) REFERENCES cases (id) ON DELETE CASCADE NOT VALID; - -ALTER TABLE case_notes VALIDATE CONSTRAINT case_notes_case_id_fkey; diff --git a/testdata/diff/dependency/issue_439_replaced_unique_new_table_fk/diff.sql b/testdata/diff/dependency/issue_439_replaced_unique_new_table_fk/diff.sql deleted file mode 100644 index e24549f1..00000000 --- a/testdata/diff/dependency/issue_439_replaced_unique_new_table_fk/diff.sql +++ /dev/null @@ -1,13 +0,0 @@ -CREATE TABLE IF NOT EXISTS case_notes ( - id integer, - case_id integer, - CONSTRAINT case_notes_pkey PRIMARY KEY (id) -); - -ALTER TABLE cases DROP CONSTRAINT cases_id_key; - -ALTER TABLE cases -ADD CONSTRAINT cases_pkey PRIMARY KEY (id); - -ALTER TABLE case_notes -ADD CONSTRAINT case_notes_case_id_fkey FOREIGN KEY (case_id) REFERENCES cases (id); diff --git a/testdata/diff/dependency/issue_439_replaced_unique_new_table_fk/new.sql b/testdata/diff/dependency/issue_439_replaced_unique_new_table_fk/new.sql deleted file mode 100644 index 14532094..00000000 --- a/testdata/diff/dependency/issue_439_replaced_unique_new_table_fk/new.sql +++ /dev/null @@ -1,7 +0,0 @@ -CREATE TABLE cases ( - id integer PRIMARY KEY -); -CREATE TABLE case_notes ( - id integer PRIMARY KEY, - case_id integer CONSTRAINT case_notes_case_id_fkey REFERENCES cases (id) -); diff --git a/testdata/diff/dependency/issue_439_replaced_unique_new_table_fk/old.sql b/testdata/diff/dependency/issue_439_replaced_unique_new_table_fk/old.sql deleted file mode 100644 index 622336e9..00000000 --- a/testdata/diff/dependency/issue_439_replaced_unique_new_table_fk/old.sql +++ /dev/null @@ -1,4 +0,0 @@ -CREATE TABLE cases ( - id integer NOT NULL, - CONSTRAINT cases_id_key UNIQUE (id) -); diff --git a/testdata/diff/dependency/issue_439_replaced_unique_new_table_fk/plan.json b/testdata/diff/dependency/issue_439_replaced_unique_new_table_fk/plan.json deleted file mode 100644 index fde23a5c..00000000 --- a/testdata/diff/dependency/issue_439_replaced_unique_new_table_fk/plan.json +++ /dev/null @@ -1,38 +0,0 @@ -{ - "version": "1.0.0", - "pgschema_version": "1.11.0", - "created_at": "1970-01-01T00:00:00Z", - "source_fingerprint": { - "hash": "15b7d3045f1565d067e24c418da5dbd8cf9abc4d8e7b643d004048481282fe26" - }, - "groups": [ - { - "steps": [ - { - "sql": "CREATE TABLE IF NOT EXISTS case_notes (\n id integer,\n case_id integer,\n CONSTRAINT case_notes_pkey PRIMARY KEY (id)\n);", - "type": "table", - "operation": "create", - "path": "public.case_notes" - }, - { - "sql": "ALTER TABLE cases DROP CONSTRAINT cases_id_key;", - "type": "table.constraint", - "operation": "drop", - "path": "public.cases.cases_id_key" - }, - { - "sql": "ALTER TABLE cases\nADD CONSTRAINT cases_pkey PRIMARY KEY (id);", - "type": "table.constraint", - "operation": "create", - "path": "public.cases.cases_pkey" - }, - { - "sql": "ALTER TABLE case_notes\nADD CONSTRAINT case_notes_case_id_fkey FOREIGN KEY (case_id) REFERENCES cases (id);", - "type": "table.constraint", - "operation": "create", - "path": "public.case_notes.case_notes_case_id_fkey" - } - ] - } - ] -} diff --git a/testdata/diff/dependency/issue_439_replaced_unique_new_table_fk/plan.sql b/testdata/diff/dependency/issue_439_replaced_unique_new_table_fk/plan.sql deleted file mode 100644 index e24549f1..00000000 --- a/testdata/diff/dependency/issue_439_replaced_unique_new_table_fk/plan.sql +++ /dev/null @@ -1,13 +0,0 @@ -CREATE TABLE IF NOT EXISTS case_notes ( - id integer, - case_id integer, - CONSTRAINT case_notes_pkey PRIMARY KEY (id) -); - -ALTER TABLE cases DROP CONSTRAINT cases_id_key; - -ALTER TABLE cases -ADD CONSTRAINT cases_pkey PRIMARY KEY (id); - -ALTER TABLE case_notes -ADD CONSTRAINT case_notes_case_id_fkey FOREIGN KEY (case_id) REFERENCES cases (id); diff --git a/testdata/diff/dependency/issue_439_replaced_unique_new_table_fk/plan.txt b/testdata/diff/dependency/issue_439_replaced_unique_new_table_fk/plan.txt deleted file mode 100644 index cf4010b1..00000000 --- a/testdata/diff/dependency/issue_439_replaced_unique_new_table_fk/plan.txt +++ /dev/null @@ -1,28 +0,0 @@ -Plan: 1 to add, 1 to modify. - -Summary by type: - tables: 1 to add, 1 to modify - -Tables: - + case_notes - + case_notes_case_id_fkey (constraint) - ~ cases - - cases_id_key (constraint) - + cases_pkey (constraint) - -DDL to be executed: --------------------------------------------------- - -CREATE TABLE IF NOT EXISTS case_notes ( - id integer, - case_id integer, - CONSTRAINT case_notes_pkey PRIMARY KEY (id) -); - -ALTER TABLE cases DROP CONSTRAINT cases_id_key; - -ALTER TABLE cases -ADD CONSTRAINT cases_pkey PRIMARY KEY (id); - -ALTER TABLE case_notes -ADD CONSTRAINT case_notes_case_id_fkey FOREIGN KEY (case_id) REFERENCES cases (id); diff --git a/testdata/diff/dependency/issue_439_unique_to_pk_fk_dependent/diff.sql b/testdata/diff/dependency/issue_439_unique_to_pk_fk_dependent/diff.sql index e38b0bdf..4c1a2d2e 100644 --- a/testdata/diff/dependency/issue_439_unique_to_pk_fk_dependent/diff.sql +++ b/testdata/diff/dependency/issue_439_unique_to_pk_fk_dependent/diff.sql @@ -1,3 +1,11 @@ +CREATE TABLE IF NOT EXISTS case_files ( + id integer, + case_id integer, + CONSTRAINT case_files_pkey PRIMARY KEY (id) +); + +ALTER TABLE case_comments DROP CONSTRAINT case_comments_case_id_fkey; + ALTER TABLE case_notes DROP CONSTRAINT case_notes_case_id_fkey; ALTER TABLE case_tags DROP CONSTRAINT case_tags_case_id_fkey; @@ -7,6 +15,12 @@ ALTER TABLE cases DROP CONSTRAINT cases_id_key; ALTER TABLE cases ADD CONSTRAINT cases_pkey PRIMARY KEY (id); +ALTER TABLE case_comments +ADD CONSTRAINT case_comments_case_id_fkey FOREIGN KEY (case_id) REFERENCES cases (id) ON DELETE CASCADE; + +ALTER TABLE case_files +ADD CONSTRAINT case_files_case_id_fkey FOREIGN KEY (case_id) REFERENCES cases (id); + ALTER TABLE case_notes ADD CONSTRAINT case_notes_case_id_fkey FOREIGN KEY (case_id) REFERENCES cases (id); diff --git a/testdata/diff/dependency/issue_439_unique_to_pk_fk_dependent/new.sql b/testdata/diff/dependency/issue_439_unique_to_pk_fk_dependent/new.sql index fbc8bccb..0b85a46d 100644 --- a/testdata/diff/dependency/issue_439_unique_to_pk_fk_dependent/new.sql +++ b/testdata/diff/dependency/issue_439_unique_to_pk_fk_dependent/new.sql @@ -3,12 +3,26 @@ CREATE TABLE cases ( name text ); +-- unchanged FK CREATE TABLE case_notes ( id integer PRIMARY KEY, case_id integer CONSTRAINT case_notes_case_id_fkey REFERENCES cases (id) ); +-- unchanged FK with ON DELETE CASCADE (clause must survive recreation) CREATE TABLE case_tags ( id integer PRIMARY KEY, case_id integer CONSTRAINT case_tags_case_id_fkey REFERENCES cases (id) ON DELETE CASCADE ); + +-- FK that itself changes in the same migration (gains ON DELETE CASCADE) +CREATE TABLE case_comments ( + id integer PRIMARY KEY, + case_id integer CONSTRAINT case_comments_case_id_fkey REFERENCES cases (id) ON DELETE CASCADE +); + +-- newly added table whose FK references the replaced constraint +CREATE TABLE case_files ( + id integer PRIMARY KEY, + case_id integer CONSTRAINT case_files_case_id_fkey REFERENCES cases (id) +); diff --git a/testdata/diff/dependency/issue_439_unique_to_pk_fk_dependent/old.sql b/testdata/diff/dependency/issue_439_unique_to_pk_fk_dependent/old.sql index d1428781..1b408bfd 100644 --- a/testdata/diff/dependency/issue_439_unique_to_pk_fk_dependent/old.sql +++ b/testdata/diff/dependency/issue_439_unique_to_pk_fk_dependent/old.sql @@ -4,12 +4,20 @@ CREATE TABLE cases ( CONSTRAINT cases_id_key UNIQUE (id) ); +-- unchanged FK CREATE TABLE case_notes ( id integer PRIMARY KEY, case_id integer CONSTRAINT case_notes_case_id_fkey REFERENCES cases (id) ); +-- unchanged FK with ON DELETE CASCADE (clause must survive recreation) CREATE TABLE case_tags ( id integer PRIMARY KEY, case_id integer CONSTRAINT case_tags_case_id_fkey REFERENCES cases (id) ON DELETE CASCADE ); + +-- FK that itself changes in the same migration (gains ON DELETE CASCADE) +CREATE TABLE case_comments ( + id integer PRIMARY KEY, + case_id integer CONSTRAINT case_comments_case_id_fkey REFERENCES cases (id) +); diff --git a/testdata/diff/dependency/issue_439_unique_to_pk_fk_dependent/plan.json b/testdata/diff/dependency/issue_439_unique_to_pk_fk_dependent/plan.json index 3a128b1d..9c3a5406 100644 --- a/testdata/diff/dependency/issue_439_unique_to_pk_fk_dependent/plan.json +++ b/testdata/diff/dependency/issue_439_unique_to_pk_fk_dependent/plan.json @@ -3,11 +3,23 @@ "pgschema_version": "1.11.0", "created_at": "1970-01-01T00:00:00Z", "source_fingerprint": { - "hash": "67b8ade4e7f5a944045178db3d613964042ecb7e1deb5f497db25c5251d3c430" + "hash": "d3de037447ad5303358cf43db984d109e25c0672dd39bab2b072a0ab6b4a075a" }, "groups": [ { "steps": [ + { + "sql": "CREATE TABLE IF NOT EXISTS case_files (\n id integer,\n case_id integer,\n CONSTRAINT case_files_pkey PRIMARY KEY (id)\n);", + "type": "table", + "operation": "create", + "path": "public.case_files" + }, + { + "sql": "ALTER TABLE case_comments DROP CONSTRAINT case_comments_case_id_fkey;", + "type": "table.constraint", + "operation": "drop", + "path": "public.case_comments.case_comments_case_id_fkey" + }, { "sql": "ALTER TABLE case_notes DROP CONSTRAINT case_notes_case_id_fkey;", "type": "table.constraint", @@ -32,6 +44,24 @@ "operation": "create", "path": "public.cases.cases_pkey" }, + { + "sql": "ALTER TABLE case_comments\nADD CONSTRAINT case_comments_case_id_fkey FOREIGN KEY (case_id) REFERENCES cases (id) ON DELETE CASCADE NOT VALID;", + "type": "table.constraint", + "operation": "create", + "path": "public.case_comments.case_comments_case_id_fkey" + }, + { + "sql": "ALTER TABLE case_comments VALIDATE CONSTRAINT case_comments_case_id_fkey;", + "type": "table.constraint", + "operation": "create", + "path": "public.case_comments.case_comments_case_id_fkey" + }, + { + "sql": "ALTER TABLE case_files\nADD CONSTRAINT case_files_case_id_fkey FOREIGN KEY (case_id) REFERENCES cases (id);", + "type": "table.constraint", + "operation": "create", + "path": "public.case_files.case_files_case_id_fkey" + }, { "sql": "ALTER TABLE case_notes\nADD CONSTRAINT case_notes_case_id_fkey FOREIGN KEY (case_id) REFERENCES cases (id) NOT VALID;", "type": "table.constraint", diff --git a/testdata/diff/dependency/issue_439_unique_to_pk_fk_dependent/plan.sql b/testdata/diff/dependency/issue_439_unique_to_pk_fk_dependent/plan.sql index 8ebbff84..6b5a1fb0 100644 --- a/testdata/diff/dependency/issue_439_unique_to_pk_fk_dependent/plan.sql +++ b/testdata/diff/dependency/issue_439_unique_to_pk_fk_dependent/plan.sql @@ -1,3 +1,11 @@ +CREATE TABLE IF NOT EXISTS case_files ( + id integer, + case_id integer, + CONSTRAINT case_files_pkey PRIMARY KEY (id) +); + +ALTER TABLE case_comments DROP CONSTRAINT case_comments_case_id_fkey; + ALTER TABLE case_notes DROP CONSTRAINT case_notes_case_id_fkey; ALTER TABLE case_tags DROP CONSTRAINT case_tags_case_id_fkey; @@ -7,6 +15,14 @@ ALTER TABLE cases DROP CONSTRAINT cases_id_key; ALTER TABLE cases ADD CONSTRAINT cases_pkey PRIMARY KEY (id); +ALTER TABLE case_comments +ADD CONSTRAINT case_comments_case_id_fkey FOREIGN KEY (case_id) REFERENCES cases (id) ON DELETE CASCADE NOT VALID; + +ALTER TABLE case_comments VALIDATE CONSTRAINT case_comments_case_id_fkey; + +ALTER TABLE case_files +ADD CONSTRAINT case_files_case_id_fkey FOREIGN KEY (case_id) REFERENCES cases (id); + ALTER TABLE case_notes ADD CONSTRAINT case_notes_case_id_fkey FOREIGN KEY (case_id) REFERENCES cases (id) NOT VALID; diff --git a/testdata/diff/dependency/issue_439_unique_to_pk_fk_dependent/plan.txt b/testdata/diff/dependency/issue_439_unique_to_pk_fk_dependent/plan.txt index 42808842..c7fff552 100644 --- a/testdata/diff/dependency/issue_439_unique_to_pk_fk_dependent/plan.txt +++ b/testdata/diff/dependency/issue_439_unique_to_pk_fk_dependent/plan.txt @@ -1,9 +1,14 @@ -Plan: 3 to modify. +Plan: 1 to add, 4 to modify. Summary by type: - tables: 3 to modify + tables: 1 to add, 4 to modify Tables: + ~ case_comments + - case_comments_case_id_fkey (constraint) + + case_comments_case_id_fkey (constraint) + + case_files + + case_files_case_id_fkey (constraint) ~ case_notes - case_notes_case_id_fkey (constraint) + case_notes_case_id_fkey (constraint) @@ -17,6 +22,14 @@ Tables: DDL to be executed: -------------------------------------------------- +CREATE TABLE IF NOT EXISTS case_files ( + id integer, + case_id integer, + CONSTRAINT case_files_pkey PRIMARY KEY (id) +); + +ALTER TABLE case_comments DROP CONSTRAINT case_comments_case_id_fkey; + ALTER TABLE case_notes DROP CONSTRAINT case_notes_case_id_fkey; ALTER TABLE case_tags DROP CONSTRAINT case_tags_case_id_fkey; @@ -26,6 +39,14 @@ ALTER TABLE cases DROP CONSTRAINT cases_id_key; ALTER TABLE cases ADD CONSTRAINT cases_pkey PRIMARY KEY (id); +ALTER TABLE case_comments +ADD CONSTRAINT case_comments_case_id_fkey FOREIGN KEY (case_id) REFERENCES cases (id) ON DELETE CASCADE NOT VALID; + +ALTER TABLE case_comments VALIDATE CONSTRAINT case_comments_case_id_fkey; + +ALTER TABLE case_files +ADD CONSTRAINT case_files_case_id_fkey FOREIGN KEY (case_id) REFERENCES cases (id); + ALTER TABLE case_notes ADD CONSTRAINT case_notes_case_id_fkey FOREIGN KEY (case_id) REFERENCES cases (id) NOT VALID; From ab80c02d4dc75fa03d64e982171397129c4c16c9 Mon Sep 17 00:00:00 2001 From: tianzhou Date: Wed, 10 Jun 2026 04:33:43 -0700 Subject: [PATCH 4/5] refactor: simplify FK recreation for replaced constraints Apply cleanup-review findings to the issue 439 change: - Reuse generateDeferredConstraintsSQL for the post-add emission instead of the near-identical generateAddRecreatedFKsSQL; fkPostAdds is now a []*deferredConstraint, removing a duplicated copy of the FK ADD format. - Replace the post-sort mutation of tableDiff DroppedConstraints and ModifiedConstraints with a preDroppedFKSet skip-set consulted at emission time in generateAlterTableStatements, matching the existing skip-set pattern (constraintDroppedWithColumns, droppedTableSet, preDroppedViews) and deleting the bespoke list-rewrite helpers. - Share constraint identity keys via constraintPathKey. Generated plans are unchanged; all fixtures pass without regeneration. Co-Authored-By: Claude Fable 5 --- internal/diff/diff.go | 6 +- internal/diff/table.go | 127 ++++++++++++++--------------------------- 2 files changed, 46 insertions(+), 87 deletions(-) diff --git a/internal/diff/diff.go b/internal/diff/diff.go index 5b1b2dad..b5c1f3a8 100644 --- a/internal/diff/diff.go +++ b/internal/diff/diff.go @@ -316,7 +316,7 @@ type ddlDiff struct { // old constraint are kept out of CREATE TABLE (suppressedInlineFKs) and // created via fkPostAdds instead (issue #439). fkPreDrops []*ir.Constraint - fkPostAdds []*ir.Constraint + fkPostAdds []*deferredConstraint suppressedInlineFKs map[string]bool } @@ -1930,10 +1930,10 @@ func (d *ddlDiff) generateModifySQL(targetSchema string, collector *diffCollecto generateDropRecreatedFKsSQL(d.fkPreDrops, targetSchema, collector) // Modify tables - generateModifyTablesSQL(d.modifiedTables, d.droppedTables, targetSchema, collector) + generateModifyTablesSQL(d.modifiedTables, d.droppedTables, d.fkPreDrops, targetSchema, collector) // (Re)create the dependent foreign keys now that the replacement constraints exist - generateAddRecreatedFKsSQL(d.fkPostAdds, targetSchema, collector) + generateDeferredConstraintsSQL(d.fkPostAdds, targetSchema, collector) // Create views deferred from generateCreateSQL — their bodies reference // columns just added by ALTER TABLE above (issue #414). Likewise, emit diff --git a/internal/diff/table.go b/internal/diff/table.go index c1771431..a3649d4b 100644 --- a/internal/diff/table.go +++ b/internal/diff/table.go @@ -542,13 +542,20 @@ func generateDeferredConstraintsSQL(deferred []*deferredConstraint, targetSchema } // generateModifyTablesSQL generates ALTER TABLE statements -func generateModifyTablesSQL(diffs []*tableDiff, droppedTables []*ir.Table, targetSchema string, collector *diffCollector) { +func generateModifyTablesSQL(diffs []*tableDiff, droppedTables []*ir.Table, fkPreDrops []*ir.Constraint, 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 { droppedTableSet[t.Schema+"."+t.Name] = true } + // Build a set of FKs already dropped in the pre-drop step because they were + // bound to a unique/PK constraint being replaced (#439) + preDroppedFKSet := make(map[string]bool, len(fkPreDrops)) + for _, fk := range fkPreDrops { + preDroppedFKSet[constraintPathKey(fk)] = true + } + // Diffs are already sorted by the Diff operation for _, diff := range diffs { // Build a set of columns being dropped (DROP COLUMN will remove dependent constraints) @@ -558,7 +565,7 @@ func generateModifyTablesSQL(diffs []*tableDiff, droppedTables []*ir.Table, targ } // Pass collector to generateAlterTableStatements to collect with proper context - diff.generateAlterTableStatements(targetSchema, collector, droppedTableSet, droppedColumnSet) + diff.generateAlterTableStatements(targetSchema, collector, droppedTableSet, droppedColumnSet, preDroppedFKSet) } } @@ -570,8 +577,9 @@ func generateModifyTablesSQL(diffs []*tableDiff, droppedTables []*ir.Table, targ // // It returns: // - preDrops: old-state FKs bound to a replaced constraint, to drop before -// the table modifications. Their drop/modify entries are removed from -// their own table diff so they are not dropped twice. +// the table modifications. The drop/modify entries they cover are skipped +// at emission time (see generateAlterTableStatements) so they are not +// dropped twice. // - postAdds: desired-state FKs to (re)create after the table modifications, // once the replacement constraint exists. // - suppressedInlineFKs: FKs on newly added tables (keyed schema.table.name) @@ -582,13 +590,11 @@ func generateModifyTablesSQL(diffs []*tableDiff, droppedTables []*ir.Table, targ // Note: Postgres matches an FK's referenced columns to a unique constraint as // a column set, not an ordered list — FOREIGN KEY (x, y) REFERENCES t (b, a) // is valid against UNIQUE (a, b) — so matching here is order-insensitive. -func planFKRecreationForReplacedConstraints(modifiedTables []*tableDiff, addedTables []*ir.Table, oldTables, newTables map[string]*ir.Table) (preDrops, postAdds []*ir.Constraint, suppressedInlineFKs map[string]bool) { +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) - tableDiffByKey := make(map[string]*tableDiff, len(modifiedTables)) for _, td := range modifiedTables { key := td.Table.Schema + "." + td.Table.Name - tableDiffByKey[key] = td for _, c := range td.DroppedConstraints { if c.Type == ir.ConstraintTypeUnique || c.Type == ir.ConstraintTypePrimaryKey { replaced[key] = append(replaced[key], c) @@ -612,7 +618,6 @@ func planFKRecreationForReplacedConstraints(modifiedTables []*tableDiff, addedTa if newTable == nil { continue // table is dropped before the modify phase; its FKs go with it } - td := tableDiffByKey[tableKey] for _, name := range sortedKeys(oldTable.Constraints) { fk := oldTable.Constraints[name] if fk.Type != ir.ConstraintTypeForeignKey { @@ -630,14 +635,8 @@ func planFKRecreationForReplacedConstraints(modifiedTables []*tableDiff, addedTa } preDrops = append(preDrops, fk) - if td != nil { - // The FK is now dropped in the pre-drop step; remove it from its - // own table diff so it is not dropped twice or re-added too early. - td.DroppedConstraints = removeConstraintByName(td.DroppedConstraints, name) - td.ModifiedConstraints = removeConstraintDiffByName(td.ModifiedConstraints, name) - } if newFK != nil { - postAdds = append(postAdds, newFK) + postAdds = append(postAdds, &deferredConstraint{table: newTable, constraint: newFK}) } } } @@ -655,16 +654,25 @@ func planFKRecreationForReplacedConstraints(modifiedTables []*tableDiff, addedTa if !fkReferencesAnyConstraint(fk, replaced[fkReferencedTableKey(fk)]) { continue } - suppressedInlineFKs[table.Schema+"."+table.Name+"."+name] = true - postAdds = append(postAdds, fk) + suppressedInlineFKs[constraintPathKey(fk)] = true + postAdds = append(postAdds, &deferredConstraint{table: table, constraint: fk}) } } - sortConstraintsByPath(preDrops) - sortConstraintsByPath(postAdds) + sort.Slice(preDrops, func(i, j int) bool { + return constraintPathKey(preDrops[i]) < constraintPathKey(preDrops[j]) + }) + sort.Slice(postAdds, func(i, j int) bool { + return constraintPathKey(postAdds[i].constraint) < constraintPathKey(postAdds[j].constraint) + }) return preDrops, postAdds, suppressedInlineFKs } +// constraintPathKey returns the schema-qualified identity of a constraint. +func constraintPathKey(c *ir.Constraint) string { + return c.Schema + "." + c.Table + "." + c.Name +} + // fkReferencedTableKey returns the schema-qualified key of the table an FK references. func fkReferencedTableKey(fk *ir.Constraint) string { refSchema := fk.ReferencedSchema @@ -699,39 +707,6 @@ func fkReferencesAnyConstraint(fk *ir.Constraint, candidates []*ir.Constraint) b return false } -func removeConstraintByName(list []*ir.Constraint, name string) []*ir.Constraint { - result := list[:0] - for _, c := range list { - if c.Name != name { - result = append(result, c) - } - } - return result -} - -func removeConstraintDiffByName(list []*ConstraintDiff, name string) []*ConstraintDiff { - result := list[:0] - for _, cd := range list { - if cd.Old.Name != name { - result = append(result, cd) - } - } - return result -} - -func sortConstraintsByPath(constraints []*ir.Constraint) { - sort.Slice(constraints, func(i, j int) bool { - a, b := constraints[i], constraints[j] - if a.Schema != b.Schema { - return a.Schema < b.Schema - } - if a.Table != b.Table { - return a.Table < b.Table - } - return a.Name < b.Name - }) -} - // generateDropRecreatedFKsSQL drops foreign keys bound to unique/PK constraints // being replaced. Must run before the table modifications. (#439) func generateDropRecreatedFKsSQL(fks []*ir.Constraint, targetSchema string, collector *diffCollector) { @@ -750,36 +725,6 @@ func generateDropRecreatedFKsSQL(fks []*ir.Constraint, targetSchema string, coll } } -// generateAddRecreatedFKsSQL recreates the foreign keys dropped by -// generateDropRecreatedFKsSQL, after the replacement constraints exist. (#439) -func generateAddRecreatedFKsSQL(fks []*ir.Constraint, targetSchema string, collector *diffCollector) { - for _, fk := range fks { - columns := sortConstraintColumnsByPosition(fk.Columns) - var columnNames []string - for _, col := range columns { - columnNames = append(columnNames, ir.QuoteIdentifier(col.Name)) - } - if fk.IsTemporal && len(columnNames) > 0 { - columnNames[len(columnNames)-1] = "PERIOD " + columnNames[len(columnNames)-1] - } - - tableName := getTableNameWithSchema(fk.Schema, fk.Table, targetSchema) - sql := fmt.Sprintf("ALTER TABLE %s\nADD CONSTRAINT %s FOREIGN KEY (%s) %s;", - tableName, ir.QuoteIdentifier(fk.Name), - strings.Join(columnNames, ", "), - generateForeignKeyClause(fk, targetSchema, false)) - - context := &diffContext{ - Type: DiffTypeTableConstraint, - Operation: DiffOperationCreate, - Path: fmt.Sprintf("%s.%s.%s", fk.Schema, fk.Table, fk.Name), - Source: fk, - CanRunInTransaction: true, - } - collector.collect(context, sql) - } -} - // generateDropTablesSQL generates DROP TABLE statements // Tables are assumed to be pre-sorted in reverse topological order for dependency-aware dropping func generateDropTablesSQL(tables []*ir.Table, targetSchema string, collector *diffCollector) { @@ -839,7 +784,7 @@ func generateTableSQL(table *ir.Table, targetSchema string, createdTables map[st for _, constraint := range inlineConstraints { // FKs bound to a unique/PK constraint being replaced by this migration // are created after the replacement instead (issue #439) - if suppressedInlineFKs[currentKey+"."+constraint.Name] { + if suppressedInlineFKs[constraintPathKey(constraint)] { continue } if shouldDeferConstraint(table, constraint, currentKey, createdTables, existingTables) { @@ -920,7 +865,10 @@ func constraintDroppedWithColumns(constraint *ir.Constraint, droppedColumnSet ma // FK constraints referencing these tables are skipped since CASCADE already removes them. // droppedColumnSet contains column names being dropped from this table; constraints that // depend on those columns are skipped because DROP COLUMN already removes them. (#384) -func (td *tableDiff) generateAlterTableStatements(targetSchema string, collector *diffCollector, droppedTableSet map[string]bool, droppedColumnSet map[string]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) { // 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 @@ -949,6 +897,11 @@ func (td *tableDiff) generateAlterTableStatements(targetSchema string, collector continue } + // Skip FKs already dropped in the pre-drop step. (#439) + if preDroppedFKSet[constraintPathKey(constraint)] { + continue + } + // Skip FK constraints whose referenced table is being dropped with CASCADE, // since the CASCADE will already remove the constraint. (#382) if constraint.Type == ir.ConstraintTypeForeignKey && constraint.ReferencedTable != "" { @@ -1237,6 +1190,12 @@ func (td *tableDiff) generateAlterTableStatements(targetSchema string, collector // Handle modified constraints - drop and recreate them as separate operations for _, ConstraintDiff := range td.ModifiedConstraints { + // Skip FKs already dropped in the pre-drop step; the post-add step + // recreates them from the desired-state definition. (#439) + if preDroppedFKSet[constraintPathKey(ConstraintDiff.Old)] { + continue + } + tableName := getTableNameWithSchema(td.Table.Schema, td.Table.Name, targetSchema) constraint := ConstraintDiff.New From b8bcb3140487f2817c92be11f279dbfca2886411 Mon Sep 17 00:00:00 2001 From: Tianzhou Date: Wed, 10 Jun 2026 04:42:21 -0700 Subject: [PATCH 5/5] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- internal/diff/table.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/internal/diff/table.go b/internal/diff/table.go index a3649d4b..2b94b5e4 100644 --- a/internal/diff/table.go +++ b/internal/diff/table.go @@ -582,10 +582,10 @@ func generateModifyTablesSQL(diffs []*tableDiff, droppedTables []*ir.Table, fkPr // dropped twice. // - postAdds: desired-state FKs to (re)create after the table modifications, // once the replacement constraint exists. -// - suppressedInlineFKs: FKs on newly added tables (keyed schema.table.name) -// that must be left out of CREATE TABLE — emitted inline they would bind -// to the old constraint before the replacement runs. They are recreated -// via postAdds instead. +// - suppressedInlineFKs: FKs on newly added tables (keyed by +// schema.table.constraint) that must be left out of CREATE TABLE — emitted +// inline they would bind to the old constraint before the replacement runs. +// They are recreated via postAdds instead. // // Note: Postgres matches an FK's referenced columns to a unique constraint as // a column set, not an ordered list — FOREIGN KEY (x, y) REFERENCES t (b, a)