From 58495816d10a59da532b0236b8787a2004ed4dfb Mon Sep 17 00:00:00 2001 From: Tianzhou Date: Fri, 16 Jan 2026 09:07:00 -0800 Subject: [PATCH 1/2] fix: topological sort for modified tables' constraint dependencies (#248) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Modified tables are now sorted topologically based on constraint dependencies, ensuring that UNIQUE/PK constraints are added before foreign keys that reference them. Previously, modified tables were sorted only alphabetically, which caused failures when a table with a newly-added FK came before the table with the newly-added UNIQUE constraint it references (e.g., a_employees before z_companies). This fix implements topologicallySortModifiedTables() which: - Detects when table A adds FK → table B's newly-added UNIQUE/PK - Creates dependency edge: B → A (B must be processed first) - Uses Kahn's algorithm for topological sort - Falls back to alphabetical for cycle breaking Enhanced testdata/diff/online/add_fk/ to expose and verify the fix: - Renamed tables to z_companies and a_employees (reverse alphabetical) - Added scenario testing FK to newly-added UNIQUE constraint - Added comments explaining the test purpose Fixes issue #248: "ERROR: there is no unique constraint matching given keys for referenced table" Co-Authored-By: Claude Opus 4.5 --- internal/diff/diff.go | 5 +- internal/diff/topological.go | 142 ++++++++++++++++++++++++++ testdata/diff/online/add_fk/diff.sql | 10 +- testdata/diff/online/add_fk/new.sql | 23 ++++- testdata/diff/online/add_fk/old.sql | 18 +++- testdata/diff/online/add_fk/plan.json | 28 ++++- testdata/diff/online/add_fk/plan.sql | 14 ++- testdata/diff/online/add_fk/plan.txt | 25 +++-- 8 files changed, 238 insertions(+), 27 deletions(-) diff --git a/internal/diff/diff.go b/internal/diff/diff.go index 07c05465..d8bc5fa9 100644 --- a/internal/diff/diff.go +++ b/internal/diff/diff.go @@ -1272,9 +1272,10 @@ func GenerateMigration(oldIR, newIR *ir.IR, targetSchema string) []Diff { diff.addedViews = topologicallySortViews(diff.addedViews) diff.droppedViews = reverseSlice(topologicallySortViews(diff.droppedViews)) - // Sort ModifiedTables alphabetically for consistent ordering - // (topological sorting isn't needed for modified tables since they already exist) + // Sort ModifiedTables topologically based on constraint dependencies + // This ensures that UNIQUE/PK constraints are added before FKs that reference them sortModifiedTables(diff.modifiedTables) + diff.modifiedTables = topologicallySortModifiedTables(diff.modifiedTables) // Sort individual table objects (indexes, triggers, policies, constraints) within each table sortTableObjects(diff.modifiedTables) diff --git a/internal/diff/topological.go b/internal/diff/topological.go index 3664eb57..1cb9083c 100644 --- a/internal/diff/topological.go +++ b/internal/diff/topological.go @@ -510,3 +510,145 @@ func topologicallySortFunctions(functions []*ir.Function) []*ir.Function { return sortedFunctions } + +// topologicallySortModifiedTables sorts modified tables based on constraint dependencies +// Tables with added UNIQUE/PK constraints that are referenced by other tables' added FKs +// will come before those tables +func topologicallySortModifiedTables(tableDiffs []*tableDiff) []*tableDiff { + if len(tableDiffs) <= 1 { + return tableDiffs + } + + // Build maps for efficient lookup + tableDiffMap := make(map[string]*tableDiff) + var insertionOrder []string + for _, td := range tableDiffs { + key := td.Table.Schema + "." + td.Table.Name + tableDiffMap[key] = td + insertionOrder = append(insertionOrder, key) + } + + // Build dependency graph based on added constraints + inDegree := make(map[string]int) + adjList := make(map[string][]string) + + // Initialize + for key := range tableDiffMap { + inDegree[key] = 0 + adjList[key] = []string{} + } + + // Build edges: if tableA adds a FK to tableB's newly-added UNIQUE/PK, add edge tableB -> tableA + for keyA, tdA := range tableDiffMap { + // Look at FK constraints being added to tableA + for _, fkConstraint := range tdA.AddedConstraints { + if fkConstraint.Type != ir.ConstraintTypeForeignKey { + continue + } + + // Build referenced table key + referencedSchema := fkConstraint.ReferencedSchema + if referencedSchema == "" { + referencedSchema = tdA.Table.Schema + } + keyB := referencedSchema + "." + fkConstraint.ReferencedTable + + // Check if referenced table exists in our modified tables set + tdB, exists := tableDiffMap[keyB] + if !exists || keyA == keyB { + continue + } + + // Check if tableB is adding a UNIQUE or PK constraint that matches the FK reference + for _, constraint := range tdB.AddedConstraints { + if constraint.Type != ir.ConstraintTypeUnique && constraint.Type != ir.ConstraintTypePrimaryKey { + continue + } + + // Check if this constraint matches the FK's referenced columns + if constraintMatchesFKReference(constraint, fkConstraint) { + // Add edge: tableB (with new UNIQUE/PK) -> tableA (with new FK) + adjList[keyB] = append(adjList[keyB], keyA) + inDegree[keyA]++ + break + } + } + } + } + + // Kahn's algorithm with deterministic cycle breaking + var queue []string + var result []string + processed := make(map[string]bool, len(tableDiffMap)) + + // Seed queue with nodes that have no incoming edges + for key, degree := range inDegree { + if degree == 0 { + queue = append(queue, key) + } + } + sort.Strings(queue) + + for len(result) < len(tableDiffMap) { + if len(queue) == 0 { + // Cycle detected: pick the next unprocessed table using original insertion order + next := nextInOrder(insertionOrder, processed) + if next == "" { + break + } + queue = append(queue, next) + inDegree[next] = 0 + } + + current := queue[0] + queue = queue[1:] + if processed[current] { + continue + } + processed[current] = true + result = append(result, current) + + neighbors := append([]string(nil), adjList[current]...) + sort.Strings(neighbors) + + for _, neighbor := range neighbors { + inDegree[neighbor]-- + if inDegree[neighbor] <= 0 && !processed[neighbor] { + queue = append(queue, neighbor) + sort.Strings(queue) + } + } + } + + // Convert result back to tableDiff slice + sortedTableDiffs := make([]*tableDiff, 0, len(result)) + for _, key := range result { + sortedTableDiffs = append(sortedTableDiffs, tableDiffMap[key]) + } + + return sortedTableDiffs +} + +// constraintMatchesFKReference checks if a UNIQUE/PK constraint matches the columns +// referenced by a foreign key constraint +func constraintMatchesFKReference(uniqueConstraint, fkConstraint *ir.Constraint) bool { + // Must have same number of columns + if len(uniqueConstraint.Columns) != len(fkConstraint.ReferencedColumns) { + return false + } + + // Build a set of referenced column names + refColNames := make(map[string]bool) + for _, col := range fkConstraint.ReferencedColumns { + refColNames[col.Name] = true + } + + // Check if all unique constraint columns are in the referenced columns + for _, col := range uniqueConstraint.Columns { + if !refColNames[col.Name] { + return false + } + } + + return true +} diff --git a/testdata/diff/online/add_fk/diff.sql b/testdata/diff/online/add_fk/diff.sql index 8cc58392..172850a5 100644 --- a/testdata/diff/online/add_fk/diff.sql +++ b/testdata/diff/online/add_fk/diff.sql @@ -1,2 +1,8 @@ -ALTER TABLE employees -ADD CONSTRAINT employees_company_fkey FOREIGN KEY (tenant_id, company_id) REFERENCES companies (tenant_id, company_id) ON UPDATE CASCADE ON DELETE RESTRICT DEFERRABLE; +ALTER TABLE z_companies +ADD CONSTRAINT z_companies_company_id_name_key UNIQUE (company_id, company_name); + +ALTER TABLE a_employees +ADD CONSTRAINT a_employees_company_fkey FOREIGN KEY (tenant_id, company_id) REFERENCES z_companies (tenant_id, company_id) ON UPDATE CASCADE ON DELETE RESTRICT DEFERRABLE; + +ALTER TABLE a_employees +ADD CONSTRAINT a_employees_company_name_fkey FOREIGN KEY (company_id, company_name) REFERENCES z_companies (company_id, company_name) ON UPDATE CASCADE ON DELETE RESTRICT DEFERRABLE; diff --git a/testdata/diff/online/add_fk/new.sql b/testdata/diff/online/add_fk/new.sql index e4c89ec0..308b1239 100644 --- a/testdata/diff/online/add_fk/new.sql +++ b/testdata/diff/online/add_fk/new.sql @@ -1,16 +1,31 @@ -CREATE TABLE public.companies ( +-- Test Case: Adding foreign keys with NOT VALID (online migration) +-- Scenario 1: FK referencing existing constraint (z_companies_pkey) +-- Scenario 2: FK referencing newly-added constraint (tests cross-table constraint ordering bug #248) +-- +-- Table names are intentionally chosen to test cross-table constraint ordering: +-- - z_companies (referenced table) comes AFTER a_employees alphabetically +-- - Without proper topological sorting, a_employees FK would be added before z_companies UNIQUE constraint +-- - This reproduces bug #248: "ERROR: there is no unique constraint matching given keys for referenced table" + +CREATE TABLE public.z_companies ( tenant_id integer NOT NULL, company_id integer NOT NULL, company_name text NOT NULL, - CONSTRAINT companies_pkey PRIMARY KEY (tenant_id, company_id) + CONSTRAINT z_companies_pkey PRIMARY KEY (tenant_id, company_id), + -- New UNIQUE constraint added in same migration as FK that references it + CONSTRAINT z_companies_company_id_name_key UNIQUE (company_id, company_name) ); -CREATE TABLE public.employees ( +CREATE TABLE public.a_employees ( id integer NOT NULL, employee_number text NOT NULL, first_name text NOT NULL, last_name text NOT NULL, tenant_id integer NOT NULL, company_id integer NOT NULL, - CONSTRAINT employees_company_fkey FOREIGN KEY (tenant_id, company_id) REFERENCES public.companies(tenant_id, company_id) ON UPDATE CASCADE ON DELETE RESTRICT DEFERRABLE + company_name text NOT NULL, + -- Scenario 1: FK to existing PK (no cross-table dependency issue) + CONSTRAINT a_employees_company_fkey FOREIGN KEY (tenant_id, company_id) REFERENCES public.z_companies(tenant_id, company_id) ON UPDATE CASCADE ON DELETE RESTRICT DEFERRABLE, + -- Scenario 2: FK to newly-added UNIQUE constraint (requires correct ordering: UNIQUE before VALIDATE) + CONSTRAINT a_employees_company_name_fkey FOREIGN KEY (company_id, company_name) REFERENCES public.z_companies(company_id, company_name) ON UPDATE CASCADE ON DELETE RESTRICT DEFERRABLE ); \ No newline at end of file diff --git a/testdata/diff/online/add_fk/old.sql b/testdata/diff/online/add_fk/old.sql index 09cf34a7..a80bb4f0 100644 --- a/testdata/diff/online/add_fk/old.sql +++ b/testdata/diff/online/add_fk/old.sql @@ -1,15 +1,25 @@ -CREATE TABLE public.companies ( +-- Test Case: Adding foreign keys with NOT VALID (online migration) +-- Scenario 1: FK referencing existing constraint (z_companies_pkey) +-- Scenario 2: FK referencing newly-added constraint (tests cross-table constraint ordering bug #248) +-- +-- Table names are intentionally chosen to test cross-table constraint ordering: +-- - z_companies (referenced table) comes AFTER a_employees alphabetically +-- - Without proper topological sorting, a_employees FK would be added before z_companies UNIQUE constraint +-- - This reproduces bug #248: "ERROR: there is no unique constraint matching given keys for referenced table" + +CREATE TABLE public.z_companies ( tenant_id integer NOT NULL, company_id integer NOT NULL, company_name text NOT NULL, - CONSTRAINT companies_pkey PRIMARY KEY (tenant_id, company_id) + CONSTRAINT z_companies_pkey PRIMARY KEY (tenant_id, company_id) ); -CREATE TABLE public.employees ( +CREATE TABLE public.a_employees ( id integer NOT NULL, employee_number text NOT NULL, first_name text NOT NULL, last_name text NOT NULL, tenant_id integer NOT NULL, - company_id integer NOT NULL + company_id integer NOT NULL, + company_name text NOT NULL ); \ No newline at end of file diff --git a/testdata/diff/online/add_fk/plan.json b/testdata/diff/online/add_fk/plan.json index 020efd00..17be8a14 100644 --- a/testdata/diff/online/add_fk/plan.json +++ b/testdata/diff/online/add_fk/plan.json @@ -3,22 +3,40 @@ "pgschema_version": "1.6.1", "created_at": "1970-01-01T00:00:00Z", "source_fingerprint": { - "hash": "1ccd17ca4d799e41166388913d207aaa3c5f76ce563867c094967903060fbd42" + "hash": "d694b29e56c5eee996f8ea5dc58525a625b4c7d0e18196578ca27fe2aa390542" }, "groups": [ { "steps": [ { - "sql": "ALTER TABLE employees\nADD CONSTRAINT employees_company_fkey FOREIGN KEY (tenant_id, company_id) REFERENCES companies (tenant_id, company_id) ON UPDATE CASCADE ON DELETE RESTRICT DEFERRABLE NOT VALID;", + "sql": "ALTER TABLE z_companies\nADD CONSTRAINT z_companies_company_id_name_key UNIQUE (company_id, company_name);", "type": "table.constraint", "operation": "create", - "path": "public.employees.employees_company_fkey" + "path": "public.z_companies.z_companies_company_id_name_key" }, { - "sql": "ALTER TABLE employees VALIDATE CONSTRAINT employees_company_fkey;", + "sql": "ALTER TABLE a_employees\nADD CONSTRAINT a_employees_company_fkey FOREIGN KEY (tenant_id, company_id) REFERENCES z_companies (tenant_id, company_id) ON UPDATE CASCADE ON DELETE RESTRICT DEFERRABLE NOT VALID;", "type": "table.constraint", "operation": "create", - "path": "public.employees.employees_company_fkey" + "path": "public.a_employees.a_employees_company_fkey" + }, + { + "sql": "ALTER TABLE a_employees VALIDATE CONSTRAINT a_employees_company_fkey;", + "type": "table.constraint", + "operation": "create", + "path": "public.a_employees.a_employees_company_fkey" + }, + { + "sql": "ALTER TABLE a_employees\nADD CONSTRAINT a_employees_company_name_fkey FOREIGN KEY (company_id, company_name) REFERENCES z_companies (company_id, company_name) ON UPDATE CASCADE ON DELETE RESTRICT DEFERRABLE NOT VALID;", + "type": "table.constraint", + "operation": "create", + "path": "public.a_employees.a_employees_company_name_fkey" + }, + { + "sql": "ALTER TABLE a_employees VALIDATE CONSTRAINT a_employees_company_name_fkey;", + "type": "table.constraint", + "operation": "create", + "path": "public.a_employees.a_employees_company_name_fkey" } ] } diff --git a/testdata/diff/online/add_fk/plan.sql b/testdata/diff/online/add_fk/plan.sql index bfd81dc3..72b0ac79 100644 --- a/testdata/diff/online/add_fk/plan.sql +++ b/testdata/diff/online/add_fk/plan.sql @@ -1,4 +1,12 @@ -ALTER TABLE employees -ADD CONSTRAINT employees_company_fkey FOREIGN KEY (tenant_id, company_id) REFERENCES companies (tenant_id, company_id) ON UPDATE CASCADE ON DELETE RESTRICT DEFERRABLE NOT VALID; +ALTER TABLE z_companies +ADD CONSTRAINT z_companies_company_id_name_key UNIQUE (company_id, company_name); -ALTER TABLE employees VALIDATE CONSTRAINT employees_company_fkey; +ALTER TABLE a_employees +ADD CONSTRAINT a_employees_company_fkey FOREIGN KEY (tenant_id, company_id) REFERENCES z_companies (tenant_id, company_id) ON UPDATE CASCADE ON DELETE RESTRICT DEFERRABLE NOT VALID; + +ALTER TABLE a_employees VALIDATE CONSTRAINT a_employees_company_fkey; + +ALTER TABLE a_employees +ADD CONSTRAINT a_employees_company_name_fkey FOREIGN KEY (company_id, company_name) REFERENCES z_companies (company_id, company_name) ON UPDATE CASCADE ON DELETE RESTRICT DEFERRABLE NOT VALID; + +ALTER TABLE a_employees VALIDATE CONSTRAINT a_employees_company_name_fkey; diff --git a/testdata/diff/online/add_fk/plan.txt b/testdata/diff/online/add_fk/plan.txt index b89c8470..4ea7270e 100644 --- a/testdata/diff/online/add_fk/plan.txt +++ b/testdata/diff/online/add_fk/plan.txt @@ -1,16 +1,27 @@ -Plan: 1 to modify. +Plan: 2 to modify. Summary by type: - tables: 1 to modify + tables: 2 to modify Tables: - ~ employees - + employees_company_fkey (constraint) + ~ a_employees + + a_employees_company_fkey (constraint) + + a_employees_company_name_fkey (constraint) + ~ z_companies + + z_companies_company_id_name_key (constraint) DDL to be executed: -------------------------------------------------- -ALTER TABLE employees -ADD CONSTRAINT employees_company_fkey FOREIGN KEY (tenant_id, company_id) REFERENCES companies (tenant_id, company_id) ON UPDATE CASCADE ON DELETE RESTRICT DEFERRABLE NOT VALID; +ALTER TABLE z_companies +ADD CONSTRAINT z_companies_company_id_name_key UNIQUE (company_id, company_name); -ALTER TABLE employees VALIDATE CONSTRAINT employees_company_fkey; +ALTER TABLE a_employees +ADD CONSTRAINT a_employees_company_fkey FOREIGN KEY (tenant_id, company_id) REFERENCES z_companies (tenant_id, company_id) ON UPDATE CASCADE ON DELETE RESTRICT DEFERRABLE NOT VALID; + +ALTER TABLE a_employees VALIDATE CONSTRAINT a_employees_company_fkey; + +ALTER TABLE a_employees +ADD CONSTRAINT a_employees_company_name_fkey FOREIGN KEY (company_id, company_name) REFERENCES z_companies (company_id, company_name) ON UPDATE CASCADE ON DELETE RESTRICT DEFERRABLE NOT VALID; + +ALTER TABLE a_employees VALIDATE CONSTRAINT a_employees_company_name_fkey; From e7dae287a9d0b277b056e283a18989c3a3a444f9 Mon Sep 17 00:00:00 2001 From: Tianzhou Date: Fri, 16 Jan 2026 09:25:13 -0800 Subject: [PATCH 2/2] fix: use order-preserving column comparison for FK constraint matching Address PR review feedback: 1. Fix constraintMatchesFKReference to use order-preserving comparison - PostgreSQL requires composite FK columns to match UNIQUE/PK columns in exact order - Previous set-based approach incorrectly matched FK (col1, col2) with UNIQUE (col2, col1) - Now uses position-by-position comparison after sorting by column position 2. Add comment explaining alphabetical pre-sort for modified tables - Clarifies that pre-sort ensures deterministic insertion order for cycle breaking - Consistent with comment pattern used for added tables (line 1262) Co-Authored-By: Claude Opus 4.5 --- internal/diff/diff.go | 1 + internal/diff/topological.go | 19 ++++++++++--------- 2 files changed, 11 insertions(+), 9 deletions(-) diff --git a/internal/diff/diff.go b/internal/diff/diff.go index d8bc5fa9..f5dceb40 100644 --- a/internal/diff/diff.go +++ b/internal/diff/diff.go @@ -1274,6 +1274,7 @@ func GenerateMigration(oldIR, newIR *ir.IR, targetSchema string) []Diff { // Sort ModifiedTables topologically based on constraint dependencies // This ensures that UNIQUE/PK constraints are added before FKs that reference them + // Pre-sort by name to ensure deterministic insertion order for cycle breaking sortModifiedTables(diff.modifiedTables) diff.modifiedTables = topologicallySortModifiedTables(diff.modifiedTables) diff --git a/internal/diff/topological.go b/internal/diff/topological.go index 1cb9083c..cfbc13d4 100644 --- a/internal/diff/topological.go +++ b/internal/diff/topological.go @@ -630,22 +630,23 @@ func topologicallySortModifiedTables(tableDiffs []*tableDiff) []*tableDiff { } // constraintMatchesFKReference checks if a UNIQUE/PK constraint matches the columns -// referenced by a foreign key constraint +// referenced by a foreign key constraint. +// In PostgreSQL, composite foreign keys must reference columns in the same order as they +// appear in the referenced unique/primary key constraint. +// For example, FK (col1, col2) can only reference UNIQUE (col1, col2), not UNIQUE (col2, col1). func constraintMatchesFKReference(uniqueConstraint, fkConstraint *ir.Constraint) bool { // Must have same number of columns if len(uniqueConstraint.Columns) != len(fkConstraint.ReferencedColumns) { return false } - // Build a set of referenced column names - refColNames := make(map[string]bool) - for _, col := range fkConstraint.ReferencedColumns { - refColNames[col.Name] = true - } + // Sort both constraint columns by position to ensure order-preserving comparison + uniqueCols := sortConstraintColumnsByPosition(uniqueConstraint.Columns) + refCols := sortConstraintColumnsByPosition(fkConstraint.ReferencedColumns) - // Check if all unique constraint columns are in the referenced columns - for _, col := range uniqueConstraint.Columns { - if !refColNames[col.Name] { + // Check if columns match in the same order (position by position) + for i := 0; i < len(uniqueCols); i++ { + if uniqueCols[i].Name != refCols[i].Name { return false } }