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

On Tue, Oct 20, 2026 at 8:30 PM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
> 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.

One more thought on the trade-off, against my own patch.

Deferring every free to the end of the transaction creates a cliff as
Tomas would call it. A transaction that invalidates and replans the
RI plan many times accumulates that many SPI plans and the
CachedPlanSources they point to, until commit. That’s not hard to
construct, and it hurts most where the plan is big, say a PK with
thousands of partitions where plancache went generic. Today’s code
frees those promptly, and so does Lucas's patch; mine would be a
regression there. I’ll also concede the per-row cost point:
GetCachedPlan() already does a fair amount of bookkeeping on that path
for every row, so the system accepts such costs for robustness.

I think my patch can get the same bound without the refcounting,
though. As posted, ri_FetchPreparedPlan() queues every invalid plan
until commit because it can’t tell whether a check up the stack is
executing it. A depth counter around SPI_execute_snapshot() in
ri_PerformCheck() fixes both halves: when it’s zero at detach time,
free immediately as today; and when it drops back to zero after an
execution, free whatever was queued meanwhile, so nothing outlives the
outermost check that was running, like this:

ri_check_depth++;
spi_result = SPI_execute_snapshot(...);
if (--ri_check_depth == 0 && ri_plan_dead_list != NIL)
ri_FreeDeadPlans(); /* same drain AtEOXact_RI() does */

An error inside the query can only leave the counter too high, which
just defers to AtEOXact_RI() for the rest of that transaction, so it
needs no PG_TRY or resource owner. Maybe I should try that.

--
Thanks, Amit Langote