fix: defer FK constraints when referenced tables don't exist yet

feedback.md

@@ -0,0 +1,120 @@
+# PR #156 Feedback Transcript
+
+## Pull Request: Fix non-deterministic table ordering in topological sort
+
+### Timeline of Comments and Feedback
+
+---
+
+#### **p-c-h-b** | November 8, 2025, 03:00
+**Initial PR Description:**
+
+Outlined the fix for FK constraint ordering issues when referenced tables don't exist yet. The contributor explained that "FK-heavy tables could be reordered alphabetically" causing creation failures, and proposed deterministic cycle breaking with deferred constraints applied via ALTER TABLE.
+
+---
+
+#### **Copilot AI** | November 8, 2025, 03:00
+**Automated Review:**
+
+Generated pull request overview noting:
+- Refactored topological sort to handle cycles using insertion order as tiebreaker
+- Added deferred constraint mechanism
+- Deferred RLS policy creation
+
+---
+
+#### **p-c-h-b** | November 8, 2025, 22:15
+**Author Response to Copilot:**
+
+- Added validation for empty constraint names in generateDeferredConstraintsSQL
+- Fixed the comment to accurately reflect insertion order instead of alphabetical order
+
+---
+
+#### **tianzhou** | November 8, 2025, 03:43
+**Reviewer Feedback #1:**
+
+Requested two actions:
+1. Enhance the table_to_table test case to validate the fix
+2. Remove test artifact files (`*_actual.sql`, `*_expected.sql`)
+
+---
+
+#### **p-c-h-b** | November 8, 2025, 22:28
+**Author Response:**
+
+Addressed feedback by enhancing dependency test with circular FK dependencies between departments and users tables to validate constraint deferral.
+
+---
+
+#### **tianzhou** | November 9, 2025, 02:55
+**Reviewer Question:**
+
+"have you forgot to push the update?"
+
+---
+
+#### **p-c-h-b** | November 9, 2025, 09:37
+**Author Update:**
+
+Pushed corrections:
+- Removed test artifacts
+- Fixed employee_status_log constraint to stay inline (one-way dependency)
+- Updated shouldDeferConstraint() logic to check existing tables from old schema
+
+---
+
+#### **tianzhou** | November 10, 2025, 02:55
+**Reviewer Feedback #2 (Latest):**
+
+Identified remaining issues:
+
+1. **Policy Ordering Problem**: In `testdata/dump/employee/pgschema.sql`, RLS policies should be positioned alongside their corresponding `ALTER TABLE...ENABLE ROW LEVEL SECURITY` statements rather than separated. Currently policies appear much later after functions and procedures.
+
+2. **File Exclusion**: Directed the contributor to "exclude this from the PR" regarding the `cmd/dump/employee_expected.sql` file - it shouldn't be committed.
+
+Noted overall progress with "Almost there," indicating the submission is nearly complete pending these structural adjustments.
+
+---
+
+#### **tianzhou** | November 10, 2025, 15:45
+**Reviewer Feedback #3:**
+
+- Reported that emitting `CREATE POLICY` immediately after enabling RLS causes migrations to fail when policies reference helper functions defined later in the diff.
+- Requested restoring deferred policy creation until after all new functions/procedures exist.
+
+---
+
+#### **p-c-h-b** | November 10, 2025, 18:02
+**Author Update:**
+
+- Added selective policy deferral: policies stay co-located with their tables unless they reference helper functions introduced in the same migration, in which case they're executed after the corresponding functions/procedures are created.
+- Updated the employee dump fixture to match the dependency-safe ordering.
+
+---
+
+## Summary of Key Issues Addressed
+
+### Round 1 (Nov 8):
+- Enhanced test coverage for circular FK dependencies
+- Removed test artifact files
+
+### Round 2 (Nov 9):
+- Fixed constraint deferral logic
+- Cleaned up test artifacts
+
+### Round 3 (Nov 10 - Current):
+- **Policy Ordering**: Policies must be co-located with RLS enable statements
+- **Test Artifacts**: Remove `cmd/dump/employee_expected.sql`
+- **Policy Deferral**: Ensure CREATE POLICY statements execute only after dependent functions/procedures when helper functions are newly added
+
+---
+
+## Current Status
+
+**Status**: Feedback addressed in latest commits
+- ✅ Removed test artifact file (`cmd/dump/employee_expected.sql`)
+- ✅ RLS enable statements remain co-located with their tables, with policies only deferred when they depend on helper functions added in the same migration
+- ✅ Updated test fixture (`testdata/dump/employee/pgschema.sql`)
+- ✅ All tests passing
+- ✅ Restored dependency-safe policy creation without sacrificing readability

