Skip to content

fix: render policy and constraint expressions with table schema search_path (#449) #467

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
tianzhou merged 1 commit into from
Jun 10, 2026
+199 −64
Merged

fix: render policy and constraint expressions with table schema search_path (#449) #467

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
96 changes: 96 additions & 0 deletions cmd/issue_449_integration_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
package cmd

import (
"context"
"os"
"path/filepath"
"testing"

"github.com/pgplex/pgschema/testutil"
)

// TestIssue449RepeatPlanIdempotency verifies that a plan generated immediately after a
// successful apply reports no changes (https://github.com/pgplex/pgschema/issues/449).
//
// Two distinct drift sources are covered:
// 1. CHECK constraints casting to a type in the managed (non-public) schema: the current
// state rendered the cast schema-qualified while the desired state stripped the
// qualifier, producing a perpetual DROP/ADD/VALIDATE cycle (also issue #445).
// 2. RLS policy expressions on a table whose name equals the managed schema name: the
// same-schema qualifier stripping mistook table-qualified column references
// (profiles.id) for schema qualifiers and removed them from the current state only.
func TestIssue449RepeatPlanIdempotency(t *testing.T) {
if testing.Short() {
t.Skip("Skipping integration test in short mode")
}

ctx := context.Background()

embeddedPG := testutil.SetupPostgres(t)
defer embeddedPG.Stop()
conn, host, port, dbname, user, password := testutil.ConnectToPostgres(t, embeddedPG)
defer conn.Close()

// applyThenReplan applies the desired state to the given schema and returns the SQL
// of a plan generated immediately afterwards. An idempotent plan returns "".
applyThenReplan := func(t *testing.T, schema, desiredSQL string) string {
t.Helper()

if _, err := conn.ExecContext(ctx, "CREATE SCHEMA IF NOT EXISTS "+schema); err != nil {
t.Fatalf("Failed to create schema %s: %v", schema, err)
}

desiredStateFile := filepath.Join(t.TempDir(), "desired.sql")
if err := os.WriteFile(desiredStateFile, []byte(desiredSQL), 0644); err != nil {
t.Fatalf("Failed to write desired state file: %v", err)
}

if err := applySchemaChanges(host, port, dbname, user, password, schema, desiredStateFile); err != nil {
t.Fatalf("Failed to apply desired state: %v", err)
}

replanOutput, err := generatePlanSQLFormatted(host, port, dbname, user, password, schema, desiredStateFile)
if err != nil {
t.Fatalf("Failed to generate repeat plan: %v", err)
}
return replanOutput
}

t.Run("check_constraint_same_schema_type_cast", func(t *testing.T) {
replan := applyThenReplan(t, "app449", `
CREATE TYPE item_status AS ENUM ('active', 'disabled');

CREATE TABLE items (
id integer PRIMARY KEY,
status item_status NOT NULL,
CONSTRAINT items_status_check CHECK (status <> 'disabled'::item_status)
);
`)
if replan != "" {
t.Errorf("Expected no changes on repeat plan after apply, but got:\n%s", replan)
}
})

t.Run("policy_schema_name_equals_table_name", func(t *testing.T) {
replan := applyThenReplan(t, "profiles", `
CREATE TABLE profiles (
id uuid PRIMARY KEY,
user_id uuid NOT NULL
);

CREATE TABLE profile_media (
id uuid PRIMARY KEY,
profile_id uuid NOT NULL REFERENCES profiles (id)
);

ALTER TABLE profile_media ENABLE ROW LEVEL SECURITY;

CREATE POLICY "Users can read their own media" ON profile_media
FOR SELECT
USING (profile_id IN (SELECT id FROM profiles));
`)
if replan != "" {
t.Errorf("Expected no changes on repeat plan after apply, but got:\n%s", replan)
}
})
}
37 changes: 13 additions & 24 deletions ir/normalize.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,9 +82,9 @@ func normalizeTable(table *Table) {
normalizeColumn(column, table.Schema)
}

// Normalize policies (pass table schema for context - Issue #220)
// Normalize policies
for _, policy := range table.Policies {
normalizePolicy(policy, table.Schema)
normalizePolicy(policy)
}

// Normalize triggers
Expand Down Expand Up @@ -204,8 +204,7 @@ func normalizeDefaultValue(value string, tableSchema string) string {
}

// normalizePolicy normalizes RLS policy representation
// tableSchema is used to strip same-schema qualifiers from function calls (Issue #220)
func normalizePolicy(policy *RLSPolicy, tableSchema string) {
func normalizePolicy(policy *RLSPolicy) {
if policy == nil {
return
}
Expand All @@ -215,8 +214,8 @@ func normalizePolicy(policy *RLSPolicy, tableSchema string) {

// Normalize expressions by removing extra whitespace
// For policy expressions, we want to preserve parentheses as they are part of the expected format
policy.Using = normalizePolicyExpression(policy.Using, tableSchema)
policy.WithCheck = normalizePolicyExpression(policy.WithCheck, tableSchema)
policy.Using = normalizePolicyExpression(policy.Using)
policy.WithCheck = normalizePolicyExpression(policy.WithCheck)
}

// normalizePolicyRoles normalizes policy roles for consistent comparison
Expand All @@ -243,8 +242,14 @@ func normalizePolicyRoles(roles []string) []string {

// normalizePolicyExpression normalizes policy expressions (USING/WITH CHECK clauses)
// It preserves parentheses as they are part of the expected format for policies
// tableSchema is used to strip same-schema qualifiers from function calls and table references (Issue #220, #224)
func normalizePolicyExpression(expr string, tableSchema string) string {
//
// Same-schema qualifiers no longer need stripping here: GetRLSPoliciesForSchema renders
// expressions with search_path set to the table's own schema, so same-schema references
// (Issue #220, #224) come out unqualified on both the current and desired side, while
// cross-schema references (Issue #427) stay qualified. Regex-stripping was also unsafe:
// when the schema name equals a table name, it mangled table-qualified column
// references like profiles.id (Issue #449).
func normalizePolicyExpression(expr string) string {
if expr == "" {
return expr
}
Expand All @@ -253,22 +258,6 @@ func normalizePolicyExpression(expr string, tableSchema string) string {
expr = strings.TrimSpace(expr)
expr = regexp.MustCompile(`\s+`).ReplaceAllString(expr, " ")

// Strip same-schema qualifiers from function calls (Issue #220)
// This matches PostgreSQL's behavior where same-schema qualifiers are stripped
// Example: tenant1.auth_uid() -> auth_uid() (when tableSchema is "tenant1")
// util.get_status() -> util.get_status() (preserved, different schema)
if tableSchema != "" && strings.Contains(expr, tableSchema+".") {
prefix := tableSchema + "."
pattern := regexp.MustCompile(regexp.QuoteMeta(prefix) + `([a-zA-Z_][a-zA-Z0-9_]*)\(`)
expr = pattern.ReplaceAllString(expr, `${1}(`)

// Strip same-schema qualifiers from table references (Issue #224)
// Matches schema.identifier followed by whitespace, comma, closing paren, or end of string
// Example: public.users -> users (when tableSchema is "public")
tablePattern := regexp.MustCompile(regexp.QuoteMeta(prefix) + `([a-zA-Z_][a-zA-Z0-9_]*)(\s|,|\)|$)`)
expr = tablePattern.ReplaceAllString(expr, `${1}${2}`)
}

// Handle all parentheses normalization (adding required ones, removing unnecessary ones)
expr = normalizeExpressionParentheses(expr)

Expand Down
30 changes: 16 additions & 14 deletions ir/normalize_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -361,30 +361,32 @@ func TestNormalizeExpressionParentheses(t *testing.T) {

func TestNormalizePolicyExpression(t *testing.T) {
tests := []struct {
name string
expr string
tableSchema string
expected string
name string
expr string
expected string
}{
{
name: "nested function call in policy expression",
expr: "(id IN ( SELECT unnest(get_user_assigned_projects()) AS unnest))",
tableSchema: "public",
expected: "(id IN ( SELECT unnest(get_user_assigned_projects()) AS unnest))",
name: "nested function call in policy expression",
expr: "(id IN ( SELECT unnest(get_user_assigned_projects()) AS unnest))",
expected: "(id IN ( SELECT unnest(get_user_assigned_projects()) AS unnest))",
},
{
name: "schema-qualified nested function call",
expr: "(id IN ( SELECT unnest(public.get_user_assigned_projects()) AS unnest))",
tableSchema: "public",
expected: "(id IN ( SELECT unnest(get_user_assigned_projects()) AS unnest))",
// Schema qualifiers are preserved as rendered: GetRLSPoliciesForSchema sets
// search_path to the table's schema, so qualifiers only appear for
// cross-schema references and must survive normalization (issue #427).
// Table-qualified column references (profiles.id when the schema is also
// named profiles) must survive too (issue #449).
name: "qualified references preserved",
expr: "(id IN ( SELECT profiles.id FROM profiles WHERE (profiles.user_id = auth.uid())))",
expected: "(id IN ( SELECT profiles.id FROM profiles WHERE (profiles.user_id = auth.uid())))",
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
result := normalizePolicyExpression(tt.expr, tt.tableSchema)
result := normalizePolicyExpression(tt.expr)
if result != tt.expected {
t.Errorf("normalizePolicyExpression(%q, %q) = %q, want %q", tt.expr, tt.tableSchema, result, tt.expected)
t.Errorf("normalizePolicyExpression(%q) = %q, want %q", tt.expr, result, tt.expected)
}
})
}
Expand Down
50 changes: 37 additions & 13 deletions ir/queries/queries.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -325,8 +325,8 @@ SELECT
COALESCE(fcl.relname, '') AS foreign_table_name,
COALESCE(fa.attname, '') AS foreign_column_name,
COALESCE(fa.attnum, 0) AS foreign_ordinal_position,
CASE WHEN c.contype = 'c' THEN pg_get_constraintdef(c.oid, true) ELSE NULL END AS check_clause,
CASE WHEN c.contype = 'x' THEN pg_get_constraintdef(c.oid, true) ELSE NULL END AS exclusion_definition,
cd.check_clause,
cd.exclusion_definition,
CASE c.confdeltype
WHEN 'a' THEN 'NO ACTION'
WHEN 'r' THEN 'RESTRICT'
Expand Down Expand Up @@ -355,6 +355,16 @@ LEFT JOIN pg_attribute a ON a.attrelid = c.conrelid AND a.attnum = ANY(c.conkey)
LEFT JOIN pg_class fcl ON c.confrelid = fcl.oid
LEFT JOIN pg_namespace fn ON fcl.relnamespace = fn.oid
LEFT JOIN pg_attribute fa ON fa.attrelid = c.confrelid AND fa.attnum = c.confkey[array_position(c.conkey, a.attnum)]
LEFT JOIN LATERAL (
SELECT
-- Render with search_path set to the table's own schema so same-schema
-- type/function references come out unqualified while cross-schema
-- references stay qualified. This matches how the desired state renders
-- in its temporary schema, keeping both sides comparable (issue #449).
set_config('search_path', quote_ident(n.nspname), true) AS dummy,
CASE WHEN c.contype = 'c' THEN pg_get_constraintdef(c.oid, true) ELSE NULL END AS check_clause,
CASE WHEN c.contype = 'x' THEN pg_get_constraintdef(c.oid, true) ELSE NULL END AS exclusion_definition
) cd ON true

@tianzhou tianzhou Jun 10, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Keeping ON true deliberately: it matches the existing LATERAL set_config pattern used by the column-default and index queries in this file, which also execute for every row (columns are far more numerous than constraints). Inspection is a cold path that runs once per plan/dump, and set_config costs microseconds per row, so filtering on contype would save negligible work while diverging from the established pattern.

WHERE n.nspname NOT IN ('information_schema', 'pg_catalog', 'pg_toast')
AND n.nspname NOT LIKE 'pg_temp_%'
AND n.nspname NOT LIKE 'pg_toast_temp_%'
Expand Down Expand Up @@ -769,13 +779,12 @@ ORDER BY n.nspname, c.relname;

-- GetRLSPolicies retrieves all row level security policies
-- This replicates the pg_policies system view but computes the qual/with_check
-- expressions with a forced search_path so cross-schema references stay qualified:
-- 1. set_config sets search_path to only pg_catalog
-- 2. pg_get_expr then uses that search_path and includes schema qualifiers for any
-- function or table reference outside pg_catalog. Same-schema qualifiers are stripped
-- later by normalizePolicyExpression. Without this, references to objects on the
-- session search_path (e.g. Supabase's auth.uid()) are emitted unqualified and the
-- generated CREATE POLICY fails on apply. (Issue #427)
-- expressions with a forced search_path so qualification is deterministic:
-- 1. set_config sets search_path to only the table's own schema
-- 2. pg_get_expr then renders same-schema references unqualified while cross-schema
-- references (e.g. Supabase's auth.uid(), issue #427) stay qualified. Without this,
-- rendering depends on the session search_path and the current/desired states
-- compare differently (issue #449).
-- name: GetRLSPolicies :many
SELECT
n.nspname AS schemaname,
Expand Down Expand Up @@ -805,7 +814,7 @@ JOIN pg_catalog.pg_class c ON c.oid = pol.polrelid
JOIN pg_catalog.pg_namespace n ON n.oid = c.relnamespace
LEFT JOIN LATERAL (
SELECT
set_config('search_path', 'pg_catalog', true) AS dummy,
set_config('search_path', quote_ident(n.nspname), true) AS dummy,
pg_get_expr(pol.polqual, pol.polrelid) AS qual,
pg_get_expr(pol.polwithcheck, pol.polrelid) AS with_check
) e ON true
Expand Down Expand Up @@ -862,7 +871,12 @@ JOIN pg_catalog.pg_class c ON c.oid = pol.polrelid
JOIN pg_catalog.pg_namespace n ON n.oid = c.relnamespace
LEFT JOIN LATERAL (
SELECT
set_config('search_path', 'pg_catalog', true) AS dummy,
-- Render with search_path set to the table's own schema so same-schema
-- references come out unqualified while cross-schema references (e.g.
-- Supabase's auth.uid(), issue #427) stay qualified. This matches how the
-- desired state renders in its temporary schema, so both sides compare
-- equal without regex-stripping qualifiers afterwards (issue #449).
set_config('search_path', quote_ident(n.nspname), true) AS dummy,

@greptile-apps greptile-apps Bot Jun 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Public fallback is missing

The desired-state database is loaded with search_path set to the temporary schema plus public, but current-state policy rendering now uses only the table schema. For a non-public managed schema with a policy that calls a helper function resolved from public, the desired side can render has_scope(...) while the current side renders public.has_scope(...), so plans can keep reporting policy changes even after a successful apply.

@tianzhou tianzhou Jun 10, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The apply-time search_path (tmp, public) only affects name resolution at CREATE time — PostgreSQL stores the resolved OID in polqual. Rendering happens at inspection time, and both the current and desired states are inspected through this same query, so both render under search_path = the inspected table's own schema (temp schema on the desired side, managed schema on the current side). A helper resolved from public therefore renders as public.has_scope(...) on both sides — symmetric, no drift. Verified empirically on PG 17 with exactly this scenario (non-public schema, policy + CHECK calling a public helper and casting to a public enum, with both qualified and unqualified references in the source file): apply followed by replan reports "No changes detected" in all variants.

pg_get_expr(pol.polqual, pol.polrelid) AS qual,
pg_get_expr(pol.polwithcheck, pol.polrelid) AS with_check
Comment on lines +879 to 881

@greptile-apps greptile-apps Bot Jun 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Policy rendering can drift

This has the same ordering problem as the constraint query: set_config is selected as an unused dummy column, so pg_get_expr is not structurally forced to run after the table-schema search_path is installed. If the expression is rendered under the old session state, a policy can still alternate between qualified and unqualified references, producing repeated ALTER POLICY output after an apply.

@tianzhou tianzhou Jun 10, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Same as the constraint-query thread: this is the pre-existing LATERAL set_config pattern from the column-default and index queries (issue #218), relying on in-order evaluation of volatile target-list entries, re-executed per row. Empirically validated by the red-to-green TestIssue449RepeatPlanIdempotency plus the full diff and dump suites.

@greptile-apps greptile-apps Bot Jun 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Fair point. You're right that this is the pre-existing LATERAL set_config pattern established in #218 for column defaults and indexes, and it's consistent across all expression-rendering queries in the file. My concern about ordering was based on SQL standard semantics, but PostgreSQL's implementation guarantees left-to-right evaluation of volatile target-list entries within a SELECT — and this codebase already relies on that guarantee in multiple places with empirical validation.

Given the red-to-green test coverage on TestIssue449RepeatPlanIdempotency plus the full diff/dump regression suite, the concern is resolved. Happy to withdraw it.

) e ON true
Expand Down Expand Up @@ -970,8 +984,8 @@ SELECT
COALESCE(fcl.relname, '') AS foreign_table_name,
COALESCE(fa.attname, '') AS foreign_column_name,
COALESCE(fa.attnum, 0) AS foreign_ordinal_position,
CASE WHEN c.contype = 'c' THEN pg_get_constraintdef(c.oid, true) ELSE NULL END AS check_clause,
CASE WHEN c.contype = 'x' THEN pg_get_constraintdef(c.oid, true) ELSE NULL END AS exclusion_definition,
cd.check_clause,
cd.exclusion_definition,
CASE c.confdeltype
WHEN 'a' THEN 'NO ACTION'
WHEN 'r' THEN 'RESTRICT'
Expand Down Expand Up @@ -1004,6 +1018,16 @@ LEFT JOIN pg_class fcl ON c.confrelid = fcl.oid
LEFT JOIN pg_namespace fn ON fcl.relnamespace = fn.oid
LEFT JOIN pg_attribute fa ON fa.attrelid = c.confrelid AND fa.attnum = c.confkey[array_position(c.conkey, a.attnum)]
LEFT JOIN pg_index i ON i.indexrelid = c.conindid
LEFT JOIN LATERAL (
SELECT
-- Render with search_path set to the table's own schema so same-schema
-- type/function references come out unqualified while cross-schema
-- references stay qualified. This matches how the desired state renders
-- in its temporary schema, keeping both sides comparable (issue #449).
set_config('search_path', quote_ident(n.nspname), true) AS dummy,

@greptile-apps greptile-apps Bot Jun 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Constraint context differs

The desired-state path applies SQL with public in the fallback search path, but this current-state constraint query renders with only the table schema. A CHECK constraint in a non-public schema that calls a public helper or casts to a public extension type can therefore render unqualified on the desired side and public.-qualified on the current side, causing repeat plans to keep dropping and re-adding the constraint.

@tianzhou tianzhou Jun 10, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Same as the policy-query thread: apply-time search_path only affects resolution, not rendering. Both sides are inspected through this same query, so a CHECK constraint referencing a public helper or casting to a public type renders public.-qualified on both sides — symmetric, no drift. Verified empirically on PG 17 (non-public schema, CHECK (public.has_scope(scope)) and 'bad'::public.pub_status, qualified and unqualified source variants): repeat plan after apply reports "No changes detected".

@greptile-apps greptile-apps Bot Jun 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You're right — I was wrong. Since both the current state and desired state are read through the same query (with the same set_config lateral), cross-schema references like public.has_scope(...) or 'bad'::public.pub_status render public.-qualified on both sides, keeping them symmetric. My concern about asymmetric rendering was unfounded. Thanks for the empirical verification.

CASE WHEN c.contype = 'c' THEN pg_get_constraintdef(c.oid, true) ELSE NULL END AS check_clause,
CASE WHEN c.contype = 'x' THEN pg_get_constraintdef(c.oid, true) ELSE NULL END AS exclusion_definition
Comment on lines +1027 to +1029

@greptile-apps greptile-apps Bot Jun 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Search path ordering is unsafe

This lateral subquery relies on the unreferenced dummy target column to run before pg_get_constraintdef, but the outer query never reads dummy and SQL target-list evaluation is not a safe ordering dependency for changing a GUC. When pg_get_constraintdef runs under the previous session or previous row's search_path, CHECK and EXCLUDE definitions can still be rendered with the wrong qualification, leaving the repeat-plan drift this change is meant to remove.

@tianzhou tianzhou Jun 10, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The ordering here is the established pattern in this file: the column-default and index-predicate queries have used the same LATERAL set_config(...) + pg_get_expr arrangement since issue #218 — see the comment block on GetColumnsForSchema ("Use LATERAL join to guarantee execution order") which also documents the rejected alternative of a wrapper function. Within a row's target list PostgreSQL evaluates entries in order, and set_config is volatile, so the planner neither reorders nor elides it; it also re-executes for every row, so there is no stale previous-row state. Empirically: the new TestIssue449RepeatPlanIdempotency was red before this change and green after, and the full diff (150+ cases) and dump suites pass — those inspect many constraint rows across schemas. If this ordering were unreliable, the pre-existing column-default query would have exhibited the same nondeterminism long ago.

@greptile-apps greptile-apps Bot Jun 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You're right. My concern was overstated. PostgreSQL does guarantee that volatile functions in a target list are evaluated in order and not elided — the planner cannot skip set_config because it has side effects, regardless of whether dummy is referenced upstream. The pre-existing column-default queries using this exact pattern would have surfaced nondeterminism long ago if the ordering were unreliable.

I withdraw the comment. The implementation is sound.

) cd ON true
Comment thread
tianzhou marked this conversation as resolved.
WHERE n.nspname = $1
-- Skip internal per-partition FK rows (conparentid != 0) that PostgreSQL
-- creates when a FK references a partitioned table. pg_dump omits these;
Expand Down
Loading