Fix segfault on DETACH DELETE under RLS with a function-based edge policy (#2474)

[BenSpex]

Summary

Fixes #2474.

A non-superuser subject to FORCE ROW LEVEL SECURITY crashed the backend with SIGSEGV when running MATCH (n) DETACH DELETE n on a vertex that has a connected edge, if the edge label carried an RLS policy whose USING/WITH CHECK qual invokes a function (e.g. a STABLE tenant/owner accessor). Reproduces on release_PG18_1.7.0 and current master.

Root cause / mechanism

AGE deletes a vertex's connected edges in check_for_connected_edges() (src/backend/executor/cypher_delete.c), which is called from end_cypher_delete() — i.e. during executor shutdown (ExecEndPlan, reached via PortalCleanup → ExecutorEnd when the portal is dropped). By that point the portal's active snapshot has already been popped, so the active-snapshot stack is empty.

RLS for Cypher DELETE is enforced at the executor level (added in #2309): check_for_connected_edges() compiles the edge label's security quals and evaluates them per candidate edge via check_security_quals() → ExecQual(). When the qual calls a SQL-language function, ExecQual() dispatches into fmgr_sql() → postquel_start(), which runs the function's query and reads the current snapshot with GetActiveSnapshot(). With no snapshot on the stack that is a NULL-pointer dereference → SIGSEGV. (A cassert build trips the Assert(ActiveSnapshotSet()) in postquel_start first — that assert is exactly the "caller should have ensured a suitable snapshot is active" contract.)

Backtrace (assert build) at the crash site:

Assertion failed: ActiveSnapshotSet(), functions.c
  postquel_start (functions.c)
  fmgr_sql
  ExecQual -> check_security_quals   (cypher_utils.c)
  process_edges_by_index             (cypher_delete.c)
  check_for_connected_edges          (cypher_delete.c)
  end_cypher_delete                  (cypher_delete.c)
  ExecEndCustomScan -> ExecEndPlan -> standard_ExecutorEnd

This explains the full narrowing in the issue:

• vertex path (process_delete_list) runs during normal execution while a snapshot is active → fine;
• edge-only DELETE and node-only DETACH DELETE never evaluate an edge qual at teardown → fine;
• superuser bypasses RLS → fine;
• a constant-only edge policy never enters fmgr_sql → fine;
• only DETACH DELETE of an edge-connected vertex under a function-bearing edge policy trips it.

Fix

Ensure an active snapshot for the duration of the connected-edge scan in check_for_connected_edges(). es_snapshot is still valid there (the EState is not torn down until this returns) and is the correct snapshot for reading the edges, so push it if none is active and pop it before returning. The scans themselves already pass es_snapshot explicitly, so this only affects RLS qual evaluation; non-RLS paths are unchanged. On an error path (e.g. an RLS denial raised mid-scan) transaction abort resets the active-snapshot stack, so the unpaired push is cleaned up.

if (!ActiveSnapshotSet())
{
    PushActiveSnapshot(estate->es_snapshot);
    pushed_snapshot = true;
}
...
if (pushed_snapshot)
    PopActiveSnapshot();

Test

Adds a regression to the security suite (PART 11b): an edge-label policy whose qual calls a STABLE SQL function, then DETACH DELETE of an edge-connected vertex as the RLS-bound role. It must delete the vertex and its edge (leaving the other endpoint) instead of crashing. The existing PART 11 uses a constant-only edge policy, which does not exercise the snapshot-dependent function path.

Verification

• A/B on an identical release-style build (PG18, -O2, --enable-cassert off): unpatched → client backend ... was terminated by signal 11: Segmentation fault; patched → deletes cleanly, no crash.
• make installcheck against PG18 (--enable-cassert) — all 43 tests pass, including the new case.
• The issue's function-based repro no longer crashes and deletes correctly.

[gregfelice]

Approving. I reproduced the crash, verified the fix, and reviewed the snapshot handling. The diagnosis in the description is correct and the fix is the right one.

Reproduced

Built current master (9fb7df8) against PostgreSQL 18.4 and ran this PR's new PART 11b case against it. The DETACH DELETE under the function-based edge policy kills the backend:

SET ROLE rls_user1;
SELECT * FROM cypher('rls_graph', $$
    MATCH (p:Person {name: 'DetachFn1'}) DETACH DELETE p
$$) AS (a agtype);
server closed the connection unexpectedly
	This probably means the server terminated abnormally
	before or while processing the request.
connection to server was lost

With this branch applied, the same test passes, and the full 41-test regress suite is green. So the new regress case is a genuine reproducer — it fails on master and passes here — which is exactly what a crash fix needs.

On the fix

Pushing estate->es_snapshot when the active-snapshot stack is empty is the right call, and es_snapshot is the correct snapshot to use — it is what the rest of the statement read the graph with, so connected-edge visibility stays consistent with the vertex scan that selected them.

I specifically checked the failure mode a PushActiveSnapshot/PopActiveSnapshot pair around a loop invites — an early exit between push and pop leaking the snapshot. There is none: check_for_connected_edges() has no return or goto between the two, only a break in an inner loop. The one non-local exit is the pre-existing RLS-denial ereport(ERROR), and the reasoning in the comment holds — AbortTransaction() runs AtAbort_Snapshot(), which resets the active-snapshot stack, so the unpaired push on that path is cleaned up. The if (!ActiveSnapshotSet()) guard also correctly makes this a no-op on any path that already has a snapshot, so nothing changes for the normal-execution callers.

The pushed_snapshot flag rather than an unconditional push/pop is the right shape, for the same reason.

One thing for the release, not for this PR

@jrgemignani — this is a backend SIGSEGV, and PG18/v1.8.0-rc0 is already tagged with artifacts staged in dev/age/PG18/1.8.0.rc0. So this either slips to 1.8.1 or it justifies respinning RC1. That is your call as RM, but I did not want a crash fix to land quietly while an RC is in flight — flagging it explicitly rather than letting the timing decide by default.

Nice write-up on the root cause, @BenSpex. The narrowing in the description (vertex path fine because it runs during normal execution with a snapshot active; edge-only DELETE and node-only DETACH DELETE fine because they never evaluate an edge qual at teardown; superuser fine because RLS is bypassed; constant-only policy fine because it never reaches fmgr_sql) matches what I see in the code, and it is what makes the LANGUAGE sql STABLE accessor in the test the right thing to exercise.

[MuhammadTahaNaveed]

lgtm

[gregfelice]

@jrgemignani — pulling a few release-adjacent items into one place so they're not scattered across threads.

1. Two PRs are reviewed and ready to merge:

• Fix segfault on DETACH DELETE under RLS with a function-based edge policy (#2474) #2475 (this one) — DETACH DELETE SIGSEGV under a function-based edge RLS policy. Approved; I reproduced the crash on current master and verified the fix + full regress suite. Cassert CI green on the head SHA.
• support openCypher-like relationsiop-type edge alternation #2468 — openCypher edge-type alternation [:A|B]. Approved with comments; all 42 regress tests pass, grammar adds no new conflicts. Two non-blocking fixes requested (CREATE/MERGE error wording, a wrong code comment) — your call whether those block merge or land as follow-up.

2. Release decision on #2475: this is a backend crash, and PG18/v1.8.0-rc0 is already tagged with artifacts staged in dev/age/PG18/1.8.0.rc0. So it's either a 1.8.1 item or a reason to respin RC1. Your call as RM — I just didn't want a segfault fix landing quietly against an in-flight RC.

3. PR-queue CI was stalled: the workflow runs on every recent contributor PR were sitting in action_required (unapproved), so they showed zero checks. I approved the pending runs across #2468/#2469/#2470/#2475/#2476, so those now have real CI signal. Result of that: #2469 (loader delimiter) is red — new SQL test cases without a regenerated expected file — I left the author a note.

Since I reviewed both of the above, they'll need another committer's hand on the merge button.