Thread: track generic and custom plans in pg_stat_statements
Hi, Currently, there is little visibility for queries that are being executed using generic or custom plans. There is pg_prepared_statements which show generic_plans and custom_plans as of d05b172a760, but this information is backend local and not very useful to a DBA that wishes to track this information cumulatively and over time. [0] had intentions to add these counters to pg_stat_statements, but was withdrawn due to timing with the commitfest at the time [0] and never picked back up again. I think it's useful to add these counters. Therefore, the attached patch adds two new columns to pg_stat_statements: "generic_plan_calls" and "custom_plan_calls". These columns track the number of executions performed using a generic or custom plan. Only non-utility statements are counted, as utility statements with an optimizable parameterized query (i.e. CTAS) cannot be called with PREPARE. Additionally, when such statements are repeatedly executed using an extended protocol prepared statement, pg_stat_statements may not handle them properly, since queryId is set to 0 during pgss_ProcessUtility. To avoid introducing two additional counters in CachedPlan, the existing boolean is_reusable—which determines whether a generic plan is reused to manage lock requirements—has been repurposed as an enum. This enum now tracks different plan states, including "generic reused", "generic first time" and "custom". pg_stat_statements uses these states to differentiate between generic and custom plans for tracking purposes. ( I felt this was better than having to carry 2 extra booleans in CachedPlan for this purpose, but that will be the alternative approach ). Not included in this patch and maybe for follow-up work, is this information can be added to EXPLAIN output and perhaps pg_stat_database. Maybe that's a good idea also? This patch bumps the version pf pg_stat_statements. -- Sami Imseih Amazon Web Services (AWS) [0] https://www.postgresql.org/message-id/add1e591fbe8874107e75d04328859ec%40oss.nttdata.com
Attachment
Overall I like the idea; adds some nice visibility to something that has been very ephemeral in the past.
Not included in this patch and maybe for follow-up work, is this information a good idea also?
can be added to EXPLAIN output and perhaps pg_stat_database.
I could see EXPLAIN being somewhat useful (especially for non-interactive things like auto_explain), so a weak +1 on that.
Definitely not useful for pg_stat_database IMHO.
Some quick comments on the patch:
FILE: contrib/pg_stat_statements/expected/plan_cache.out
These tests seem very verbose. Why not just prepare a simple query:
prepare foo as select $1 > 0;
execute foo(1);
...then tweak things via plan_cache_mode to ensure we test the right things?
Also would be nice to constrain the pg_stat_statement SELECTs with some careful WHERE clauses. Would also allow you to remove the ORDER BY and the COLLATE.
FILE: contrib/pg_stat_statements/meson.build
oldextversions should still have a trailing comma
FILE: contrib/pg_stat_statements/pg_stat_statements.c
+ if (cplan && cplan->status > PLAN_CACHE_STATUS_CUSTOM_PLAN)
Not really comfortable with using the enum like this. Better would be explicitly listing the two states that lead to the increment. Not as good but still better:
cplan->status >= PLAN_CACHE_STATUS_GENERIC_PLAN_BUILD
+ PlanCacheStatus status; /* is it a reused generic plan? */
The comment should be updated
+ PlanCacheStatus status; /* is it a reused generic plan? */
The comment should be updated
FILE: contrib/pg_stat_statements/sql/plan_cache.sql
Missing a newline at the end
Missing a newline at the end
FILE: doc/src/sgml/pgstatstatements.sgml
+ Total number of non-utility statements executed using a generic plan
I'm not sure we need to specify non-utility here.
+ Total number of non-utility statements executed using a generic plan
I'm not sure we need to specify non-utility here.
Cheers,
Greg
--
Crunchy Data - https://www.crunchydata.com
Enterprise Postgres Software Products & Tech Support
Thanks for the review! > I could see EXPLAIN being somewhat useful (especially for non-interactive things like auto_explain), so a weak +1 on that. I'll make this a follow-up to this patch. > Definitely not useful for pg_stat_database IMHO. agree as well. I did not have strong feelings about this. > FILE: contrib/pg_stat_statements/expected/plan_cache.out > > These tests seem very verbose. Why not just prepare a simple query: > > prepare foo as select $1 > 0; > execute foo(1); > ...then tweak things via plan_cache_mode to ensure we test the right things? Yeah, I went overboard to see what others think. I toned it down drastically for v2 following your advice. > oldextversions should still have a trailing comma done > FILE: contrib/pg_stat_statements/pg_stat_statements.c > > + if (cplan && cplan->status > PLAN_CACHE_STATUS_CUSTOM_PLAN) > > Not really comfortable with using the enum like this. Better would be explicitly listing the two states that lead to theincrement. Not as good but still better: > cplan->status >= PLAN_CACHE_STATUS_GENERIC_PLAN_BUILD fair, I use explicit values for each plan type. > + PlanCacheStatus status; /* is it a reused generic plan? */ > > The comment should be updated done > > FILE: contrib/pg_stat_statements/sql/plan_cache.sql > > Missing a newline at the end done > FILE: doc/src/sgml/pgstatstatements.sgml > > + Total number of non-utility statements executed using a generic plan > > I'm not sure we need to specify non-utility here. > fair, I did not have strong feeling about this either. Please see v2 Thanks! -- Sami Imseih Amazon Web Services (AWS)
Attachment
> > Please see v2 > oops, had to fix a typo in meson.build. Please see v3. -- Sami
Attachment
Hi, Thank you for your patch. It is really useful for tracking the history of generic and custom plan usage. At first glance, I have the following suggestions for improvement: 1. Is there any reason for the double check of cplan != NULL? It seems unnecessary, and we could simplify it to: -if (cplan && cplan->status == PLAN_CACHE_STATUS_CUSTOM_PLAN) +if (cplan->status == PLAN_CACHE_STATUS_CUSTOM_PLAN) 2. Should we add Assert(kind == PGSS_EXEC) at this place to ensure that generic_plan_calls and custom_plan_calls are only incremented when appropriate? -- Best regards, Ilia Evdokimov, Tantor Labs LLC.
> Thank you for your patch. It is really useful for tracking the history > of generic and custom plan usage. Thanks for the review! > 1. Is there any reason for the double check of cplan != NULL? It seems > unnecessary, and we could simplify it to: > > -if (cplan && cplan->status == PLAN_CACHE_STATUS_CUSTOM_PLAN) > +if (cplan->status == PLAN_CACHE_STATUS_CUSTOM_PLAN) No, it's not necessary and an oversight. removed. > 2. Should we add Assert(kind == PGSS_EXEC) at this place to ensure that > generic_plan_calls and custom_plan_calls are only incremented when > appropriate? > I don't think an assert is needed here. There is an assert at the start of the block for PGSS_EXEC and PGSS_PLAN, but cplan is only available in the executor. v4 attached -- Sami
Attachment
On 06.03.2025 18:04, Sami Imseih wrote:
2. Should we add Assert(kind == PGSS_EXEC) at this place to ensure that generic_plan_calls and custom_plan_calls are only incremented when appropriate? I don't think an assert is needed here. There is an assert at the start of the block for PGSS_EXEC and PGSS_PLAN, but cplan is only available in the executor.
You're right! Moreover, I didn't account for the fact that we pass NULL to pgss_ProcessUtility. In that case, Assert shouldn't be here.
I don't quite understand why do we need to differentiate between PLAN_CACHE_STATUS_GENERIC_PLAN_BUILD and PLAN_CACHE_STATUS_GENERIC_PLAN_REUSE? We could simply keep PLAN_CACHE_STATUS_GENERIC_PLAN_REUSE. I don't think users would see much of a difference in either pg_stat_statements or EXPLAIN.
As for EXPLAIN, maybe we should include this in VERBOSE mode?
--
Best regards,
Ilia Evdokimov,
Tantor Labs LLC.
> I don't quite understand why do we need to differentiate between > PLAN_CACHE_STATUS_GENERIC_PLAN_BUILD and > PLAN_CACHE_STATUS_GENERIC_PLAN_REUSE? > We could simply keep PLAN_CACHE_STATUS_GENERIC_PLAN_REUSE. > I don't think users would see much of a difference in either pg_stat_statements or EXPLAIN. If we removed GENERIC_PLAN_BUILD, I suppose we can simply rely on CPlan != NULL && cplan->status != PLAN_CACHE_STATUS_CUSTOM_PLAN to determine that we have a generic plan. However, I rather keep the status(s) defined this way for clarity. > As for EXPLAIN, maybe we should include this in VERBOSE mode? This could be a fast follow-up patch as there appears to be support for this idea. -- Sami
rebased in the attached v5. -- Sami Imseih Amazon Web Services (AWS)
Attachment
After the introduction of pg_overexplain extension and Robert's comment [0], I'm starting to have doubts about whether it's still appropriate to add this information to EXPLAIN itself. If there are compelling reasons that showing the plan type would be broadly useful to users in EXPLAIN, I’m happy to proceed. Otherwise, perhaps this is something better suited for extension. [0]: https://www.postgresql.org/message-id/CA%2BTgmoZ8qXiZmmn4P9Mk1cf2mjMMLFPOjSasCjuKSiHFcm-ncw%40mail.gmail.com -- Best regards, Ilia Evdokimov, Tantor Labs LLC.
After the introduction of pg_overexplain extension and Robert's comment
[0], I'm starting to have doubts about whether it's still appropriate to
add this information to EXPLAIN itself. If there are compelling reasons
that showing the plan type would be broadly useful to users in EXPLAIN,
I’m happy to proceed. Otherwise, perhaps this is something better suited
for extension.
[0]:
https://www.postgresql.org/message-id/CA%2BTgmoZ8qXiZmmn4P9Mk1cf2mjMMLFPOjSasCjuKSiHFcm-ncw%40mail.gmail.com
Yes, pg_overexplain can be used for the explain, and we
may not even need it at all since generic plans
are already displayed with $1, $2 parameters.
Let me know if you have other comments for v5, the
pg_stat_statements enhancements.
—
Sami Imseih
Amazon Web Services (AWS)
> It turns out that 1722d5eb05d8e reverted 525392d5727f, which > made CachedPlan available in QueryDesc and thus > available to pgss_ExecutorEnd. I also forgot to mention, the revert also removes the generic plan is_reused logic: ``` bool is_reused; /* is it a reused generic plan? */ ``` which we had to account for up until v5. So this simplifies the tracking a bit more as the only states to track are "generic plan" or "custom plan" only. -- Sami Imseih Amazon Web Services (AWS)
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)
> 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
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