internal/diff/diff.go

@@ -3,6 +3,7 @@ package diff
 import (
 	"encoding/json"
 	"fmt"
+	"regexp"
 	"sort"
 	"strings"
 
@@ -878,7 +879,15 @@ func GenerateMigration(oldIR, newIR *ir.IR, targetSchema string) []Diff {
 	}
 
 	// Sort tables and views topologically for consistent ordering
+	// Pre-sort by name to ensure deterministic insertion order for cycle breaking
+	sort.Slice(diff.addedTables, func(i, j int) bool {
+		return diff.addedTables[i].Schema+"."+diff.addedTables[i].Name < diff.addedTables[j].Schema+"."+diff.addedTables[j].Name
+	})
 	diff.addedTables = topologicallySortTables(diff.addedTables)
+
+	sort.Slice(diff.droppedTables, func(i, j int) bool {
+		return diff.droppedTables[i].Schema+"."+diff.droppedTables[i].Name < diff.droppedTables[j].Schema+"."+diff.droppedTables[j].Name
+	})
 	diff.droppedTables = reverseSlice(topologicallySortTables(diff.droppedTables))
 	diff.addedViews = topologicallySortViews(diff.addedViews)
 	diff.droppedViews = reverseSlice(topologicallySortViews(diff.droppedViews))
@@ -919,15 +928,36 @@ func (d *ddlDiff) generateCreateSQL(targetSchema string, collector *diffCollecto
 	// Create sequences
 	generateCreateSequencesSQL(d.addedSequences, targetSchema, collector)
 
-	// Create tables with co-located indexes, constraints, triggers, and RLS
-	generateCreateTablesSQL(d.addedTables, targetSchema, collector)
+	// Build map of existing tables (tables being modified, so they already exist)
+	existingTables := make(map[string]bool, len(d.modifiedTables))
+	for _, tableDiff := range d.modifiedTables {
+		key := fmt.Sprintf("%s.%s", tableDiff.Table.Schema, tableDiff.Table.Name)
+		existingTables[key] = true
+	}
+
+	newFunctionLookup := buildFunctionLookup(d.addedFunctions)
+	var shouldDeferPolicy func(*ir.RLSPolicy) bool
+	if len(newFunctionLookup) > 0 {
+		shouldDeferPolicy = func(policy *ir.RLSPolicy) bool {
+			return policyReferencesNewFunction(policy, newFunctionLookup)
+		}
+	}
+
+	// Create tables with co-located indexes, policies, and RLS and collect deferred work
+	deferredPolicies, deferredConstraints := generateCreateTablesSQL(d.addedTables, targetSchema, collector, existingTables, shouldDeferPolicy)
+
+	// Add deferred foreign key constraints now that referenced tables exist
+	generateDeferredConstraintsSQL(deferredConstraints, targetSchema, collector)
 
 	// Create functions (functions may depend on tables)
 	generateCreateFunctionsSQL(d.addedFunctions, targetSchema, collector)
 
 	// Create procedures (procedures may depend on tables)
 	generateCreateProceduresSQL(d.addedProcedures, targetSchema, collector)
 
+	// Create policies after functions/procedures to satisfy dependencies
+	generateCreatePoliciesSQL(deferredPolicies, targetSchema, collector)
+
 	// Create triggers (triggers may depend on functions/procedures)
 	generateCreateTriggersFromTables(d.addedTables, targetSchema, collector)
 
@@ -1117,6 +1147,75 @@ func sortedKeys[T any](m map[string]T) []string {
 	return keys
 }
 
+// buildFunctionLookup returns case-insensitive lookup keys for newly added functions.
+// Keys include both unqualified (function name only) and schema-qualified identifiers.
+func buildFunctionLookup(functions []*ir.Function) map[string]struct{} {
+	if len(functions) == 0 {
+		return nil
+	}
+
+	lookup := make(map[string]struct{}, len(functions)*2)
+	for _, fn := range functions {
+		if fn == nil || fn.Name == "" {
+			continue
+		}
+
+		name := strings.ToLower(fn.Name)
+		lookup[name] = struct{}{}
+
+		if fn.Schema != "" {
+			qualified := fmt.Sprintf("%s.%s", strings.ToLower(fn.Schema), name)
+			lookup[qualified] = struct{}{}
+		}
+	}
+	return lookup
+}
+
+var functionCallRegex = regexp.MustCompile(`(?i)([a-z_][a-z0-9_$.]*)\s*\(`)
+
+// policyReferencesNewFunction determines if a policy references any newly added functions.
+func policyReferencesNewFunction(policy *ir.RLSPolicy, newFunctions map[string]struct{}) bool {
+	if len(newFunctions) == 0 || policy == nil {
+		return false
+	}
+
+	for _, expr := range []string{policy.Using, policy.WithCheck} {
+		if referencesNewFunction(expr, policy.Schema, newFunctions) {
+			return true
+		}
+	}
+	return false
+}
+
+func referencesNewFunction(expr, defaultSchema string, newFunctions map[string]struct{}) bool {
+	if expr == "" || len(newFunctions) == 0 {
+		return false
+	}
+
+	matches := functionCallRegex.FindAllStringSubmatch(expr, -1)
+	for _, match := range matches {
+		if len(match) < 2 {
+			continue
+		}
+		identifier := strings.ToLower(match[1])
+		if identifier == "" {
+			continue
+		}
+
+		if _, ok := newFunctions[identifier]; ok {
+			return true
+		}
+
+		if !strings.Contains(identifier, ".") && defaultSchema != "" {
+			qualified := fmt.Sprintf("%s.%s", strings.ToLower(defaultSchema), identifier)
+			if _, ok := newFunctions[qualified]; ok {
+				return true
+			}
+		}
+	}
+	return false
+}
+
 // DiffSource interface implementations for diff types
 func (d *schemaDiff) IsDiffSource()     {}
 func (d *functionDiff) IsDiffSource()   {}

internal/diff/policy_dependency_test.go

@@ -0,0 +1,61 @@
+package diff
+
+import (
+	"testing"
+
+	"github.com/pgschema/pgschema/ir"
+)
+
+func TestPolicyReferencesNewFunction_Unqualified(t *testing.T) {
+	functions := []*ir.Function{
+		{Schema: "public", Name: "tenant_filter"},
+	}
+	lookup := buildFunctionLookup(functions)
+
+	policy := &ir.RLSPolicy{
+		Schema: "public",
+		Table:  "users",
+		Name:   "tenant_policy",
+		Using:  "tenant_filter(tenant_id)",
+	}
+
+	if !policyReferencesNewFunction(policy, lookup) {
+		t.Fatalf("expected policy to reference newly added function")
+	}
+}
+
+func TestPolicyReferencesNewFunction_Qualified(t *testing.T) {
+	functions := []*ir.Function{
+		{Schema: "auth", Name: "tenant_filter"},
+	}
+	lookup := buildFunctionLookup(functions)
+
+	policy := &ir.RLSPolicy{
+		Schema: "public",
+		Table:  "users",
+		Name:   "tenant_policy",
+		Using:  "auth.tenant_filter(tenant_id)",
+	}
+
+	if !policyReferencesNewFunction(policy, lookup) {
+		t.Fatalf("expected policy to match schema-qualified helper function")
+	}
+}
+
+func TestPolicyReferencesNewFunction_BuiltInIgnored(t *testing.T) {
+	functions := []*ir.Function{
+		{Schema: "public", Name: "tenant_filter"},
+	}
+	lookup := buildFunctionLookup(functions)
+
+	policy := &ir.RLSPolicy{
+		Schema: "public",
+		Table:  "audit",
+		Name:   "audit_policy",
+		Using:  "user_name = CURRENT_USER",
+	}
+
+	if policyReferencesNewFunction(policy, lookup) {
+		t.Fatalf("expected policy referencing only built-in functions to remain inline")
+	}
+}