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

On Sat, Oct 24, 2026 at 4:01 PM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
>
> On Fri, Oct 23, 2026 at 10:26 PM Lucas Jeffrey <lucas(dot)jeffrey(at)anachronics(dot)com> wrote:
> > Hi Amit,
> >
> > Thanks for v10, and for the credit in the commit message.
> >
> > I tested v10 on master and on REL_14 through REL_19, built with
> > cassert, and make check passes on all of them, including the new
> > tests in foreign_key. It addresses everything we found, so it LGTM.
> >
> > I found just one small thing, non-locking: the new test comment in
> > foreign_key.sql (and in foreign_key.out) says "(an assertion at end
> > of transaction checks that)", but that assertion was removed in v10,
> > so I would remove that part of the comment before commiting
> > (doesn't needs a v11).
>
> Thanks for reporting that. Will fix.

Tomas, given that you have looked at Lucas's patch and said it's on
the right track, I thought to check whether you like the patch after I
changed it in the following ways:

* Instead of a hash table to track running SPI plans, it uses an array
to represent a stack of running plans. ri_PerformCheck() pushes the
plan onto the stack before calling SPI_execute_snapshot() and pops it
once it returns normally or if an error occurs as described below

* Instead of using ResourceOwner to handle the pop during transaction
or subtransaction unwinding, which is only available in 17 and later,
so Lucas's patch already used PG_TRY in the older branches, the
SPI_execute_snapshot() call is wrapped in PG_TRY/PG_FINALLY, so any
error during that call pops the plan from the stack as well. That
makes the fix the same in all branches.

I chose this approach because the hash lookup and the resource owner
entry add a fixed cost to every check, on the normal path where no
tracking is needed, and I'd rather not add that cost to older
branches, even if it's small. The stack array approach adds depth (or
array size) check, a stack index increment, an array store and a
decrement, plus the PG_TRY, which Lucas's patch needed in the older
branches anyway. It also ends up being less code overall.

If you think there are any reasons to stick to the original approach,
please let me know. If not, I'd like to commit v10 in the next couple
days after doing the fixes mentioned by Lucas.

--
Thanks, Amit Langote