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

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