Re: track generic and custom plans in pg_stat_statements

On Sat, May 31, 2025 at 5:06 PM Nikolay Samokhvalov <nik(at)postgres(dot)ai> wrote:

> On Thu, May 29, 2025 at 11:56 AM Sami Imseih <samimseih(at)gmail(dot)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