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

Hi Amit, Tomas

I tested and reviewed v11, I think comments are fine, and all the issues
Tomas raised were addressed.

+1 to bringing the Assert back.

I tested v11 on master and REL_14 through REL_19, built with cassert,
and make check passes on all of them. LGTM, I think it's ready to
commit.

Thanks!

Regards,
Lucas Jeffrey

El mar, 3 nov 2026 a las 10:41, Amit Langote
(<amitlangote09(at)gmail(dot)com>) escribió:
>
> On Tue, Nov 3, 2026 at 2:39 AM Tomas Vondra <tomas(at)vondra(dot)me> wrote:
> > 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.
>
> Thanks for chiming in and for the review.
>
> > 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.
>
> Actually, there used to be an Assert there that I removed, because I
> believed that PG_FINALLY() would not get a chance in some cases (FATAL
> errors) to reset the stack and the Assert would fail. AtEOXact_RI()
> itself already sets it to 0 so ri_FreeDeadPlans() can free all plans
> in ri_plan_dead_list.
>
> I've now revised the Assert to this:
>
> /*
> * No RI check can be in progress here, so clear the stack. On commit it
> * must be empty already, since ri_PerformCheck() pops its plan whether
> * the query returns or throws. On abort it may not be, because a FATAL
> * error skips those pops.
> */
> Assert(ri_executing_depth == 0 || !isCommit);
>
> As for why nothing failed, commenting out "ri_executing_depth =
> exec_depth;" means nothing is ever popped, so every plan invalidated
> during the transaction is held in ri_plan_dead_list until
> AtEOXact_RI() frees it at the end, which is fine but it means that for
> a long statement where every row uses a new plan after the old one is
> invalidated, many such plans will accumulate. The stack approach
> avoids that accumulation, but none of that is observable with a
> regression test.
>
> > 2) On 14, the first test says "Unlike the case above", but that test is
> > missing on 14.
>
> You're right. The test case it's talking about is only present in 19
> and master because it got added with fast-path work.
>
> Fixed the comment in the patches for 14-18 like this:
>
> --- Unlike the case above, this reproduces the crash without cache clobbering.
> +-- This reproduces the crash without cache clobbering.
>
> > 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"?
>
> Ok, I chose wording similar in meaning but matching nearby text:
>
> * returns. If the plan has been invalidated in between, freeing it here
> * would pull the SPIPlan out from under the outer _SPI_execute_plan(). So
> * in that case queue it on ri_plan_dead_list, to be freed once no check
> - * is in progress.
> + * is executing it any more.
>
> > 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.
>
> I think it's better to have the details about both Lucas's report and
> Tom's buildfarm report, but I agree it's not great to have the same
> message in all the branches. The message for 19 and master keeps the
> buildfarm details, because the history matters there. Checks needing
> a cast function went through the fast path, which didn’t have this
> bug, until 6aecabcd5ed moved them to SPI, and that is what made
> trilobite fail. Older branches never had that history but they still
> would have crashes under CLOBBER_CACHE_ALWAYS if the test case had
> existed, so their message only says how the bug was found.
>
> --
> Thanks, Amit Langote