Re: [PATCH] Fix segmentation fault caused by reentrancy in RI_Fkey_cascade_del (ri_triggers.c)

Hi,

On Sat, Oct 17, 2026 at 2:57 AM Lucas Jeffrey
<lucas(dot)jeffrey(at)anachronics(dot)com> wrote:
> Hi Tomas,
>
> Thanks for your reply. I attached a v8 with patches for all the supported
> branches:
>
> - v8-0001/0002: master, no changes from v7, just rebased on current master.
> - nocfbot-v8-0001/0002-PG19: same as master.
> - nocfbot-v8-0001/0002-PG14 to PG18: same test as master, and the same fix
> on the five branches, with two differences:
>
> 1) ResourceOwnerDesc doesn't exist before 17, so ri_PerformCheck() wraps
> the execution in PG_TRY(), and the catch block drops the count of that
> execution and re-throws. I used PG_TRY() on 17 and 18 too, so that
> the five branches get the same patch. On master I kept the resource
> owner, since SPI already pins the CachedPlan it executes that way, and
> it warns if a code path ever misses the decrement.
>
> 2) AtEOXact_RI() doesn't exist before 19, so a plan whose freeing was
> deferred and whose last execution was aborted is freed in the next call to
> ri_PerformCheck().
>
> Without the fix, the test crashes the backend on 14, 15, 18, 19 and master.
> On 16 and 17 it doesn't crash in my builds, although the plan is freed while
> in use (I checked it with some logging), since it depends on how the freed
> memory gets reused. With the fix, regress and isolation pass on all
> branches.
>
> For the benchmark I used release builds of master with and without the
> patch, one client, 10 rounds alternating both builds, and took the average:
>
> test case base patched diff
> cascade, no children (1M) 4642 ms 4772 ms +2.8%
> cascade, one child (1M) 8046 ms 8251 ms +2.6%
> self-ref, nested (200k) 7168 ms 7258 ms +1.3%
> self-ref, nested + inval (20k) crash 115705 ms
>
> The first two are the common case, a cascade with no nesting. The last two
> are the case of the test: a BEFORE DELETE trigger on a self-referencing
> table reenters the cascade, so the refcount table is not empty and
> ri_PreparedPlanFreeDeferred() scans it on every nested call; in the last one
> the trigger also invalidates the plan, which is the crash without the fix.
>
> The setup minimizes I/O (unlogged tables, fsync and autovacuum off) to focus
> on the cost of the patched code, so with real I/O the difference should be
> much smaller. The scripts are attached.
>
> The scripts used for benchmarking are attached.

Thanks for all your work on this, and for notifying me about this on
the thread where I posted a patch for basically the same issue, found
by the buildfarm [1].

I’m attaching that patch here too, since it takes a different
approach. It defers freeing an invalid SPI plan to end of transaction
by collecting such plans in a list that AtEOXact_RI() frees, much like
170c9344def does for FastPathMeta objects whose immediate freeing
caused the same kind of problem. Your 0001 test passes with it. It’s
also the same patch across every branch. The only variation is that
branches before 19 need AtEOXact_RI() added, with just this cleanup in
it, and called from the same three places in xact.c.

Reading your 0002, if I understand it right, the refcounting allows
you to free a detached plan as soon as its last execution finishes
rather than at commit as with my patch. The hash table, the resource
owner callback (PG_TRY on 14-16) are there to keep that count correct
across errors and subtransaction aborts. My hunch is that this
approach requires more machinery than freeing a plan a bit earlier is
worth. This only matters when one transaction invalidates and replans
the same RI plan many times, which doesn’t seem like a common
scenario. The cost of waiting until commit in that case is memory
usage. There’s also some CPU cost as the patch stands, because the
detached SPI plans’ CachedPlanSources stay on the plan cache’s
saved-plan list until commit, so later invalidations in the
transaction have to rescan them. The latter is easy to avoid by
unlinking them from that list when the SPI plan is detached, which I’d
do in a follow-up if we proceed this way.

I’m also not keen on what this adds to every ri_PerformCheck():

@@ -2719,11 +2764,17 @@ ri_PerformCheck(const RI_ConstraintInfo *riinfo,
* Set fire_triggers to false to ensure that AFTER triggers are queued in
* the outer query's after-trigger context and fire after all RI updates
* on the same row are complete, rather than immediately.
+ *
+ * Triggers fired by the query may reach ri_FetchPreparedPlan() for the
+ * same query key and find the plan invalid, so tell it that the plan is
+ * in use while it runs.
*/
+ ri_PreparedPlanExecutionStarted(qplan);
spi_result = SPI_execute_snapshot(qplan,
vals, nulls,
test_snapshot, crosscheck_snapshot,
false, false, limit);
+ ri_PreparedPlanExecutionFinished(qplan);

That’s two hash lookups and a resource owner remember/forget per row
(a PG_TRY per row on 14-16), paid by every FK check to handle a case
that almost none of them are in. Your numbers put it at 2-3%, IIUC,
which isn’t large, but the deferral approach doesn’t touch this path
at all.

--
Thanks, Amit Langote

[1] https://www.postgresql.org/message-id/CAObkL7NeQPxj7SBi3u5k%3DMKtaZUPoqgyehiwug626sjD2fo9ZQ%40mail.gmail.com