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

Hi,

On Wed, Oct 21, 2026 at 12:44 PM Trakshan Mishra
<trakshanmishra477(at)gmail(dot)com> wrote:
>
> Hi Lucas, Amit,
>
> I ran Lucas's counterexample against Amit's v1, v1 plus the depth counter from Amit's second mail, and Lucas's v8, all on master e75fcc9867f (debugoptimized, cassert). To see the peak rather than the state after the statement, dummy_bd reads pg_backend_memory_contexts right after its DELETE FROM parent, while the outer cascade check is still running, and counts the SPI Plan and CachedPlan* contexts.
>
> rows v1 v1 + depth v8
> 1000 3024 / 7 MB 3024 / 7 MB 27 / 80 kB
> 5000 15024 / 34 MB 15024 / 34 MB 27 / 80 kB
> 20000 60024 / 137 MB 60024 / 137 MB
>
> So Lucas is right about this case. The depth counter does drain once the outer check returns (9 SPI plans left at the end of the statement), but the peak is the same as v1.

Yeah, I’ve come around to agreeing that the plans should be dropped as
soon as they become unnecessary, mainly to avoid introducing cliff
cases, which may not be as rare as I was thinking.

> I don't think that means we need the hash table though. What ri_FetchPreparedPlan() needs to know is whether this particular plan is being executed further up the stack, and the plans being executed are just the ri_PerformCheck() frames, usually one or two.

I tend to agree that a hash table is too big a hammer for our
requirement here, which as you wrote, is to tell if a plan is being
used anywhere in the stack. I believe the stack stays short in
practice, A chain of cascading FKs doesn’t require tracking of nested
plan use by itself, because the check’s query runs with fire_triggers
= false, so the RI triggers it fires are queued on the enclosing query
and run after the check returns. The only thing that can start another
check while one is still running is user code that runs during the
check’s query, like a BEFORE ROW trigger or a cast function, doing its
own DML. So the stack is as deep as that code nests, which is the
user’s own recursion, not something that grows with the number of rows
or the number of FKs.

So an array scanned linearly should be enough to tell whether a plan
is in use. A plan that turns out to be in use when it’s found invalid
goes on a separate list (ri_plan_dead_list in my patch), and gets
freed the next time a check finishes and it’s no longer in use.
Keeping that list separate from the array means a check that exits
with an error can just pop its array entry and leave the plan on the
list, instead of freeing it in the middle of error handling. While
that might be acceptable, keeping it simple seems better.

> I tried that on top of v1: ri_PerformCheck() pushes qplan on a small array before SPI_execute_snapshot() and pops it after. ri_FetchPreparedPlan() frees the stale plan right away unless it is on the array, and queues it otherwise. After the pop, queued plans that are no longer on the array are freed. An error can only skip a pop, so the depth can end up too high but never too low, and every live frame's entry stays below it. The worst case is a plan kept until AtEOXact_RI(), which resets the depth. No PG_TRY or resource owner, so it would be the same on every branch.
>
> With that:
>
> - the counterexample stays at 27 contexts / 80 kB at 1000 and 5000 rows, same as v8
> - regress and isolation pass with Lucas's v8-0001 test added
> - the fp_reentry test and the v8-0001 test under debug_discard_caches = 1 don't crash (neither do v1, v1 + depth or v8)
> - Lucas's benchmark script, release builds, 6 rounds, 300k rows / 60k roots, base is v1 (which has no per-row cost):
>
> array v8
> cascade_nochild -0.65% +2.36%
> cascade_child +0.93% +4.41%
> selfref_nested -0.11% +4.64%
>
> An A/A run on the same machine gave -0.97% to -0.06%, so the array version is within noise and v8 is a bit above it. My v8 numbers are somewhat higher than Lucas's 1.3-2.8%, probably the laptop.
>
> The diff against v1 is attached as a .txt, just to show the idea, not as a patch for cfbot.

Thanks for the patch. I'm going to study it and post an updated
version tomorrow. I'd like to handle errors as well as Lucas's patch
does but I'd stick to using PG_TRY in all the branches instead of
resowner in some and PG_TRY in others. I believe subtransaction
aborts are handled correctly by PG_TRY-based solution but I'd need to
test that. In any case, I want to avoid hash table and resowner calls
in the per-row path and keep the tracking code as minimal as possible.

Just to be upfront about credit, my plan is to tag Lucas as the main
author of this patch, since his report identified the problem and his
patch is what the rest of us have been refining, with you and me as
co-authors.

--
Thanks, Amit Langote