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