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

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