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