fix: defer cross-table policy creation until all tables exist (#373)#374
fix: defer cross-table policy creation until all tables exist (#373)#374
Conversation
Policies may reference other tables in USING/WITH CHECK expressions (e.g., subqueries), and those tables may not exist yet when the policy's parent table is created. Always defer policy creation to after all tables exist, rather than only deferring policies that reference new functions. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Greptile SummaryThis PR fixes a DDL ordering bug where RLS policies containing cross-table subqueries in their Confidence Score: 5/5Safe to merge — the fix is a clean simplification, the regression test validates the exact bug scenario, and the only remaining items are non-blocking doc/dead-code cleanup. The logic change is minimal and correct: unconditionally deferring all policies is strictly safer than the previous selective deferral, which only caught function-reference dependencies but missed cross-table references. Existing tests (10 create_policy + 13 dependency cases) plus the new regression test all pass. The two P2 issues (stale doc comment and dead internal/diff/table.go (stale function comment) and internal/diff/diff.go (dead Important Files Changed
Sequence DiagramsequenceDiagram
participant G as generateCreateSQL
participant B1 as tablesWithoutDeps batch
participant B2 as tablesWithDeps batch
participant C as diffCollector
Note over G,C: Before fix — policy emitted inline (wrong order)
G->>B1: generateCreateTablesSQL(tablesWithoutDeps, shouldDeferPolicy)
B1->>C: CREATE TABLE manager
B1->>C: ALTER TABLE manager ENABLE ROW LEVEL SECURITY
B1->>C: ❌ CREATE POLICY (project_manager doesn't exist yet)
B1->>C: CREATE TABLE project_manager
Note over G,C: After fix — all policies deferred
G->>B1: generateCreateTablesSQL(tablesWithoutDeps)
B1->>C: CREATE TABLE manager
B1->>C: ALTER TABLE manager ENABLE ROW LEVEL SECURITY
B1-->>G: deferredPolicies1 = [employee_manager_select]
G->>B2: generateCreateTablesSQL(tablesWithDeps)
B2->>C: CREATE TABLE project_manager
B2-->>G: deferredPolicies2 = []
G->>C: ✅ CREATE POLICY employee_manager_select (both tables exist)
|
There was a problem hiding this comment.
Pull request overview
This PR fixes statement ordering for PostgreSQL RLS policy creation so policies are created only after all tables exist, preventing failures when policy USING/WITH CHECK expressions reference other tables (issue #373).
Changes:
- Defer all
CREATE POLICYstatements until after both table-creation batches complete. - Simplify create-order logic by removing the “defer only policies referencing new functions” pathway.
- Add a new dependency regression fixture
dependency/issue_373_policy_references_other_table.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
internal/diff/table.go |
Stops emitting policies inline during table creation; collects all policies for a later phase. |
internal/diff/diff.go |
Removes selective policy deferral logic and always applies deferred policies after all tables are created. |
testdata/diff/dependency/issue_373_policy_references_other_table/* |
Adds a repro/expected-plan fixture validating correct ordering for cross-table policy references. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if len(table.Policies) > 0 { | ||
| var inlinePolicies []*ir.RLSPolicy | ||
| policyNames := sortedKeys(table.Policies) | ||
| for _, name := range policyNames { | ||
| policy := table.Policies[name] | ||
| if shouldDeferPolicy != nil && shouldDeferPolicy(policy) { | ||
| deferredPolicies = append(deferredPolicies, policy) | ||
| } else { | ||
| inlinePolicies = append(inlinePolicies, policy) | ||
| } | ||
| } | ||
|
|
||
| if len(inlinePolicies) > 0 { | ||
| generateCreatePoliciesSQL(inlinePolicies, targetSchema, collector) | ||
| deferredPolicies = append(deferredPolicies, table.Policies[name]) | ||
| } |
There was a problem hiding this comment.
With all policies now deferred, ordering across tables becomes more important for deterministic plans. generateCreatePoliciesSQL currently sorts only by policy.Name (policy names are only unique per table in Postgres), so identical policy names on different tables can lead to nondeterministic ordering. Consider sorting by (schema, table, name) or by the full path instead.
internal/diff/table.go
Outdated
| // Defer all policies to after all tables are created. | ||
| // Policies may reference other tables in USING/WITH CHECK expressions (e.g., subqueries), | ||
| // and those tables may not exist yet at this point. (#373) |
There was a problem hiding this comment.
The function-level doc comment for generateCreateTablesSQL (above this block) still describes “co-located” policy creation and deferring only policies referencing new helper functions, but the implementation now defers all policies. Please update the doc comment to match the new behavior to avoid misleading future changes/debugging.
…omment Address review feedback: - Sort policies by (schema, table, name) for deterministic ordering when policies from multiple tables are collected together - Update generateCreateTablesSQL doc comment to reflect that all policies are now deferred Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
internal/diff/diff.go:1553
- Policies are now deferred until after all tables exist, but they are still emitted before views and before the “functions with view dependencies” batch. Since policy USING/WITH CHECK expressions can reference views and/or functions that aren’t created until after
generateCreateViewsSQL/generateCreateFunctionsSQL(functionsWithViewDeps), this ordering can still produce invalid plans for schemas where policies depend on newly-created views or view-dependent functions. Consider moving policy creation to after view creation (and afterfunctionsWithViewDeps) or adding dependency-aware ordering for policy expressions.
// Create tables WITHOUT function/domain dependencies first (functions may reference these)
deferredPolicies1, deferredConstraints1 := generateCreateTablesSQL(tablesWithoutDeps, targetSchema, collector, existingTables)
// Build view lookup - needed for detecting functions that depend on views
newViewLookup := buildViewLookup(d.addedViews)
// Separate functions into those with/without view dependencies
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // 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 | ||
| } | ||
| var shouldDeferPolicy func(*ir.RLSPolicy) bool | ||
| if len(newFunctionLookup) > 0 { | ||
| shouldDeferPolicy = func(policy *ir.RLSPolicy) bool { | ||
| return policyReferencesNewFunction(policy, newFunctionLookup) | ||
| } | ||
| } | ||
|
|
||
| // Create default privileges BEFORE tables so auto-grants apply to new tables | ||
| generateCreateDefaultPrivilegesSQL(d.addedDefaultPrivileges, targetSchema, collector) | ||
|
|
There was a problem hiding this comment.
shouldDeferPolicy was removed, but policyReferencesNewFunction (and its unit test policy_dependency_test.go) now appears to be unused outside tests. Consider deleting the helper/tests or repurposing them for the new “defer all policies” behavior to avoid carrying dead dependency logic.
Instead of deferring all policies (which changed dump output ordering), selectively defer only policies whose USING/WITH CHECK expressions reference another new table being created in the same batch. Policies that only reference their own table or existing tables remain inline. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
internal/diff/table.go:493
- The updated docstring/inline comment says only policies that reference other new tables are deferred, and the implementation still emits
inlinePoliciesduring table creation for policies that don’t matchshouldDeferPolicy. This doesn’t match the PR title/description (“defer all policies to after all tables are created”) and still relies on heuristic detection for correctness. Consider deferring allCREATE POLICYstatements for newly created tables (collect all policies indeferredPoliciesand emit them in one pass after all tables are created), or adjust the PR description/comments to match the selective behavior.
// Collect policies: defer those that reference other new tables or new functions (#373),
// emit the rest inline with their parent table.
if len(table.Policies) > 0 {
var inlinePolicies []*ir.RLSPolicy
policyNames := sortedKeys(table.Policies)
for _, name := range policyNames {
policy := table.Policies[name]
if shouldDeferPolicy != nil && shouldDeferPolicy(policy) {
deferredPolicies = append(deferredPolicies, policy)
} else {
inlinePolicies = append(inlinePolicies, policy)
}
}
if len(inlinePolicies) > 0 {
generateCreatePoliciesSQL(inlinePolicies, targetSchema, collector)
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
internal/diff/table.go
Outdated
| // generateCreateTablesSQL generates CREATE TABLE statements with co-located indexes, policies, and RLS. | ||
| // Policies that reference newly added helper functions are collected for deferred creation after | ||
| // dependent functions/procedures have been created, while all other policies are emitted inline. | ||
| // Policies that reference other new tables in the same batch (via USING/WITH CHECK expressions) |
There was a problem hiding this comment.
Comment says policies referencing other new tables “in the same batch” are deferred, but shouldDeferPolicy is built from all d.addedTables and generateCreateTablesSQL is invoked in two batches (tablesWithoutDeps and tablesWithDeps). Please update the comment to reflect that this defers policies referencing other new tables in the same migration, not just within a single batch.
| // Policies that reference other new tables in the same batch (via USING/WITH CHECK expressions) | |
| // Policies that reference other new tables in the same migration (via USING/WITH CHECK expressions) |
| // policyReferencesOtherNewTable determines if a policy's USING or WITH CHECK expressions | ||
| // reference any newly added table other than the policy's own table (#373). | ||
| func policyReferencesOtherNewTable(policy *ir.RLSPolicy, newTables map[string]struct{}) bool { | ||
| if len(newTables) == 0 || policy == nil { | ||
| return false | ||
| } | ||
|
|
||
| for _, expr := range []string{policy.Using, policy.WithCheck} { | ||
| if expr == "" { | ||
| continue | ||
| } | ||
| exprLower := strings.ToLower(expr) | ||
| for tableName := range newTables { | ||
| // Skip the policy's own table | ||
| if strings.EqualFold(tableName, policy.Table) || | ||
| strings.EqualFold(tableName, fmt.Sprintf("%s.%s", policy.Schema, policy.Table)) { | ||
| continue | ||
| } | ||
| // Check if the expression contains a reference to this table name | ||
| if strings.Contains(exprLower, strings.ToLower(tableName)) { | ||
| return true | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
policyReferencesOtherNewTable does an O(policies × newTables) scan and uses strings.Contains against raw SQL text. If the intended fix is to guarantee policies are created only after all tables exist, it would be more robust (and simpler/faster) to defer policy creation unconditionally for newly created tables instead of trying to detect table references via substring matching.
| // policyReferencesOtherNewTable determines if a policy's USING or WITH CHECK expressions | |
| // reference any newly added table other than the policy's own table (#373). | |
| func policyReferencesOtherNewTable(policy *ir.RLSPolicy, newTables map[string]struct{}) bool { | |
| if len(newTables) == 0 || policy == nil { | |
| return false | |
| } | |
| for _, expr := range []string{policy.Using, policy.WithCheck} { | |
| if expr == "" { | |
| continue | |
| } | |
| exprLower := strings.ToLower(expr) | |
| for tableName := range newTables { | |
| // Skip the policy's own table | |
| if strings.EqualFold(tableName, policy.Table) || | |
| strings.EqualFold(tableName, fmt.Sprintf("%s.%s", policy.Schema, policy.Table)) { | |
| continue | |
| } | |
| // Check if the expression contains a reference to this table name | |
| if strings.Contains(exprLower, strings.ToLower(tableName)) { | |
| return true | |
| } | |
| } | |
| } | |
| // policyReferencesOtherNewTable determines if a policy belongs to a newly added table | |
| // that should have its policy creation deferred until all tables exist (#373). | |
| func policyReferencesOtherNewTable(policy *ir.RLSPolicy, newTables map[string]struct{}) bool { | |
| if len(newTables) == 0 || policy == nil { | |
| return false | |
| } | |
| // Normalize table name for lookup. | |
| tableName := strings.ToLower(policy.Table) | |
| // Check unqualified table name. | |
| if _, ok := newTables[tableName]; ok { | |
| return true | |
| } | |
| // Also check schema-qualified form if a schema is present. | |
| if policy.Schema != "" { | |
| qualified := fmt.Sprintf("%s.%s", strings.ToLower(policy.Schema), tableName) | |
| if _, ok := newTables[qualified]; ok { | |
| return true | |
| } | |
| } |
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
tianzhou
changed the title
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
internal/diff/table.go
Outdated
| // Policies that reference other new tables in the same migration (via USING/WITH CHECK expressions) | ||
| // are deferred for creation after all tables exist (#373). All other policies are emitted inline. | ||
| // It returns deferred policies and foreign key constraints that should be applied after dependent objects exist. | ||
| // Tables are assumed to be pre-sorted in topological order for dependency-aware creation. |
There was a problem hiding this comment.
The doc comment for generateCreateTablesSQL says only policies referencing other new tables are deferred, but the implementation also defers policies that reference newly added functions (via shouldDeferPolicy calling policyReferencesNewFunction). Please update this comment to reflect both deferral conditions so future readers don’t assume function-dependency deferral was removed.
| // Policies that reference other new tables in the same migration (via USING/WITH CHECK expressions) | |
| // are deferred for creation after all tables exist (#373). All other policies are emitted inline. | |
| // It returns deferred policies and foreign key constraints that should be applied after dependent objects exist. | |
| // Tables are assumed to be pre-sorted in topological order for dependency-aware creation. | |
| // Policies that reference other new tables in the same migration or newly added functions (via USING/WITH CHECK | |
| // expressions) are deferred for creation after all tables and functions exist (#373). All other policies are | |
| // emitted inline. It returns deferred policies and foreign key constraints that should be applied after dependent | |
| // objects exist. Tables are assumed to be pre-sorted in topological order for dependency-aware creation. |
| newTableLookup := make(map[string]struct{}, len(d.addedTables)) | ||
| for _, table := range d.addedTables { | ||
| newTableLookup[table.Name] = struct{}{} | ||
| newTableLookup[fmt.Sprintf("%s.%s", table.Schema, table.Name)] = struct{}{} | ||
| } |
There was a problem hiding this comment.
newTableLookup stores an unqualified key (table.Name) for every added table. This collides when the migration adds tables with the same name in different schemas, which can lead to incorrect deferral decisions (e.g., the unqualified key may be treated as the policy’s “own table” and skipped even when the referenced table is actually in another schema). Consider tracking tables by fully-qualified name only, or keeping a mapping from unqualified name → set of qualified names so cross-schema cases can’t be lost.
- Store only fully-qualified (schema.table) keys in newTableLookup to avoid cross-schema name collisions - Update generateCreateTablesSQL doc comment to mention both table and function deferral conditions Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary
policyReferencesOtherNewTablewhich uses substring matching on policy expressions — false positives are safe (just defer unnecessarily), and this matches the existingreferencesNewFunctionpatterndependency/issue_373_policy_references_other_tablereproducing the exact scenario from the issueFixes #373
Test plan
PGSCHEMA_TEST_FILTER="dependency/issue_373" go test -v ./internal/diff -run TestDiffFromFiles— new test passesPGSCHEMA_TEST_FILTER="dependency/" go test -v ./internal/diff -run TestDiffFromFiles— all 14 dependency tests passPGSCHEMA_TEST_FILTER="create_policy/" go test -v ./internal/diff -run TestDiffFromFiles— all 10 policy tests passPGSCHEMA_TEST_FILTER="create_policy/" go test -v ./cmd -run TestPlanAndApply— all 10 policy integration tests passgo test -v ./cmd/dump -run TestDumpCommand_Employee— dump output ordering unchanged for tables without cross-table policy references🤖 Generated with Claude Code