diff --git a/internal/diff/diff.go b/internal/diff/diff.go index 07c05465..f5dceb40 100644 --- a/internal/diff/diff.go +++ b/internal/diff/diff.go @@ -1272,9 +1272,11 @@ 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 + // Pre-sort by name to ensure deterministic insertion order for cycle breaking 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..cfbc13d4 100644 --- a/internal/diff/topological.go +++ b/internal/diff/topological.go @@ -510,3 +510,146 @@ 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. +// 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 + } + + // Sort both constraint columns by position to ensure order-preserving comparison + uniqueCols := sortConstraintColumnsByPosition(uniqueConstraint.Columns) + refCols := sortConstraintColumnsByPosition(fkConstraint.ReferencedColumns) + + // 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 + } + } + + 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;