On Sat, May 31, 2025 at 5:06 PM Nikolay Samokhvalov <n...@postgres.ai> wrote:
> On Thu, May 29, 2025 at 11:56 AM Sami Imseih <samims...@gmail.com> wrote: > >> It turns out that 1722d5eb05d8e reverted 525392d5727f, which >> made CachedPlan available in QueryDesc and thus >> available to pgss_ExecutorEnd. >> >> So now we have to make CachedPlan available to QueryDesc as >> part of this change. The reason the patch was reverted is related >> to a memory leak [0] in the BuildCachedPlan code and is not related >> to the part that made CachedPlan available to QueryDesc. >> >> See v6 for the rebase of the patch and addition of testing for EXPLAIN >> and EXPLAIN ANALYZE which was missing from v5. >> > > I reviewed v6: > > - applies to master cleanly, builds, tests pass and all works as expected > - overall, the patch looks great and I found no major issues > - tests and docs look good overall > - in docs, one minor comment: > > "Total number of statements executed using a generic plan" vs. what > we already have for `calls` > here, in "Number of times the statement was executed", I see some > inconsistencies: > - the word "total" should be removed, I think > - and maybe we should make wording consistent with the existing > text – "number of times the statement ...") > - Also very minor, the test queries have duplicate `calls` columns: > > SELECT calls, generic_plan_calls, custom_plan_calls, toplevel, > calls, ... > - plan->status is set in GetCachedPlan() but I don't see explicit > initialization in plan creation paths; maybe it's worth having a defensive > initialization for possible edge cases: > > plan->status = PLAN_CACHE_STATUS_UNKNOWN; > (here I'm not 100% sure, as palloc0 in CreateCachedPlan should > zero-initialize it to PLAN_CACHE_STATUS_UNKNOWN anyway) > > that's all I could find – and overall it's a great addition, > > thank you, looking forward to having these two columns in prod. > Ah, one more thing: the subject here and in CommitFest entry, "track generic and custom plans" slightly confused me at first, I think it's worth including words "calls" or "execution" there, and in the commit message, for clarity. Or just including the both column names as is. Nik
