executor: apply row security changes atomically - #123
Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Budget and missing-table failures bypass the stable error taxonomy, and the live SQL generation boundary needs correction.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 4
Open (5)
Use native_safer_sequence for synthesized policy replay · New Normalize derived deadline expiry as statement budget exceeded · New Map PostgreSQL lock and missing-table errors to stable outcomes · New Generate policy SQL from admitted catalog model · New Add end-to-end coverage for qualified external helper resolution · New
What changed in this PR
Adds an atomic Go executor for applying complete row-level security definitions to existing tables.
Changes:
- Adds bounded, transactional RLS replacement and convergence verification.
- Adds statement qualification, scratch introspection, reports, and outcome codes.
- Updates tests, capability metadata, safety contracts, and documentation.
| File | Description |
|---|---|
SAFETY.md |
Extends the trusted-core model for RLS execution. |
README.md |
Documents Go API support. |
.agents/checks/review.md |
Updates review guidance for RLS execution. |
pkg/statement/row_security_execution.go |
Generates qualified RLS statements. |
pkg/statement/row_security_execution_test.go |
Tests RLS statement qualification. |
pkg/statement/desired_rls.go |
Expands the declaration type’s execution scope. |
pkg/schemadiff/row_security_roundtrip.go |
Adds transactional desired-state inspection. |
pkg/schemadiff/desired.go |
Refactors scratch inspection around caller transactions. |
pkg/executor/row_security.go |
Implements atomic RLS execution. |
pkg/executor/row_security_test.go |
Tests input validation. |
pkg/executor/row_security_integration_test.go |
Covers execution, rollback, locking, and convergence. |
pkg/executor/code.go |
Adds an uncertain RLS commit outcome code. |
pkg/executor/code_test.go |
Tests outcome-code classification. |
pkg/capabilities/capabilities.yaml |
Records Go API RLS support. |
pkg/capabilities/capabilities_test.go |
Updates capability validation expectations. |
docs/tcb-model.md |
Documents the RLS declaration proof boundary. |
docs/limitations.md |
Narrows remaining RLS limitations. |
docs/invariants.md |
Defines atomic RLS invariants. |
docs/execution-model.md |
Documents the new outcome code. |
docs/declarative-row-security.md |
Updates the RLS workflow and roadmap. |
docs/capabilities.md |
Regenerates the support matrix. |
docs/atomic-row-security.md |
Documents the new API and execution contract. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Signed-off-by: Armand Parajon <armand@squareup.com>
|
🤖 Review findings - created by Kiran's code review agent - for pg-sprite/pull/123, 5761822. Verdict: 4 findings — 1 blocking (refusals mis-typed as retryable), 2 non-blocking, 1 suggestion. BlockingEvery RLS refusal escapes as an unmapped sibling-package sentinel, so Non-blockingThe
General suggestionsFive new error returns propagate bare errors against the repo's mandated wrapping style. The one thing that could have broken, verifiedThe convergence gate could have been outside the lock or the transaction, letting a concurrent writer's policy change land unnoticed. It is not: Verified correct
This review was generated by Claude Code (claude-opus-5). |
Signed-off-by: Armand Parajon <armand@squareup.com>
|
🤖 Addressed the review findings in 1803dba.
Focused PostgreSQL tests, unit/safety checks, lint, and the pre-push race tests pass. CI is running on the new commit. The PR remains ready for review. Generated with Codex (GPT-6) |
|
🤖 Review findings - created by Kiran's code review agent - for pg-sprite/pull/123, 1803dba. Verdict: 3 findings — 1 blocking (untested under-lock recheck), 2 non-blocking. BlockingThe new under-lock ownership recheck has no test — delete it and the suite still passes. Non-blockingThe doc promises permanent The pre-lock gate covers table ownership only; the scratch-schema CREATE privilege is still discovered after ACCESS EXCLUSIVE is held. The one thing that could have broken, verifiedWhether the under-lock recheck is real protection or dead code: Verified correct
This review was generated by Claude Code (claude-opus-5). |
Signed-off-by: Armand Parajon <armand@squareup.com>
|
🤖 Addressed all three follow-up findings in 64d16a9.
The focused PostgreSQL suite, unit/safety checks, lint, and pre-push race tests pass. CI is running on the new commit; the PR remains ready for review. Generated with Codex (GPT-6) |
morgo
left a comment
There was a problem hiding this comment.
🤖 Reviewed on Morgan's behalf, at 64d16a97. Approving. The design is the right one — one transaction, lock first, derive the change from the locked catalog rather than from caller input, verify before commit — and the details hold up under attack: the inverted DiffWithRowSecurity contract is used correctly at both call sites, the shared search_path discipline is consistent across all three introspections, and the errgroup-free single-connection story is real. Five of seven genuine fault injections were caught.
The two that were not caught are both on the commit boundary, and that is what the notes below are about. Neither blocks.
1. The RS-2 convergence verification is unpinned — deleting it leaves the suite green
pkg/executor/row_security.go:156:
if _, err := schemadiff.DiffWithRowSecurity(schema, actual, wanted); err != nil {
return RowSecurityReport{}, fmt.Errorf("%w: RS-2: row security did not converge: %w", ErrInvariantViolation, err)
}Removing those three lines leaves every test in pkg/executor passing, including all 26 RowSecurity tests. grep confirms it: no test in the package asserts ErrInvariantViolation from this executor at all — the ErrInvariantViolation assertions in optimistic_integration_test.go and create_integration_test.go belong to the other two executors.
This matters more than an ordinary missing test for three reasons.
It is the check that makes the rendered SQL trustworthy. Everything executed here comes from RenderRowSecurity re-rendering the scratch catalog, not from replaying the caller's input. That is the right call, but it makes correctness depend on a render → execute → catalog round trip that nothing proves is lossless. The read-back is the only thing standing between a render bug and committing a policy set that is not the one that was asked for. The sibling check one line up (admitRowSecurityTable(schema, actual, wanted), "RS-2: target shape changed") has the same exposure.
What it protects is access control. A silently-wrong policy set is not a failed migration an operator notices; it is rows becoming visible or invisible to the wrong roles, committed, with a success report listing statements that all succeeded. RowSecurityReport would name exactly the DDL that ran, and every one of those statements would have returned without error.
The PR itself declares this as a test obligation. docs/invariants.md gives RS-2 as "Policy changes and convergence verification commit together or roll back | ExecuteRowSecurity; real DDL fault injection restores original policies". The atomicity half is genuinely covered — TestExecuteRowSecurityRollsBackAfterLiveDDLFailure and TestExecuteRowSecurityDeadlineRollsBackLiveDDL both prove the original policies survive. The verification half has nothing. A test that seeds a divergence between the applied result and wanted (a stubbed renderer, or a policy the re-render cannot reproduce) and asserts ErrInvariantViolation plus an unchanged live policy set would close it.
2. The unknown-commit-outcome wrapping is unpinned too
row_security.go:159:
if err := tx.Commit(ctx); err != nil {
return RowSecurityReport{}, &RowSecurityOutcomeUnknownError{Err: err}
}Replacing that with a bare return RowSecurityReport{}, err also leaves the whole package green. All three tests that mention the type — row_security_internal_test.go:42, code_test.go:27, code_test.go:167 — construct the error by hand and check how it classifies. None drives executeRowSecurity to a failing commit, so nothing proves the one site that produces it still does.
The consequence is the difference between the two pieces of advice the executor can give. docs/atomic-row-security.md says "A lost commit response is an unknown outcome: inspect the database before retrying. There are no automatic retries." Downgraded to an ordinary error, the same event reads as a plain failure that an orchestrator may retry — and a retry after a commit that actually landed re-derives from a converged target, so it is a harmless no-op. That is why this is a note rather than a finding: the failure mode is a misleading message and a lost instruction to go look, not a wrong policy set. It is still the one error in this file whose whole purpose is to change what a human does next.
3. The desired-state materialization runs inside the blocking window, and does not need to
executeRowSecurity orders the work: privilege check → LOCK TABLE ... ACCESS EXCLUSIVE → privilege recheck → introspect live → IntrospectDesiredWithRowSecurityTx → admit → DDL → verify → commit.
That fifth step is a full materialization of the declaration: CREATE SCHEMA, a search_path switch, every statement of the declaration replayed one at a time, a complete introspection of the scratch table (columns, constraints, indexes, policies), and a savepoint rollback. It runs with every reader and writer of the production table blocked — and it depends on nothing but desired. wanted never reads the live table; admitRowSecurityTable is the first thing that needs both.
Hoisting it above the LOCK TABLE would take roughly half the statements out of the blocking window at no cost to any invariant. RS-1 is about the live baseline and the derived change ("Read the live baseline only after taking the target lock"), which would still hold: live, the comparison, and every live statement stay after the lock. The savepoint's own locks are released on rollback either way, so nothing about the parent's ACCESS EXCLUSIVE changes.
The stronger argument is the one this PR already makes for itself one line above the lock:
// Reject missing owner or scratch privileges before taking an application-blocking lock.
if err := checkRowSecurityPrivileges(ctx, tx, schema, desired.Table()); err != nil {An invalid declaration is a far more likely refusal than a missing privilege, and it is far more expensive to discover. Today a typo'd column name, a missing role, or an unresolvable helper is found by classifyRowSecurityDesiredError after the application has already been blocked — and the blocking lasts until whichever statement fails. The same sentence that justifies the privilege pre-check justifies moving this above the lock with more force.
Notes
- The new capability row is the only
✅/t1row in the matrix that no front door can reach. I scanned all 54 rows:declarative-row-security-libraryis the sole entry withtier: t1,status_mark: "✅"and bothmigrateanddiffset torefused. Every other✅row is CLI-reachable, so✅has meant "the CLI does this" everywhere until now, and in the rendered matrix the mark is the scannable column while "(Go API only)" and "No CLI execution" live in the operation and notes text. The validator has no rule tying reachability to the mark, so nothing will flag it. Worth deciding deliberately rather than by omission —🟡with the notes as written would read closer to the truth for a CLI user, at the cost of understating it for a library consumer. Budget.validate()does not requireLockTimeout <= StatementTimeout, and the doc's error-code promise depends on it. With the ordering inverted (sayLockTimeout: 30s, StatementTimeout: 5s) the attempt context expires while theLOCK TABLEis still waiting, sorowSecurityErrortakes thecontext.DeadlineExceededbranch at line 69 beforeasBudgetErrorever sees a55P03, and lock contention reportsbudget-statement-exceeded.docs/atomic-row-security.mdsays "Lock exhaustion reportsbudget-lock-exceeded". Only a misconfigured budget reaches it — the documented example (100ms/5s) is fine — butvalidate()is already the place that refuses undecidable budgets, and this one is decidable there.- A deadline that expires before the commit is sent still reports
row-security-outcome-unknown.rowSecurityErrorchecks the unknown wrapper first, before the deadline classification, so atx.Commit(ctx)on an already-expired context — where pgx never puts aCOMMITon the wire and rollback is certain — sends the operator to inspect the catalog anyway. Failing toward "unknown" is the right direction and I would not change the ordering; noting it only because the over-report is invisible from the error text.
What I checked rather than took on trust
- The inverted
DiffWithRowSecurityusage, which reads like a bug and is not. Both call sites act onerr != nilrather than on a change set. That is correct: the function returns an error whenever the two models differ at all (row_security_roundtrip.go:160,:169), soerr == nil⟺ fully converged, and a non-empty-but-nil-error result is unrepresentable. By line 128admitRowSecurityTablehas already excludedErrDifferentTablesand the mixed-change error, so the only error reachable there is the RLS one. search_pathconsistency across the three introspections, which is where a spurious "did not converge" would come from. It holds by construction rather than by luck:introspectRowSecuritysetspg_catalog-only itself (row_security.go:64) and reads throughpg_catalog.pg_get_expr, so policy expressions render identically whatever the caller's path was.SET LOCALinside the savepoint is undone by its rollback, so the desired inspection cannot leak its scratch path into the verification.- RS-4's
pg_catalog-only replay path, twice, because my first attempt proved nothing. SwappingLocalSearchPath("pg_catalog")forLocalSearchPath(schema, "public")leaves every test green — but that is a correct green: PostgreSQL searchespg_catalogimplicitly first when it is not named, anddbconn.LocalSearchPathstrips a trailingpg_catalogfor exactly this reason (CO-9), so the shadowing case is not reachable through the helper at all. Bypassing the helper with a rawSET LOCAL search_path = <schema>, pg_catalogdoes reach it, andTestExecuteRowSecurityPreservesHelperResolutioncatches it immediately. The guard is real and it is tested; it is defended twice over. - The privilege query is the right one.
pg_has_role(current_user, c.relowner, 'USAGE')is the same test PostgreSQL's own ownership check uses, andhas_database_privilege(..., 'CREATE')is whatCREATE SCHEMAactually needs — the error message names the scratch schema as the reason, which is accurate. - Fault injection — five of seven. Post-lock privilege recheck removed →
TestExecuteRowSecurityRechecksOwnerAfterLockWait; theACCESS EXCLUSIVElock never taken →ReadsStateAfterWaitingForLock+RechecksOwnerAfterLockWait;admitRowSecurityTabledisabled → four refusal tests; theDROP POLICYsweep removed so policies are added rather than replaced → five tests;pg_catalogdemoted behind the target schema →PreservesHelperResolution. The RS-2 verification and the unknown-commit wrapper are the two above. (Two ~65s tests in unrelated files,TestRunSequenceWithProgressTracksStepsAndFinishesandTestExecuteCreateRefusesImplicitIndexNameCollision, failed intermittently during the injection sweep and pass at baseline — that is my harness running suites back to back against one database, not this branch.) go build ./...,go vet ./..., andpkg/executor,pkg/schemadiff,pkg/statement,pkg/capabilitiesare all green at this head against a live PostgreSQL, with no skips — all 26RowSecuritytests actually ran. CI: 16 checks, 0 failures, 0 pending. The branch isBEHINDmainand wants an update before merge.
Signed-off-by: Armand Parajon <armand@squareup.com>
|
🤖 Addressed the commit-boundary and blocking-window suggestions from this review in 17290fb.
For the other notes: retained ✅ for the implemented Go API and added prominent guidance that CLI reachability comes from the front-door columns. 🟡 currently means planned/tier 2 in the matrix contract, which would mislabel the implemented API. Clarified that the first timer reached wins, including an overall deadline during a lock wait, rather than adding a new restriction to the shared Budget type. Also documented the deliberately conservative classification of every commit error as unknown, including cancellation before transmission. Focused PostgreSQL tests, unit/safety checks, lint, and pre-push race tests pass. All three guard-removal experiments failed as expected, and the guards are restored. CI is running on the new commit. The latest main merge is included. Generated with Codex (GPT-6) |


Why
pg-sprite can compare row-level security (RLS) definitions but cannot yet apply them declaratively. Policy changes need to commit together so applications never see an intermediate set of access rules.
What
Add
executor.ExecuteRowSecurity, a Go API that applies a complete RLS definition to an existing supported table. It manages ENABLE/DISABLE, FORCE/NO FORCE, policies, and policy comments. CLI execution remains a follow-up.How
Everything uses one connection and a bounded transaction. Desired-state validation happens before the exclusive lock. Live SQL comes from the inspected catalog, and a matching definition performs no live DDL.
Risk
The exclusive lock blocks readers and writers; lock waits and the whole attempt have deadlines. Invalid declarations, unsupported table shapes, and insufficient privileges return permanent refusals. Failures before commit roll back the change; commit errors require catalog inspection before retrying.
A changed definition replaces the complete policy set, including unchanged policies. Grants, role membership, helper bodies, and authentication remain outside this operation. Orchestrators retain their existing replan and consent workflow.
Testing
Real PostgreSQL tests cover atomic rollback, concurrent changes, privilege checks, helper resolution, and unchanged catalog state after refusals. Fault injection verifies that policy/table divergence prevents commit and that commit failures preserve the unknown-outcome error. Removing each final verification guard or the commit-error wrapper makes its regression test fail.
Focused integration tests, unit/safety checks, lint, and pre-push race tests pass. No manual testing.
Bigger picture
Builds on #119's declarative RLS inspection and comparison. CLI execution and Supabase application-level validation are follow-ups.
Generated with Codex (GPT-6)