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

On Wed, Nov 4, 2026 at 2:30 AM Lucas Jeffrey
<lucas(dot)jeffrey(at)anachronics(dot)com> wrote:
> 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.
> >
>
> 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.

Pushed after trimming the commit messages a bit to reduce duplication
of the bug description text.

Thank you all for sharing your thoughts and for reviews.

--
Thanks, Amit Langote