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

On 10/8/26 21:26, Lucas Jeffrey 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.
>

OK. If naming is the only issue, it's in a good shape.

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

WFM, I don't see a problem with using the refcount hash table for this.

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

OK. I think you're right, but maybe it'd be a good idea to do a small
benchmark to verify this.

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

Makes sense, it doesn't make sense to developer fixes on all branches at
the same time. OTOH, isn't the fix for master pretty much done? So maybe
it's time to develop the fixes for older branches?

I think that should happen before pushing a fix to master, on the off
chance the older branches reveal some unexpected issue. And I personally
try to fix all branches at the same time. (We also sometimes adjust
patches to make backpatching simpler, but this code is not updated very
often anyway.)

regards

--
Tomas Vondra