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

On 11/3/26 14:41, Amit Langote wrote:
> 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:
>>>>
>>>> ...
>>>>
>>
>> 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.
>

Yeah, that should work, I think. There may be cases when the transaction
is interrupted or the process got interrupted.

>
>> 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.
>

Makes sense. I sometimes struggle with writing a commit message with
just the right amount of detail - not too much, not too little. But
considering how useful commit messages are while chasing bugs, I'm fine
with erring on the side of more detail. (Our commit messages are so much
better than most other repositories I had to deal with ...).

--
Tomas Vondra