Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions internal/diff/diff.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
143 changes: 143 additions & 0 deletions internal/diff/topological.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Comment on lines +514 to +517
Copy link

Copilot AI Jan 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The topologicallySortModifiedTables() function lacks unit test coverage. Other topological sorting functions in this file have comprehensive unit tests (e.g., TestTopologicallySortTablesHandlesCycles, TestTopologicallySortTypesHandlesCycles). Consider adding similar tests to verify: (1) correct dependency ordering when table A's FK references table B's newly-added UNIQUE constraint, (2) cycle detection and breaking, (3) deterministic ordering when there are no dependencies, and (4) self-reference handling.

Copilot uses AI. Check for mistakes.
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
}
Comment on lines +592 to +601
Copy link

Copilot AI Jan 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The cycle-breaking strategy lacks the detailed explanation found in similar functions (see topologicallySortTables lines 69-87 and topologicallySortFunctions lines 458-476). For consistency and maintainability, consider adding a similar detailed comment block explaining: (1) why setting inDegree[next] = 0 is safe, (2) how the processed map prevents duplicate processing, (3) why cycle breaking works for modified tables with FK/UNIQUE constraint dependencies, and (4) that using insertion order ensures deterministic output.

Copilot uses AI. Check for mistakes.

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 {
Comment on lines +632 to +637
Copy link

Copilot AI Jan 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The constraintMatchesFKReference() helper function lacks unit test coverage. Consider adding tests to verify: (1) matching single-column constraints, (2) matching multi-column constraints with correct order, (3) non-matching constraints with different column orders, (4) non-matching constraints with different column counts, and (5) handling of position-based column ordering.

Copilot uses AI. Check for mistakes.
// 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
}
10 changes: 8 additions & 2 deletions testdata/diff/online/add_fk/diff.sql
Original file line number Diff line number Diff line change
@@ -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;
23 changes: 19 additions & 4 deletions testdata/diff/online/add_fk/new.sql
Original file line number Diff line number Diff line change
@@ -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
);
18 changes: 14 additions & 4 deletions testdata/diff/online/add_fk/old.sql
Original file line number Diff line number Diff line change
@@ -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
);
28 changes: 23 additions & 5 deletions testdata/diff/online/add_fk/plan.json
Original file line number Diff line number Diff line change
Expand Up @@ -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"
}
]
}
Expand Down
14 changes: 11 additions & 3 deletions testdata/diff/online/add_fk/plan.sql
Original file line number Diff line number Diff line change
@@ -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;
25 changes: 18 additions & 7 deletions testdata/diff/online/add_fk/plan.txt
Original file line number Diff line number Diff line change
@@ -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;
Loading