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

On 10/26/26 10:23, Amit Langote wrote:
> 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.
>

I like this approach. It seems to be bit simpler than the reference
counting approach, and it has the advantages that you explained. Plus it
has the advantage of working on all branches. I can't think of a clear
reason to stick to the reference counting.

A couple comments from a quick review:

1) Isn't it a bit weird all regression tests pass even if I comment out
the "ri_executing_depth = exec_depth;" line in ri_PerformCheck? There's
a comment in the foreign_key test that says:

... must leave the stack of executing RI plans exact (an assertion
at end of transaction checks that) ...

So how come it didn't catch this? Where's the assertion that comment
references, actually? I don't see any assertions added to AtEOXact_RI.

2) On 14, the first test says "Unlike the case above", but that test is
missing on 14.

3) The comment in ri_FetchPreparedPlan says:

... to be freed once no check is in progress.

Maybe it should instead say "once no check is executing the plan"?

4) The commit message seems to have a lot of details related to
buildfarm animals, fast-path checks (which only affects 19/master), etc.
Maybe that's too much detail, esp. for backbranches. Not sure.

regards

--
Tomas Vondra