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.
--
Sami
Attachment | Content-Type | Size |
---|---|---|
v12-0001-Add-counters-for-generic-and-custom-plan-executi.patch | application/octet-stream | 37.1 KB |
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 |