fix: dump and diff RLS enabled on partitioned tables (#471)#490
Conversation
ENABLE ROW LEVEL SECURITY on a partitioned table (relkind 'p') was neither dumped nor diffed because GetRLSTables and GetRLSTablesForSchema filtered on relkind = 'r'. Policies on partitioned tables are dumped (those queries have no relkind filter), so a dump rebuilt a database with all policies present but RLS not enabled on partitioned tables — silently leaving the policies unenforced. Include relkind 'p' in both RLS queries so RLSEnabled is populated for partitioned tables in both current- and desired-state inspection. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Fixes a gap in RLS introspection so that ALTER TABLE ... ENABLE ROW LEVEL SECURITY is correctly dumped and diffed for partitioned tables (pg_class.relkind = 'p'), preventing policies from being emitted/applied without RLS actually being enabled.
Changes:
- Updated RLS table discovery queries to include
relkind IN ('r','p')so partitioned tables are considered RLS-enabled when appropriate. - Regenerated
ir/queries/queries.sql.goto reflect the updated SQL. - Added a regression diff test case covering RLS enablement + policy on a partitioned table.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| testdata/diff/create_policy/enable_rls_partitioned/plan.txt | Expected human-readable plan output showing RLS enable + policy creation on a partitioned table. |
| testdata/diff/create_policy/enable_rls_partitioned/plan.sql | Expected SQL plan output for enabling RLS and creating the policy. |
| testdata/diff/create_policy/enable_rls_partitioned/plan.json | Expected JSON plan output including a table.rls step for the partitioned table. |
| testdata/diff/create_policy/enable_rls_partitioned/old.sql | Old-state schema fixture where the partitioned table exists and RLS is not enabled. |
| testdata/diff/create_policy/enable_rls_partitioned/new.sql | New-state schema fixture enabling RLS and adding a policy (issue #471 repro). |
| testdata/diff/create_policy/enable_rls_partitioned/diff.sql | Expected diff SQL output matching the enablement + policy changes. |
| ir/queries/queries.sql | Fixes the root cause by including partitioned tables in the RLS-enabled table queries. |
| ir/queries/queries.sql.go | Regenerated sqlc output reflecting the updated RLS queries used by inspectors/plans. |
Files not reviewed (1)
- ir/queries/queries.sql.go: Generated file
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Greptile SummaryFixes a silent bug where
Confidence Score: 4/5The change is a minimal, targeted catalog query fix with a corresponding diff/plan regression test. The dump command, which also relies on the same fixed query, has no new regression test verifying that partitioned-table RLS is correctly emitted in dump output. The two-line query change is correct and the test fixtures validate the diff/plan path end-to-end. The only gap is the absence of a dump-level regression test, which means the dump half of the stated fix is untested even though the underlying query fix should cover it. No core logic files need special attention; the only gap is a missing testdata/dump/issue_471_*/ suite to cover the dump output path. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant User as User SQL Files
participant EmbeddedPG as Embedded PG
participant Inspector as Inspector
participant Catalog as pg_catalog
participant Diff as Diff Engine
User->>EmbeddedPG: Apply SQL (partitioned table + ENABLE ROW LEVEL SECURITY)
EmbeddedPG->>Inspector: Inspect desired state
Inspector->>Catalog: GetRLSTablesForSchema (relkind IN r,p — FIXED)
Catalog-->>Inspector: "events row (partitioned, rowsecurity=true)"
Inspector->>Inspector: "table.RLSEnabled = true"
Note over Inspector,Catalog: Same flow runs against the live target DB for current state
Inspector-->>Diff: "Desired IR (RLSEnabled=true) vs Current IR (RLSEnabled=false)"
Diff-->>User: ALTER TABLE events ENABLE ROW LEVEL SECURITY + CREATE POLICY
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant User as User SQL Files
participant EmbeddedPG as Embedded PG
participant Inspector as Inspector
participant Catalog as pg_catalog
participant Diff as Diff Engine
User->>EmbeddedPG: Apply SQL (partitioned table + ENABLE ROW LEVEL SECURITY)
EmbeddedPG->>Inspector: Inspect desired state
Inspector->>Catalog: GetRLSTablesForSchema (relkind IN r,p — FIXED)
Catalog-->>Inspector: "events row (partitioned, rowsecurity=true)"
Inspector->>Inspector: "table.RLSEnabled = true"
Note over Inspector,Catalog: Same flow runs against the live target DB for current state
Inspector-->>Diff: "Desired IR (RLSEnabled=true) vs Current IR (RLSEnabled=false)"
Diff-->>User: ALTER TABLE events ENABLE ROW LEVEL SECURITY + CREATE POLICY
Reviews (1): Last reviewed commit: "fix: dump and diff RLS enabled on partit..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| CREATE TABLE events ( | ||
| id uuid NOT NULL, | ||
| org_id uuid NOT NULL, | ||
| created_at timestamptz NOT NULL, | ||
| PRIMARY KEY (created_at, id) | ||
| ) PARTITION BY RANGE (created_at); | ||
|
|
||
| -- RLS is now enabled with a policy on a partitioned table (issue #471) | ||
| ALTER TABLE events ENABLE ROW LEVEL SECURITY; | ||
|
|
||
| CREATE POLICY events_org_isolation ON events | ||
| FOR ALL | ||
| TO PUBLIC | ||
| USING (org_id = NULLIF(current_setting('app.current_org_id', true), '')::uuid); |
There was a problem hiding this comment.
The PR title and description both claim to fix "dump and diff", but only a testdata/diff/ test is added. The dump command uses the same GetRLSTablesForSchema query (fixed here), so a partitioned table with RLS enabled was also silently omitted from schema dumps. Without a corresponding testdata/dump/issue_471_*/ test (with raw.sql, pgschema.sql, etc.), a future regression that re-breaks dump output for partitioned RLS tables would go undetected.
Summary
Fixes #471.
ENABLE ROW LEVEL SECURITYon a partitioned table (relkind 'p') was neither dumped nor diffed, while policies on the same table were dumped — so a database bootstrapped from a dump ended up with all policies present but RLS not enabled, silently leaving the policies unenforced.Root cause
GetRLSTablesandGetRLSTablesForSchema(ir/queries/queries.sql) filtered onc.relkind = 'r', excluding partitioned tables.table.RLSEnabledwas therefore never set forrelkind 'p', in both current-state and desired-state inspection — which is whyplanalso reported "No changes detected" even after hand-adding theENABLE ROW LEVEL SECURITYstatement (both sides of the comparison sawfalse).The policy queries have no
relkindfilter, which is why policies survived and the gap was silent.Fix
Include
relkind 'p'alongside'r'in both RLS queries (.sqlsource + regenerated.sql.go).Tests
testdata/diff/create_policy/enable_rls_partitioned/(mirrors the issue repro: partitionedeventstable + org-isolation policy).TestDiffFromFilesandTestPlanAndApplypass for the new case and the fullcreate_policy/suite.🤖 Generated with Claude Code