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
