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

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