Repository navigation
Add RLS and SELECT privilege enforcement to global graph - #2561
Conversation
The global graph cache that backs variable-length edge (VLE) and shortest-path traversal is populated with a direct heap scan (table_beginscan + heap_getnext). That scan does not go through the planner or executor, so the row-level security policies and table privileges that MATCH receives automatically are not applied to traversal: it reads every vertex and edge regardless of the current role's policies or grants. Add age.enforce_rls_in_traversal (bool, PGC_SUSET, default on). When on: * SELECT privilege is checked on every label table before it is read (pg_class_aclcheck, with a pg_attribute_aclcheck fallback so column-level grants are honored the same way the executor honors them). * When RLS is active on a label for the current role, that label is loaded through SPI (SELECT ... FROM ONLY <label>) with a batched read-only cursor so the planner applies the policies. Otherwise the direct-scan path is used, now behind the privilege check. Properties are still fetched lazily by TID: only policy-visible rows are loaded, so the later heap_fetch reads only authorized tuples, which covers shortest_path as well. When off, the previous direct-scan behavior is restored, leaving operators a single escape hatch. To match MATCH's inner-join behavior, the edge loaders drop any edge whose start or end vertex was filtered out by a vertex policy rather than leaving a dangling edge in the cache. Cache validity: * A cache that applied RLS is not reused across statements (loaded_with_rls), because policies can depend on session state such as current_setting() that is not covered by the version, snapshot, role, and GUC cache keys. Graphs that do not use RLS are unaffected. * is_ggctx_invalid checks the role and GUC keys before the graph version-counter fast path, so a role or GUC change forces a rebuild even in DSM/SHMEM mode. * The version counter tracks only graph data changes, so an enforced non-RLS cache could otherwise survive a GRANT/REVOKE or policy/RLS DDL on a label table and serve stale permissions. A relcache callback keyed by label-table OID (table/column privileges, policies, RLS flags), plus pg_authid and pg_auth_members syscache callbacks (BYPASSRLS, role membership), now mark such caches stale so they are rebuilt and re-authorized. Re-entrancy and cleanup: * The context is built detached and attached only after a successful load, so a policy expression that re-enters traversal cannot observe a half-built cache, and a failed load frees the partial context instead of leaving it in TopMemoryContext. * On attach, an existing context for the same graph is reused only if it is still valid for the current role and GUC, so a re-entrant traversal reached through a SECURITY DEFINER policy cannot be handed a context loaded as another role. If discarding an incompatible context reports a missing entry, that error is raised after first freeing the freshly built context. * Each loader frees the transient label-name list from get_ag_labels_names() on both the normal and error paths; it is allocated in TopMemoryContext and would otherwise accumulate on every rebuild. Adds regression test rls_vle covering policy and privilege enforcement, the opt-out GUC, per-role cache invalidation, a session current_setting() policy, and privilege-metadata invalidation. Co-authored-by: GitHub Copilot (Claude Opus 5) <noreply@githubcopilot.com>
There was a problem hiding this comment.
🟡 Changes recommended
The new SPI-based loaders do not check for SPI_cursor_open() returning NULL, which can lead to NULL portal dereference during traversal cache load.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR hardens AGE’s global graph cache (used by variable-length edge traversal and related algorithms) so it enforces PostgreSQL SELECT privileges and row-level security (RLS) policies, aligning traversal visibility with what MATCH would allow.
Changes:
- Adds
age.enforce_rls_in_traversal(PGC_SUSET, defaulton) to gate SELECT-ACL checks and RLS-aware loading during traversal cache population. - Updates global-graph cache loading to (a) explicitly enforce SELECT privileges for direct heap scans and (b) route RLS-enabled label tables through SPI cursor-based reads so the planner applies policies; edges referencing RLS-filtered vertices are dropped to avoid dangling edges.
- Adds cache invalidation improvements (role/GUC keying, “never reuse across statements” when RLS was applied, and relcache/syscache callbacks for ACL/RLS/role-auth changes) plus a regression test covering the behavior.
File summaries
| File | Description |
|---|---|
| src/include/utils/ag_guc.h | Declares the new traversal enforcement GUC. |
| src/backend/utils/ag_guc.c | Defines age.enforce_rls_in_traversal (SUSET, default on). |
| src/backend/utils/adt/age_global_graph.c | Implements privilege/RLS enforcement, RLS-aware SPI loaders, cache keying & invalidation callbacks, and re-entrancy-safe attach behavior. |
| regress/sql/rls_vle.sql | Adds regression coverage for RLS + ACL enforcement, opt-out, per-role invalidation, and session-dependent policies. |
| regress/expected/rls_vle.out | Expected output for the new regression test. |
| Makefile | Adds rls_vle to the regression test suite. |
Review details
Suppressed comments (1)
src/backend/utils/adt/age_global_graph.c:931
- SPI_cursor_open() can return NULL (and set SPI_result) on failure; the subsequent SPI_cursor_fetch()/SPI_cursor_close() would then dereference a NULL portal and likely crash. Add an explicit NULL check and raise an ERROR with SPI_result_code_string(SPI_result).
/* read-only cursor: reuses the active snapshot; RLS applied by planner */
portal = SPI_cursor_open(NULL, plan, NULL, NULL, true);
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
I compared
The statistics crash and empty graph error come from older code that needs updating for the new filtering behavior. Please keep cached data alive until all operations using it have finished, and handle missing vertices and empty results. Each SQL file includes its own setup. Run the statistics case last on an isolated test instance because it reproduces backend termination. Review assisted by gpt-6-astra max 05-repeated-cache-loading.sql |
These will be addressed in the followup PRs. |
Finishing up the follow up. Thank you Taha! |
|
#2592 is the follow up as promised. Sorry for the delay, it took time to address the issues. |
A traversal points into a backend-local graph context that the next acquisition could free. A context loaded through RLS is never valid for a new acquisition, so when two traversals ran in one query the second one freed the first one's context. Interleaved traversals lost results, and materializing a path after its context was freed dereferenced a NULL pointer (review issue 1 on apache#2561). Pin a traversal's graph context to the query's per-query memory context. A pinned context that becomes invalid is retired: it is no longer offered to new acquisitions, and a memory-context callback frees it when the last query holding it ends, including after an error. Deleting the caches retires pinned contexts the same way. A VLE path is materialized from a context that holds all of its vertices and edges; when there is none, the materializers raise an error instead of dereferencing a missing context. Within one query, reuse a pinned context under the rules PostgreSQL gives a statement's scans, so that the policies run once for each row a load scans instead of again for every traversal. The 400-edge workload from the review evaluated its policy 160,000 times and now evaluates it 400 times, for stable and volatile policies alike: - A load made with another role, enforcement setting or snapshot is a different scan. - A later command id means that a traversal which has not loaded the graph yet must see the statement's own writes. The traversal that loaded it keeps it while the statement writes, as a traversal of a graph without RLS does. Pinning a new copy for every row would keep them all until the query ends. - Policies and privileges hold for the whole statement: PostgreSQL applies the policies when it plans the statement and checks the privileges when it starts it, so a change made while the statement runs does not end the reuse. - When the RLS quals call only IMMUTABLE functions, besides values such as current_user that cannot change unnoticed, any traversal in the query may share the graph. Otherwise only the traversal that loaded it reuses it, as when PostgreSQL rescans the result of a scan, and another traversal loads again: a STABLE function such as current_setting() can give another result once the statement changes the setting. A traversal is identified by its FmgrInfo, which stays the same when it runs again for another row. Policies from the row-security hooks become the same security quals as catalog policies, so they are inspected as well. RLS-filtered contexts are still never reused across statements. An RLS traversal in a DETACH DELETE query loaded its graph again for every row. A reload saw a deleted vertex whose edge was still visible, and the traversal failed with "add_valid_vertex_edges: no vertex found", while the same query without RLS succeeded. Keeping the graph makes the two return the same rows. Tests cover interleaved traversals, two RLS graphs materialized in one statement, policy evaluation counts for one and two traversals with immutable, stable and volatile policies across traversal shapes, a policy reading a setting the statement changes, the contexts left after each query, a statement's own write, a policy change and a role switch made while a statement runs, and traversals before and after a DETACH DELETE with and without RLS, including the graph copies held. They also build stored paths after the caches are deleted and for a role whose context lacks their edges, run queries that fail after their traversal, and delete the caches while a query holds its graph. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
vertex_stats() looked the vertex up in the traversal graph and used the entry without checking it. Since apache#2561, row-level security can hide a vertex from that graph while an untyped MATCH, which scans the parent label table, still returns it, so vertex_stats() dereferenced a NULL entry and terminated the backend (review issue 2 on apache#2561). Return null when the vertex is not in the graph the caller can traverse. The statistics of visible vertices are computed over that graph, so edges to hidden vertices are not counted. The new rls_vle part hides a vertex with a policy and checks the statistics of every vertex an untyped MATCH returns. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The RLS loaders read each visible row's ctid so that properties could be fetched from the heap later. A role granted SELECT on every user column of a label, but not on the table, can read the label with MATCH, yet a traversal failed with "permission denied" because column privileges do not cover ctid (review issue 3 on apache#2561). A role that holds SELECT on the table keeps the ctid and the lazy fetch. Otherwise the loader selects the properties column and the entry keeps an owned, detoasted copy, which is freed with the graph context. An entry stores either the tuple location or the properties in the same field, so vertex and edge entries keep their size. The column privileges are checked before the load as before, and the SPI query enforces them, so no row or column the role cannot read is loaded. The new rls_vle part grants only column privileges on an RLS-protected edge label and then on a vertex label, and checks that traversals return the expected paths and properties. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
A traversal raised "age_vle: empty graph" when the graph held no vertices. Since apache#2561 the traversal graph holds only the vertices the caller may see, so a policy that hides every vertex turned an ordinary query into an error, although MATCH over the same labels returns no rows (review issue 4 on apache#2561). Return no paths instead. The check runs before the traversal state is allocated, which reduces what a cached call on this path leaves in long-lived memory from 352 to 64 bytes. The remaining bytes come from parsing the graph name argument in the long-lived context. That predates PR apache#2561, also happens on every cache miss, and is left to separate hardening. The new rls_vle part hides every vertex and checks a variable-length MATCH, a zero-length lower bound and a direct age_vle() call. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
A traversal points into a backend-local graph context that the next acquisition could free. A context loaded through RLS is never valid for a new acquisition, so when two traversals ran in one query the second one freed the first one's context. Interleaved traversals lost results, and materializing a path after its context was freed dereferenced a NULL pointer (review issue 1 on #2561). Pin a traversal's graph context to the query's per-query memory context. A pinned context that becomes invalid is retired: it is no longer offered to new acquisitions, and a memory-context callback frees it when the last query holding it ends, including after an error. Deleting the caches retires pinned contexts the same way. A VLE path is materialized from a context that holds all of its vertices and edges; when there is none, the materializers raise an error instead of dereferencing a missing context. Within one query, reuse a pinned context under the rules PostgreSQL gives a statement's scans, so that the policies run once for each row a load scans instead of again for every traversal. The 400-edge workload from the review evaluated its policy 160,000 times and now evaluates it 400 times, for stable and volatile policies alike: - A load made with another role, enforcement setting or snapshot is a different scan. - A later command id means that a traversal which has not loaded the graph yet must see the statement's own writes. The traversal that loaded it keeps it while the statement writes, as a traversal of a graph without RLS does. Pinning a new copy for every row would keep them all until the query ends. - Policies and privileges hold for the whole statement: PostgreSQL applies the policies when it plans the statement and checks the privileges when it starts it, so a change made while the statement runs does not end the reuse. - When the RLS quals call only IMMUTABLE functions, besides values such as current_user that cannot change unnoticed, any traversal in the query may share the graph. Otherwise only the traversal that loaded it reuses it, as when PostgreSQL rescans the result of a scan, and another traversal loads again: a STABLE function such as current_setting() can give another result once the statement changes the setting. A traversal is identified by its FmgrInfo, which stays the same when it runs again for another row. Policies from the row-security hooks become the same security quals as catalog policies, so they are inspected as well. RLS-filtered contexts are still never reused across statements. An RLS traversal in a DETACH DELETE query loaded its graph again for every row. A reload saw a deleted vertex whose edge was still visible, and the traversal failed with "add_valid_vertex_edges: no vertex found", while the same query without RLS succeeded. Keeping the graph makes the two return the same rows. Tests cover interleaved traversals, two RLS graphs materialized in one statement, policy evaluation counts for one and two traversals with immutable, stable and volatile policies across traversal shapes, a policy reading a setting the statement changes, the contexts left after each query, a statement's own write, a policy change and a role switch made while a statement runs, and traversals before and after a DETACH DELETE with and without RLS, including the graph copies held. They also build stored paths after the caches are deleted and for a role whose context lacks their edges, run queries that fail after their traversal, and delete the caches while a query holds its graph. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
vertex_stats() looked the vertex up in the traversal graph and used the entry without checking it. Since #2561, row-level security can hide a vertex from that graph while an untyped MATCH, which scans the parent label table, still returns it, so vertex_stats() dereferenced a NULL entry and terminated the backend (review issue 2 on #2561). Return null when the vertex is not in the graph the caller can traverse. The statistics of visible vertices are computed over that graph, so edges to hidden vertices are not counted. The new rls_vle part hides a vertex with a policy and checks the statistics of every vertex an untyped MATCH returns. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The RLS loaders read each visible row's ctid so that properties could be fetched from the heap later. A role granted SELECT on every user column of a label, but not on the table, can read the label with MATCH, yet a traversal failed with "permission denied" because column privileges do not cover ctid (review issue 3 on #2561). A role that holds SELECT on the table keeps the ctid and the lazy fetch. Otherwise the loader selects the properties column and the entry keeps an owned, detoasted copy, which is freed with the graph context. An entry stores either the tuple location or the properties in the same field, so vertex and edge entries keep their size. The column privileges are checked before the load as before, and the SPI query enforces them, so no row or column the role cannot read is loaded. The new rls_vle part grants only column privileges on an RLS-protected edge label and then on a vertex label, and checks that traversals return the expected paths and properties. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
A traversal raised "age_vle: empty graph" when the graph held no vertices. Since #2561 the traversal graph holds only the vertices the caller may see, so a policy that hides every vertex turned an ordinary query into an error, although MATCH over the same labels returns no rows (review issue 4 on #2561). Return no paths instead. The check runs before the traversal state is allocated, which reduces what a cached call on this path leaves in long-lived memory from 352 to 64 bytes. The remaining bytes come from parsing the graph name argument in the long-lived context. That predates PR #2561, also happens on every cache miss, and is left to separate hardening. The new rls_vle part hides every vertex and checks a variable-length MATCH, a zero-length lower bound and a direct age_vle() call. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…2593) Since #2592, a query reuses an RLS-filtered graph across its traversals when a walker over the planned row-security quals judges that the policies cannot change during the statement. The walker copies PostgreSQL's contain_mutable_functions_walker, treats SQL value functions such as CURRENT_DATE and current_user as fixed, and reads the quals through SPI_plan_get_plan_sources(), which PostgreSQL leaves undocumented. The classification is not reliable. CURRENT_DATE, CURRENT_TIME, LOCALTIME and LOCALTIMESTAMP depend on the session TimeZone, which a statement can change with set_config(), and the inspected quals do not contain function default arguments, which the planner inserts later. A policy comparing CURRENT_DATE with an immutable date expression showed 5 links to the first of two traversals and, after the second switched the time zone, the same 5 links to the second, where two scans of the label return 5 and 0. The walker also uses node types that PostgreSQL 16 and earlier do not have, so it cannot be backported unchanged, and every new node type must be tracked by hand. Remove the walker and reuse a graph loaded through RLS only for the traversal that loaded it, as already done for policies the walker classified as varying. A traversal still loads the graph once instead of once per row, so the 400-edge workload from the #2561 review still evaluates its policy 400 times instead of 160,000. A query with several traversals of one RLS-protected graph loads it once per traversal. Graphs loaded without RLS are unaffected. In rls_vle 8.3, two traversals with an immutable policy, alone and with current_user, now evaluate it 40 times instead of 20; the other counts are unchanged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The global graph cache that backs variable-length edge (VLE) and shortest-path traversal is populated with a direct heap scan (table_beginscan + heap_getnext). That scan does not go through the planner or executor, so the row-level security policies and table privileges that MATCH receives automatically are not applied to traversal: it reads every vertex and edge regardless of the current role's policies or grants.
Add age.enforce_rls_in_traversal (bool, PGC_SUSET, default on). When on:
SELECT privilege is checked on every label table before it is read (pg_class_aclcheck, with a pg_attribute_aclcheck fallback so column-level grants are honored the same way the executor honors them).
When RLS is active on a label for the current role, that label is loaded through SPI (SELECT ... FROM ONLY ) with a batched read-only cursor so the planner applies the policies. Otherwise the direct-scan path is used, now behind the privilege check. Properties are still fetched lazily by TID: only policy-visible rows are loaded, so the later heap_fetch reads only authorized tuples, which covers shortest_path as well.
When off, the previous direct-scan behavior is restored, leaving operators a single escape hatch.
To match MATCH's inner-join behavior, the edge loaders drop any edge whose start or end vertex was filtered out by a vertex policy rather than leaving a dangling edge in the cache.
Cache validity:
A cache that applied RLS is not reused across statements (loaded_with_rls), because policies can depend on session state such as current_setting() that is not covered by the version, snapshot, role, and GUC cache keys. Graphs that do not use RLS are unaffected.
is_ggctx_invalid checks the role and GUC keys before the graph version-counter fast path, so a role or GUC change forces a rebuild even in DSM/SHMEM mode.
The version counter tracks only graph data changes, so an enforced non-RLS cache could otherwise survive a GRANT/REVOKE or policy/RLS DDL on a label table and serve stale permissions. A relcache callback keyed by label-table OID (table/column privileges, policies, RLS flags), plus pg_authid and pg_auth_members syscache callbacks (BYPASSRLS, role membership), now mark such caches stale so they are rebuilt and re-authorized.
Re-entrancy and cleanup:
The context is built detached and attached only after a successful load, so a policy expression that re-enters traversal cannot observe a half-built cache, and a failed load frees the partial context instead of leaving it in TopMemoryContext.
On attach, an existing context for the same graph is reused only if it is still valid for the current role and GUC, so a re-entrant traversal reached through a SECURITY DEFINER policy cannot be handed a context loaded as another role. If discarding an incompatible context reports a missing entry, that error is raised after first freeing the freshly built context.
Each loader frees the transient label-name list from get_ag_labels_names() on both the normal and error paths; it is allocated in TopMemoryContext and would otherwise accumulate on every rebuild.
Adds regression test rls_vle covering policy and privilege enforcement, the opt-out GUC, per-role cache invalidation, a session current_setting() policy, and privilege-metadata invalidation.