Repository navigation
Pr2561 rls followup v4 - #2592
Merged
MuhammadTahaNaveed merged 5 commits intoOct 12, 2026
Merged
Conversation
jrgemignani
requested review from
MuhammadTahaNaveed and
gregfelice
and
a balanced review from Copilot
October 12, 2026 17:32
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>
The traversal graph loads RLS-protected labels through an SPI query, so the policies are evaluated in whichever process loads the graph. vertex_stats() and age_graph_stats() are parallel safe, so under a parallel plan a worker loaded the graph. A worker does not have all of the caller's session state; inet_client_addr(), for one, is NULL there. A policy that hid edges from a client connection therefore admitted them in the worker, and the statistics counted edges the caller could not see. Raise an error (SQLSTATE 25000) instead of loading an RLS-protected label in parallel mode. Roles the policies do not apply to, labels without RLS, serial plans and age.enforce_rls_in_traversal = off are unaffected. The function declarations stay parallel safe, so other queries keep their parallel plans. The new rls_vle part forces a parallel plan and checks that the restricted role is refused while the owner and a serial plan still get statistics. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
jrgemignani
force-pushed
the
pr2561-rls-followup-v4
branch
from
October 12, 2026 17:47
6392ef9 to
d205f26
Compare
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
3 open findings
What changed in this PR
This PR refines Apache AGE VLE traversal behavior around graph-context lifetime and reuse, particularly under RLS, to better match PostgreSQL scan semantics and to avoid dereferencing missing graph state during delayed path materialization.
Changes:
- Add query-lifetime pinning/retirement for graph global contexts and reuse rules keyed to snapshot/role/call-site for RLS scans.
- Make VLE path/edge materialization locate a context that still contains all referenced entities and error cleanly if none exists.
- Expand RLS handling (column-privilege loads without
ctid, disallow parallel-mode RLS loads) and add extensive regression coverage.
| File | Description |
|---|---|
| src/include/utils/age_global_graph.h | Exposes new APIs for acquiring/pinning contexts and finding a context suitable for path materialization. |
| src/backend/utils/adt/age_vle.c | Pins traversal contexts to per-query memory; avoids erroring on fully hidden graphs; materialization now validates referenced entities exist. |
| src/backend/utils/adt/age_global_graph.c | Implements pin/retire lifecycle, RLS qual-variance detection, column-privilege property storage, and parallel-mode refusal for RLS loads. |
| regress/sql/rls_vle.sql | Adds Parts 8–12 covering context lifetime, reuse semantics, column-privilege loads, empty-visible graphs, and parallel restrictions. |
| regress/expected/rls_vle.out | Updates expected output for the new regression sections and error messages. |
🧠 Review effort: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
MuhammadTahaNaveed
approved these changes
Oct 12, 2026
Copilot stopped reviewing on behalf of
jrgemignani due to an error
October 12, 2026 18:26
jrgemignani
added a commit
that referenced
this pull request
Oct 13, 2026
…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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This five-commit follow-up to #2561 improves VLE graph lifetime, scan
semantics, and RLS handling. It has no SQL, catalog, extension-version, or
function-declaration changes.
Changes
Lifecycle and correctness
contexts are retired and reclaimed after the last query pin is released,
including error cleanup.
referenced vertex and edge. If none is available, raise a runtime error
rather than dereferencing missing graph state.
semantics. The loading traversal retains its scan across the statement's
own writes. Separate traversals share a graph only when the applied RLS
quals are fixed for the statement.
traversal graph.
RLS hardening
access to ctid. These loads retain owned property values.
reproduce all session-dependent policy state. Serial execution and labels
without RLS are unchanged.
Performance
The 400-edge review workload previously evaluated its policy 160,000 times.
It now evaluates it 400 times. A 200-row RLS DETACH DELETE workload retains
one graph copy instead of one per row and completed in approximately half the
time in local measurements. Non-RLS VLE medians remained within 1.3% of the
comparison revision.
Tests and validation
rls_vle Parts 8-12 cover:
PostgreSQL 18.3:
--enable-cassert,-O0) and release (-O2);-Werrorbuilds;pg_trgm;
New regression sections were also checked red/green against their preceding
commits.
Scope