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

Hi Trakshan,

Thanks for your review. I attached a v7 that addresses both of your
comments:

1. The comment above RI_QueryPlanCacheExecutingRefCountEntry is now
rewrapped; the text is unchanged.

2. The comment on ri_PreparedPlanFreeDeferred() now says that the
table is empty unless an RI query is already being executed or a
plan left by an aborted execution is still pending, consistent with
my previous mail.

Both changes are limited to comments; there are no code changes, I also
rebased the patches
on current master (158b6d5109f).

Regards,
Lucas Jeffrey

El vie, 9 oct 2026 a las 0:32, Trakshan Mishra (<trakshanmishra477(at)gmail(dot)com>)
escribió:

> Hi Lucas,
>
> Thanks for v6. I retested it, with most of the attention on the new
> deferred-free path, since that is the part that changed.
>
> Test environment:
> master @ 1b5dd3a24a4, plus v6-0001 and v6-0002
> Linux x86_64, gcc 15.2.0
> meson, --buildtype=debug -Dcassert=true
>
>
> == Testing ==
>
> Both patches apply cleanly with no whitespace errors, and the build has
> no warnings.
>
> Full "meson test" with v6: 362 ok, 0 failed, 50 skipped.
>
> With only v6-0001 applied, the new test still segfaults the backend on
> the cascade, and foreign_key plus the tests after it fail (52 in
> total), as with v5.
>
> As last time, I added temporary LOG lines where a plan is deferred,
> left pending after an abort, and freed, to see which path frees each
> plan:
>
> - The DO block in the test: 12 plans deferred, 10 left pending by
> the aborted executions. 9 of those are freed by the next
> iteration's RI query and the last one by AtEOXact_RI() at commit.
> The other 2 are freed when their last execution finishes normally.
>
> - Top-level abort, 50 times, then an invalidation and a successful
> cascade: all 50 pending plans are freed by AtEOXact_RI() on the
> abort path. No crash and no assertion failure.
>
> - SAVEPOINT / ROLLBACK TO, then COMMIT with no further RI query: the
> plan is freed at commit, and the Assert that the table is empty
> holds.
>
> - New for v6: a case where a pending plan is freed by the hash scan
> while another RI execution is still running. An outer ON DELETE
> CASCADE on a different FK fires a BEFORE DELETE trigger that runs
> the failing self-referencing cascade three times, each inside an
> exception block. The scan sees two entries, skips the outer plan
> (refcount 1), and frees the pending one; the third pending plan
> goes at AtEOXact_RI(). The outer delete completes and the inner
> deletes are rolled back as expected. So the refcount > 0 branch
> and removing the current entry during hash_seq_search() both get
> exercised, and nothing complained.
>
>
> == Comments ==
>
> All three of Tomas's points look addressed to me. Scanning the
> refcount table instead of keeping a separate list is simpler, and the
> early return on an empty table keeps the common path cheap. Two small
> things:
>
> 1. The comment above RI_QueryPlanCacheExecutingRefCountEntry has a line
> that is 91 characters long ("error, until the next RI query execution
> or the end of the transaction. A plan is never"); it probably just
> needs rewrapping after the rename.
>
> 2. The comment on ri_PreparedPlanFreeDeferred() says the table is empty
> outside of nested RI checks. It can also hold pending entries after an
> aborted execution, until the next ri_PerformCheck() or the end of the
> transaction, which your mail describes correctly. Maybe worth saying
> the same in the comment.
>
> Doing master first and a separate, smaller fix for 14-18 sounds
> reasonable to me.
>
>
> Regards,
> Trakshan Mishra
>
> On Fri, Oct 09, 2026 12:56 AM, Lucas Jeffrey <
> lucas(dot)jeffrey(at)anachronics(dot)com> wrote:
>
>> Hi Tomas,
>>
>> Thanks for your review. I attached a v6, rebased on current master and
>> with some changes handling some items in your review.
>>
>> 1) Agreed on the naming, there were three different terms for the same
>> thing. I used "free" everywhere, but since the plan is not always freed
>> immediately, the names now say that explicitly:
>>
>> ri_PreparedPlanReleaseASAP() -> ri_PreparedPlanFreeOrDefer()
>> markedForDeletion -> freeDeferred
>> ri_PreparedPlanFreeUnused() -> ri_PreparedPlanFreeDeferred()
>>
>> The comments and the commit message were updated to use the same
>> terms. The only remaining "Release" is ResOwnerReleaseRIPlanExecution(),
>> which follows the naming convention of the other resource owner
>> callbacks (ResOwnerReleaseBuffer(), ResOwnerReleaseCachedPlan(), etc.),
>> so I kept it.
>>
>> 2) Agreed, the "free later list" was not necessary, and I removed it. To
>> be
>> precise, those plans are no longer in ri_query_cache, since
>> ri_FetchPreparedPlan() removes them from it, but their entries remain in
>> the refcount hash table, ri_query_plan_cache_executing_refcount, with a
>> zero count. ri_PreparedPlanFreeDeferred() now scans that table instead.
>>
>> That table only has entries while RI queries are being executed, so it
>> stays small. However, ri_PreparedPlanFreeDeferred() is called on every
>> ri_PerformCheck(), so it returns immediately when the table is empty,
>> which is the case unless an RI query is already being executed or a
>> deferred plan is pending. This way the common path does not pay for a
>> hash_seq_search() over all the buckets. I also reduced the initial size
>> of the table to 16, instead of RI_INIT_QUERYHASHSIZE (256).
>>
>> 3) Yes, all supported branches are affected. However, this fix cannot be
>> backpatched as is: the ResourceOwnerDesc API exists only since 17, and
>> AtEOXact_RI() is new in 19. I would propose to agree on the fix for
>> master first, and then submit a separate, less invasive patch for 14-18,
>> using only the APIs and functions available in those branches.
>>
>> Regards,
>> Lucas Jeffrey
>>
>> El mar, 6 oct 2026 a las 15:26, Tomas Vondra (<tomas(at)vondra(dot)me>)
>> escribió:
>>
>>> Hi,
>>>
>>> On 10/5/26 16:31, Lucas Jeffrey wrote:
>>> > Hi hackers,
>>> > I'll attach a new v5 version of my patch, I rebased so my patches can
>>> > now apply cleanly in master.
>>> > Reviews are welcomed.
>>> > Regards,
>>> > Lucas Jeffrey
>>> >
>>>
>>> I accidentally ran into this issue a couple days ago, unaware of this
>>> thread. I really like the approach of the proposed fix, refcounting
>>> seems like a sound approach and we already useit elsewhere to track
>>> similar objects. I've only done a quick revie, but it seems much cleaner
>>> than my PoC fix.
>>>
>>> A couple (very cosmetic) review comments:
>>>
>>>
>>> 1) The naming seems a bit inconsistent
>>>
>>> static void ri_PreparedPlanReleaseASAP(SPIPlanPtr plan);
>>> static void ri_PreparedPlanFreeUnused(void);
>>>
>>> Why release vs. free? And the flag is called markedForDeletion, so
>>> that's a third term. Also, I wouldn't use the ASAP in function name.
>>>
>>> The SPI function is called SPI_freeplan, so what about using "free"
>>> everywhere?
>>>
>>> static void ri_PreparedPlanFree(SPIPlanPtr plan);
>>> static void ri_PreparedPlanFreeUnused(void);
>>>
>>> Although, I now realize we're not necessarily freeing the plan, so maybe
>>> this is not an improvement ...
>>>
>>>
>>> 2) Is ri_plans_to_free necessary?
>>>
>>> IIUC those plans are still in the hashtable, right? Why not to simply
>>> scan the hashtable, and free the plans there? That'd mean we don't need
>>> a separate list that can get out of sync. Maybe we expect the hash table
>>> to be large, in which case this could be expensive. But is it? I don't
>>> see any such argument in this thread.
>>>
>>> If the list is necessary, maybe ri_PreparedPlanFreeUnused() should at
>>> least have an assert that the hash_search() found a matching entry?
>>>
>>>
>>> 3) What about backpatching?
>>>
>>> AFAIK this affects all backbranches, not just master, right? So we
>>> should aim to fix the backbranches too. Do you think this refcounting
>>> fix is backpatchable (it seems like it'd be OK), or do you have an idea
>>> for a less invasive fix?
>>>
>>>
>>> regards
>>>
>>> --
>>> Tomas Vondra
>>>
>>>