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

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)

>
> Please see v2
>

oops, had to fix a typo in meson.build. Please see v3.


--
Sami

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