Re: track generic and custom plans in pg_stat_statements

From: Sami Imseih <samimseih(at)gmail(dot)com>
To: Andrei Lepikhov <lepihov(at)gmail(dot)com>
Cc: Greg Sabino Mullane <htamfids(at)gmail(dot)com>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>, Nikolay Samokhvalov <nik(at)postgres(dot)ai>, Ilia Evdokimov <ilya(dot)evdokimov(at)tantorlabs(dot)com>
Subject: Re: track generic and custom plans in pg_stat_statements
Date: 2025-07-21 21:47:31
Message-ID: CAA5RZ0ss-jmqeze0G8anztzLRJuMEgz9y3gTd-rgTc2Ejtwzvg@mail.gmail.com
Views: Whole Thread | Raw Message | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

> > "plan cache mode" to be accessible in ExecutorEnd somehow.
> I agree with this - adding references to CachedPlan into the QueryDesc
> looks kludge.
> The most boring aspect of pg_stat_statements for me is the multiple
> statements case: a single incoming query (the only case in the cache of
> a generic plan) may be rewritten as various statements with the same
> query ID. So, the execution time of the initial statement should be the
> sum of the executions of all rewritten parts.

AFAICT, It does sum up the stats of rewritten statements of a single statement,
since each one of those statements does call ExecutorEnd ( under the same
queryId ).

> If the multiple-statements case didn't exist for a cached statement,
> Michael's idea with an active portal would be much better, and it would
> make sense to add PlanCacheSource::cplan and replace Portal::CachedPlan
> with Portal::PlanCacheSource reference.

I don't like ActivePortal because cplan itself does not live by ExecutorEnd [0]
and in some cases it's not available in ExecutorStart ( think of
Explain/Explain Analyze),
so you still need to add some handling for those cases, which I don't
think we can
ignore. Also, if we say we can forgo tracking the Explain/Explain Analyze case,
we surely don't want to call pgss_store at ExecutorStart to set the plan cache
mode stats and ExecutorEnd to set everything else for a particular execution.
In short, we still need some fields to be added to QueryDesc to track the
plan cache mode info.

Last week I published a v11 that adds a field to QueryDesc, but as I thought
about this more, how about we just add 2 bool fields in QueryDesc->estate
( es_cached_plan and es_is_generic_plan ), a field in CachedPlan (
is_generic_plan )
and after ExecutorStart, and if we have a cplan, we set the
appropriate plan cache
mode value. I think it's better to confine these new fields in Estate
rather than
at a higher level like QueryDesc. See v12.

[0] https://www.postgresql.org/message-id/CAA5RZ0t4kmbBM%2BetEQVb1c8bMSWjKOY8zTEA43X2UhSu0A8dCA%40mail.gmail.com

--

Sami

Attachment Content-Type Size
v12-0001-Add-counters-for-generic-and-custom-plan-executi.patch application/octet-stream 37.1 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Vik Fearing 2025-07-21 22:11:59 Re: Proposal: QUALIFY clause
Previous Message Matheus Alcantara 2025-07-21 21:29:05 Re: Proposal: QUALIFY clause