Thread

Re: track generic and custom plans in pg_stat_statements

Nikolay Samokhvalov <nik@postgres.ai> — 2025-06-01T00:17:56Z

On Sat, May 31, 2025 at 5:06 PM Nikolay Samokhvalov <nik@postgres.ai> wrote:

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