Is it useful to record whether plans are generic or custom?
Hi,
When we run prepared statements, as far as I know, running
EXPLAIN is the only way to know whether executed plans are
generic or custom.
There seems no way to know how many times a prepared
statement was executed as generic and custom.
I think it may be useful to record the number of generic
and custom plans mainly to ensure the prepared statements
are executed as expected plan type.
If people also feel it's useful, I'm going to think about adding
columns such as 'generic plans' and 'custom plans' to
pg_stat_statements.
As you know, pg_stat_statements can now track not only
'calls' but 'plans', so we can presume which plan type
was executed from them.
When both 'calls' and 'plans' were incremented, plan
type would be custom. When only 'calls' was incremented,
it would be generic.
But considering the case such as only the plan phase has
succeeded and the execution phase has failed, this
presumption can be wrong.
Thoughts?
Regards,
--
Atsushi Torikoshi
Hello,
yes this can be usefull, under the condition of differentiating all the
counters
for a queryid using a generic plan and the one using a custom one.
For me one way to do that is adding a generic_plan column to
pg_stat_statements key, someting like:
- userid,
- dbid,
- queryid,
- generic_plan
I don't know if generic/custom plan types are available during planning
and/or
execution hooks ...
Regards
PAscal
--
Sent from: https://www.postgresql-archive.org/PostgreSQL-hackers-f1928748.html
On Thu, May 14, 2020 at 2:28 AM legrand legrand <legrand_legrand@hotmail.com>
wrote:
Hello,
yes this can be usefull, under the condition of differentiating all the
counters
for a queryid using a generic plan and the one using a custom one.For me one way to do that is adding a generic_plan column to
pg_stat_statements key, someting like:
- userid,
- dbid,
- queryid,
- generic_plan
Thanks for your kind advice!
I don't know if generic/custom plan types are available during planning
and/or
execution hooks ...
Yeah, that's what I'm concerned about.
As far as I can see, there are no variables tracking executed
plan types so we may need to add variables in
CachedPlanSource or somewhere.
CachedPlanSource.num_custom_plans looked like what is needed,
but it is the number of PLANNING so it also increments when
the planner calculates both plans and decides to take generic
plan.
To track executed plan types, I think execution layer hooks
are appropriate.
These hooks, however, take QueryDesc as a param and it does
not include cached plan information.
Portal includes CachedPlanSource but there seem no hooks to
take Portal.
So I'm wondering it's necessary to add a hook to get Portal
or CachedPlanSource.
Are these too much change for getting plan types?
Regards,
--
Atsushi Torikoshi
To track executed plan types, I think execution layer hooks
are appropriate.
These hooks, however, take QueryDesc as a param and it does
not include cached plan information.
It seems that the same QueryDesc entry is reused when executing
a generic plan.
For exemple marking queryDesc->plannedstmt->queryId (outside
pg_stat_statements) with a pseudo tag during ExecutorStart
reappears in later executions with generic plans ...
Is this QueryDesc reusable by a custom plan ? If not maybe a solution
could be to add a flag in queryDesc->plannedstmt ?
(sorry, I haven't understood yet how and when this generic plan is
managed during planning)
Regards
PAscal
--
Sent from: https://www.postgresql-archive.org/PostgreSQL-hackers-f1928748.html
On Sat, May 16, 2020 at 6:01 PM legrand legrand <legrand_legrand@hotmail.com>
wrote:
To track executed plan types, I think execution layer hooks
are appropriate.
These hooks, however, take QueryDesc as a param and it does
not include cached plan information.It seems that the same QueryDesc entry is reused when executing
a generic plan.
For exemple marking queryDesc->plannedstmt->queryId (outside
pg_stat_statements) with a pseudo tag during ExecutorStart
reappears in later executions with generic plans ...Is this QueryDesc reusable by a custom plan ? If not maybe a solution
could be to add a flag in queryDesc->plannedstmt ?
Thanks for your proposal!
I first thought it was a good idea and tried to add a flag to QueryDesc,
but the comments on QueryDesc say it encapsulates everything that
the executor needs to execute a query.
Whether a plan is generic or custom is not what executor needs to
know for running queries, so now I hesitate to do so.
Instead, I'm now considering using a static hash for prepared queries
(static HTAB *prepared_queries).
BTW, I'd also appreciate other opinions about recording the number
of generic and custom plans on pg_stat_statemtents.
Regards,
--
Atsushi Torikoshi
At Tue, 19 May 2020 22:56:17 +0900, Atsushi Torikoshi <atorik@gmail.com> wrote in
On Sat, May 16, 2020 at 6:01 PM legrand legrand <legrand_legrand@hotmail.com>
wrote:To track executed plan types, I think execution layer hooks
are appropriate.
These hooks, however, take QueryDesc as a param and it does
not include cached plan information.It seems that the same QueryDesc entry is reused when executing
a generic plan.
For exemple marking queryDesc->plannedstmt->queryId (outside
pg_stat_statements) with a pseudo tag during ExecutorStart
reappears in later executions with generic plans ...Is this QueryDesc reusable by a custom plan ? If not maybe a solution
could be to add a flag in queryDesc->plannedstmt ?Thanks for your proposal!
I first thought it was a good idea and tried to add a flag to QueryDesc,
but the comments on QueryDesc say it encapsulates everything that
the executor needs to execute a query.Whether a plan is generic or custom is not what executor needs to
know for running queries, so now I hesitate to do so.Instead, I'm now considering using a static hash for prepared queries
(static HTAB *prepared_queries).BTW, I'd also appreciate other opinions about recording the number
of generic and custom plans on pg_stat_statemtents.
If you/we just want to know how a prepared statement is executed,
couldn't we show that information in pg_prepared_statements view?
=# select * from pg_prepared_statements;
-[ RECORD 1 ]---+----------------------------------------------------
name | stmt1
statement | prepare stmt1 as select * from t where b = $1;
prepare_time | 2020-05-20 12:01:55.733469+09
parameter_types | {text}
from_sql | t
exec_custom | 5 <- existing num_custom_plans
exec_total | 40 <- new member of CachedPlanSource
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
On Wed, May 20, 2020 at 1:32 PM Kyotaro Horiguchi <horikyota.ntt@gmail.com>
wrote:
At Tue, 19 May 2020 22:56:17 +0900, Atsushi Torikoshi <atorik@gmail.com>
wrote inOn Sat, May 16, 2020 at 6:01 PM legrand legrand <
legrand_legrand@hotmail.com>
wrote:
BTW, I'd also appreciate other opinions about recording the number
of generic and custom plans on pg_stat_statemtents.If you/we just want to know how a prepared statement is executed,
couldn't we show that information in pg_prepared_statements view?=# select * from pg_prepared_statements;
-[ RECORD 1 ]---+----------------------------------------------------
name | stmt1
statement | prepare stmt1 as select * from t where b = $1;
prepare_time | 2020-05-20 12:01:55.733469+09
parameter_types | {text}
from_sql | t
exec_custom | 5 <- existing num_custom_plans
exec_total | 40 <- new member of CachedPlanSource
Thanks, Horiguchi-san!
Adding counters to pg_prepared_statements seems useful when we want
to know the way prepared statements executed in the current session.
And I also feel adding counters to pg_stat_statements will be convenient
especially in production environments because it enables us to get
information about not only the current session but all sessions of a
PostgreSQL instance.
If both changes are worthwhile, considering implementation complexity,
it may be reasonable to firstly add columns to pg_prepared_statements
and then work on pg_stat_statements.
Regards,
--
Atsushi Torikoshi
On 2020/05/20 21:56, Atsushi Torikoshi wrote:
On Wed, May 20, 2020 at 1:32 PM Kyotaro Horiguchi <horikyota.ntt@gmail.com <mailto:horikyota.ntt@gmail.com>> wrote:
At Tue, 19 May 2020 22:56:17 +0900, Atsushi Torikoshi <atorik@gmail.com <mailto:atorik@gmail.com>> wrote in
On Sat, May 16, 2020 at 6:01 PM legrand legrand <legrand_legrand@hotmail.com <mailto:legrand_legrand@hotmail.com>>
wrote:BTW, I'd also appreciate other opinions about recording the number
of generic and custom plans on pg_stat_statemtents.If you/we just want to know how a prepared statement is executed,
couldn't we show that information in pg_prepared_statements view?=# select * from pg_prepared_statements;
-[ RECORD 1 ]---+----------------------------------------------------
name | stmt1
statement | prepare stmt1 as select * from t where b = $1;
prepare_time | 2020-05-20 12:01:55.733469+09
parameter_types | {text}
from_sql | t
exec_custom | 5 <- existing num_custom_plans
exec_total | 40 <- new member of CachedPlanSourceThanks, Horiguchi-san!
Adding counters to pg_prepared_statements seems useful when we want
to know the way prepared statements executed in the current session.
I like the idea exposing more CachedPlanSource fields in
pg_prepared_statements. I agree it's useful, e.g., for the debug purpose.
This is why I implemented the similar feature in my extension.
Please see [1]https://github.com/MasaoFujii/pg_cheat_funcs#record-pg_cached_plan_sourcestmt-text for details.
And I also feel adding counters to pg_stat_statements will be convenient
especially in production environments because it enables us to get
information about not only the current session but all sessions of a
PostgreSQL instance.
+1
Regards,
[1]: https://github.com/MasaoFujii/pg_cheat_funcs#record-pg_cached_plan_sourcestmt-text
https://github.com/MasaoFujii/pg_cheat_funcs#record-pg_cached_plan_sourcestmt-text
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
At Thu, 21 May 2020 12:18:16 +0900, Fujii Masao <masao.fujii@oss.nttdata.com> wrote in
On 2020/05/20 21:56, Atsushi Torikoshi wrote:
On Wed, May 20, 2020 at 1:32 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com <mailto:horikyota.ntt@gmail.com>> wrote:
At Tue, 19 May 2020 22:56:17 +0900, Atsushi Torikoshi
<atorik@gmail.com <mailto:atorik@gmail.com>> wrote inOn Sat, May 16, 2020 at 6:01 PM legrand legrand
<legrand_legrand@hotmail.com <mailto:legrand_legrand@hotmail.com>>
wrote:BTW, I'd also appreciate other opinions about recording the number
of generic and custom plans on pg_stat_statemtents.If you/we just want to know how a prepared statement is executed,
couldn't we show that information in pg_prepared_statements view?
=# select * from pg_prepared_statements;
-[ RECORD 1 ]---+----------------------------------------------------
name | stmt1
statement | prepare stmt1 as select * from t where b = $1;
prepare_time | 2020-05-20 12:01:55.733469+09
parameter_types | {text}
from_sql | t
exec_custom | 5 <- existing num_custom_plans
exec_total | 40 <- new member of CachedPlanSource
Thanks, Horiguchi-san!
Adding counters to pg_prepared_statements seems useful when we want
to know the way prepared statements executed in the current session.I like the idea exposing more CachedPlanSource fields in
pg_prepared_statements. I agree it's useful, e.g., for the debug
purpose.
This is why I implemented the similar feature in my extension.
Please see [1] for details.
Thanks. I'm not sure plan_cache_mode should be a part of the view.
Cost numbers would look better if it is cooked a bit. Is it worth
being in core?
=# select * from pg_prepared_statements;
-[ RECORD 1 ]---+--------------------------------------------
name | p1
statement | prepare p1 as select a from t where a = $1;
prepare_time | 2020-05-21 15:41:50.419578+09
parameter_types | {integer}
from_sql | t
calls | 7
custom_calls | 5
plan_generation | 6
generic_cost | 4.3100000000000005
custom_cost | 9.31
Perhaps plan_generation is not needed there.
And I also feel adding counters to pg_stat_statements will be
convenient
especially in production environments because it enables us to get
information about not only the current session but all sessions of a
PostgreSQL instance.+1
Agreed. It is global and persistent.
At Tue, 19 May 2020 22:56:17 +0900, Atsushi Torikoshi <atorik@gmail.com> wrote in
Instead, I'm now considering using a static hash for prepared queries
(static HTAB *prepared_queries).
That might be complex and fragile considering nested query and SPI
calls. I'm not sure, but could we use ActivePortal?
ActivePortal->cplan is a CachedPlan, which can hold the generic/custom
information.
Instead,
[1]
https://github.com/MasaoFujii/pg_cheat_funcs#record-pg_cached_plan_sourcestmt-text
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
Attachments:
0001-Expose-counters-of-plancache-to-pg_prepared_statemen.patchtext/x-patch; charset=us-asciiDownload
From 1c6a3dd41a59504244134ee44ddd4516191656da Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horikyoga.ntt@gmail.com>
Date: Thu, 21 May 2020 15:32:38 +0900
Subject: [PATCH] Expose counters of plancache to pg_prepared_statements
We didn't have an easy way to find how many times generic or custom
plans of a prepared statement have been executed. This patch exposes
such numbers and costs of both plans in pg_prepared_statements.
---
doc/src/sgml/catalogs.sgml | 45 +++++++++++++++++++++++++++
src/backend/commands/prepare.c | 30 ++++++++++++++++--
src/backend/utils/cache/plancache.c | 13 ++++----
src/include/catalog/pg_proc.dat | 6 ++--
src/include/utils/plancache.h | 5 +--
src/test/regress/expected/prepare.out | 43 +++++++++++++++++++++++++
src/test/regress/expected/rules.out | 9 ++++--
src/test/regress/sql/prepare.sql | 9 ++++++
8 files changed, 144 insertions(+), 16 deletions(-)
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index b1b077c97f..950ed30694 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -10826,6 +10826,51 @@ SELECT * FROM pg_locks pl LEFT JOIN pg_prepared_xacts ppx
frontend/backend protocol
</para></entry>
</row>
+
+ <row>
+ <entry role="catalog_table_entry"><para role="column_definition">
+ <structfield>calls</structfield> <type>bigint</type>
+ </para>
+ <para>
+ Number of times the prepared statement was executed
+ </para></entry>
+ </row>
+
+ <row>
+ <entry role="catalog_table_entry"><para role="column_definition">
+ <structfield>custom_calls</structfield> <type>bigint</type>
+ </para>
+ <para>
+ Number of times generic plan is executed for the prepared statement
+ </para></entry>
+ </row>
+
+ <row>
+ <entry role="catalog_table_entry"><para role="column_definition">
+ <structfield>plan_geneation</structfield> <type>bigint</type>
+ </para>
+ <para>
+ Number of times execution plan was generated for the prepared statement
+ </para></entry>
+ </row>
+
+ <row>
+ <entry role="catalog_table_entry"><para role="column_definition">
+ <structfield>generic_cost</structfield> <type>double precision</type>
+ </para>
+ <para>
+ Estimated cost of generic plan. NULL if not yet planned.
+ </para></entry>
+ </row>
+
+ <row>
+ <entry role="catalog_table_entry"><para role="column_definition">
+ <structfield>custom_cost</structfield> <type>double precision</type>
+ </para>
+ <para>
+ Estimated cost of custom plans. NULL if not yet planned. This is mean of the estimated costs for the past calls including planning cost.
+ </para></entry>
+ </row>
</tbody>
</tgroup>
</table>
diff --git a/src/backend/commands/prepare.c b/src/backend/commands/prepare.c
index 80d6df8ac1..e6755df543 100644
--- a/src/backend/commands/prepare.c
+++ b/src/backend/commands/prepare.c
@@ -723,7 +723,7 @@ pg_prepared_statement(PG_FUNCTION_ARGS)
* build tupdesc for result tuples. This must match the definition of the
* pg_prepared_statements view in system_views.sql
*/
- tupdesc = CreateTemplateTupleDesc(5);
+ tupdesc = CreateTemplateTupleDesc(10);
TupleDescInitEntry(tupdesc, (AttrNumber) 1, "name",
TEXTOID, -1, 0);
TupleDescInitEntry(tupdesc, (AttrNumber) 2, "statement",
@@ -734,6 +734,16 @@ pg_prepared_statement(PG_FUNCTION_ARGS)
REGTYPEARRAYOID, -1, 0);
TupleDescInitEntry(tupdesc, (AttrNumber) 5, "from_sql",
BOOLOID, -1, 0);
+ TupleDescInitEntry(tupdesc, (AttrNumber) 6, "calls",
+ INT8OID, -1, 0);
+ TupleDescInitEntry(tupdesc, (AttrNumber) 7, "custom_calls",
+ INT8OID, -1, 0);
+ TupleDescInitEntry(tupdesc, (AttrNumber) 8, "plan_generation",
+ INT8OID, -1, 0);
+ TupleDescInitEntry(tupdesc, (AttrNumber) 9, "generic_cost",
+ FLOAT8OID, -1, 0);
+ TupleDescInitEntry(tupdesc, (AttrNumber) 10, "custom_cost",
+ FLOAT8OID, -1, 0);
/*
* We put all the tuples into a tuplestore in one scan of the hashtable.
@@ -755,8 +765,8 @@ pg_prepared_statement(PG_FUNCTION_ARGS)
hash_seq_init(&hash_seq, prepared_queries);
while ((prep_stmt = hash_seq_search(&hash_seq)) != NULL)
{
- Datum values[5];
- bool nulls[5];
+ Datum values[10];
+ bool nulls[10];
MemSet(nulls, 0, sizeof(nulls));
@@ -766,6 +776,20 @@ pg_prepared_statement(PG_FUNCTION_ARGS)
values[3] = build_regtype_array(prep_stmt->plansource->param_types,
prep_stmt->plansource->num_params);
values[4] = BoolGetDatum(prep_stmt->from_sql);
+ values[5] = Int64GetDatumFast(prep_stmt->plansource->num_total_calls);
+ values[6] = Int64GetDatumFast(prep_stmt->plansource->num_custom_plans);
+ values[7] = Int64GetDatumFast(prep_stmt->plansource->generation);
+ if (prep_stmt->plansource->generic_cost >= 0.0)
+ values[8] = Float8GetDatum(prep_stmt->plansource->generic_cost);
+ else
+ nulls[8] = true;
+
+ if (prep_stmt->plansource->num_custom_plans > 0)
+ values[9] =
+ Float8GetDatum(prep_stmt->plansource->total_custom_cost /
+ prep_stmt->plansource->num_custom_plans);
+ else
+ nulls[9] = true;
tuplestore_putvalues(tupstore, tupdesc, values, nulls);
}
diff --git a/src/backend/utils/cache/plancache.c b/src/backend/utils/cache/plancache.c
index 75b475c179..d91444f60f 100644
--- a/src/backend/utils/cache/plancache.c
+++ b/src/backend/utils/cache/plancache.c
@@ -286,6 +286,7 @@ CreateOneShotCachedPlan(RawStmt *raw_parse_tree,
plansource->generic_cost = -1;
plansource->total_custom_cost = 0;
plansource->num_custom_plans = 0;
+ plansource->num_total_calls = 0;
return plansource;
}
@@ -1213,14 +1214,13 @@ GetCachedPlan(CachedPlanSource *plansource, ParamListInfo boundParams,
{
/* Build a custom plan */
plan = BuildCachedPlan(plansource, qlist, boundParams, queryEnv);
- /* Accumulate total costs of custom plans, but 'ware overflow */
- if (plansource->num_custom_plans < INT_MAX)
- {
- plansource->total_custom_cost += cached_plan_cost(plan, true);
- plansource->num_custom_plans++;
- }
+ /* Accumulate total costs of custom plans */
+ plansource->total_custom_cost += cached_plan_cost(plan, true);
+ plansource->num_custom_plans++;
}
+ plansource->num_total_calls++;
+
Assert(plan != NULL);
/* Flag the plan as in use by caller */
@@ -1575,6 +1575,7 @@ CopyCachedPlan(CachedPlanSource *plansource)
newsource->generic_cost = plansource->generic_cost;
newsource->total_custom_cost = plansource->total_custom_cost;
newsource->num_custom_plans = plansource->num_custom_plans;
+ newsource->num_total_calls = plansource->num_total_calls;
MemoryContextSwitchTo(oldcxt);
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 61f2c2f5b4..48d0d88d97 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -7743,9 +7743,9 @@
{ oid => '2510', descr => 'get the prepared statements for this session',
proname => 'pg_prepared_statement', prorows => '1000', proretset => 't',
provolatile => 's', proparallel => 'r', prorettype => 'record',
- proargtypes => '', proallargtypes => '{text,text,timestamptz,_regtype,bool}',
- proargmodes => '{o,o,o,o,o}',
- proargnames => '{name,statement,prepare_time,parameter_types,from_sql}',
+ proargtypes => '', proallargtypes => '{text,text,timestamptz,_regtype,bool,int8,int8,int8,float8,float8}',
+ proargmodes => '{o,o,o,o,o,o,o,o,o,o}',
+ proargnames => '{name,statement,prepare_time,parameter_types,from_sql,calls,custom_calls,plan_generation,generic_cost,custom_cost}',
prosrc => 'pg_prepared_statement' },
{ oid => '2511', descr => 'get the open cursors for this session',
proname => 'pg_cursor', prorows => '1000', proretset => 't',
diff --git a/src/include/utils/plancache.h b/src/include/utils/plancache.h
index 522020d763..146eb15d34 100644
--- a/src/include/utils/plancache.h
+++ b/src/include/utils/plancache.h
@@ -124,13 +124,14 @@ typedef struct CachedPlanSource
bool is_complete; /* has CompleteCachedPlan been done? */
bool is_saved; /* has CachedPlanSource been "saved"? */
bool is_valid; /* is the query_list currently valid? */
- int generation; /* increments each time we create a plan */
+ int64 generation; /* increments each time we create a plan */
/* If CachedPlanSource has been saved, it is a member of a global list */
dlist_node node; /* list link, if is_saved */
/* State kept to help decide whether to use custom or generic plans: */
double generic_cost; /* cost of generic plan, or -1 if not known */
double total_custom_cost; /* total cost of custom plans so far */
- int num_custom_plans; /* number of plans included in total */
+ int64 num_custom_plans; /* # of custom plans included in total */
+ int64 num_total_calls;/* total number of execution */
} CachedPlanSource;
/*
diff --git a/src/test/regress/expected/prepare.out b/src/test/regress/expected/prepare.out
index 3306c696b1..b41e75c275 100644
--- a/src/test/regress/expected/prepare.out
+++ b/src/test/regress/expected/prepare.out
@@ -64,6 +64,49 @@ EXECUTE q2('postgres');
postgres | f | t
(1 row)
+EXECUTE q2('postgres');
+ datname | datistemplate | datallowconn
+----------+---------------+--------------
+ postgres | f | t
+(1 row)
+
+EXECUTE q2('postgres');
+ datname | datistemplate | datallowconn
+----------+---------------+--------------
+ postgres | f | t
+(1 row)
+
+EXECUTE q2('postgres');
+ datname | datistemplate | datallowconn
+----------+---------------+--------------
+ postgres | f | t
+(1 row)
+
+EXECUTE q2('postgres');
+ datname | datistemplate | datallowconn
+----------+---------------+--------------
+ postgres | f | t
+(1 row)
+
+EXECUTE q2('postgres');
+ datname | datistemplate | datallowconn
+----------+---------------+--------------
+ postgres | f | t
+(1 row)
+
+EXECUTE q2('postgres');
+ datname | datistemplate | datallowconn
+----------+---------------+--------------
+ postgres | f | t
+(1 row)
+
+-- the view should return the correct counts
+SELECT name, calls, custom_calls, plan_generation FROM pg_prepared_statements;
+ name | calls | custom_calls | plan_generation
+------+-------+--------------+-----------------
+ q2 | 7 | 5 | 6
+(1 row)
+
PREPARE q3(text, int, float, boolean, smallint) AS
SELECT * FROM tenk1 WHERE string4 = $1 AND (four = $2 OR
ten = $3::bigint OR true = $4 OR odd = $5::int)
diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out
index b813e32215..ff9ce88ce1 100644
--- a/src/test/regress/expected/rules.out
+++ b/src/test/regress/expected/rules.out
@@ -1428,8 +1428,13 @@ pg_prepared_statements| SELECT p.name,
p.statement,
p.prepare_time,
p.parameter_types,
- p.from_sql
- FROM pg_prepared_statement() p(name, statement, prepare_time, parameter_types, from_sql);
+ p.from_sql,
+ p.calls,
+ p.custom_calls,
+ p.plan_generation,
+ p.generic_cost,
+ p.custom_cost
+ FROM pg_prepared_statement() p(name, statement, prepare_time, parameter_types, from_sql, calls, custom_calls, plan_generation, generic_cost, custom_cost);
pg_prepared_xacts| SELECT p.transaction,
p.gid,
p.prepared,
diff --git a/src/test/regress/sql/prepare.sql b/src/test/regress/sql/prepare.sql
index 985d0f05c9..9d73c2fea3 100644
--- a/src/test/regress/sql/prepare.sql
+++ b/src/test/regress/sql/prepare.sql
@@ -35,6 +35,15 @@ PREPARE q2(text) AS
FROM pg_database WHERE datname = $1;
EXECUTE q2('postgres');
+EXECUTE q2('postgres');
+EXECUTE q2('postgres');
+EXECUTE q2('postgres');
+EXECUTE q2('postgres');
+EXECUTE q2('postgres');
+EXECUTE q2('postgres');
+
+-- the view should return the correct counts
+SELECT name, calls, custom_calls, plan_generation FROM pg_prepared_statements;
PREPARE q3(text, int, float, boolean, smallint) AS
SELECT * FROM tenk1 WHERE string4 = $1 AND (four = $2 OR
--
2.18.2
Import Notes
Reply to msg id not found: 6685aa1e-de3a-19d2-ba80-4f009eef7c3e@oss.nttdata.comCACZ0uYGfMHVvKFCU5tEbRejew8VTagLaEqUVW0t6SZ1ynMO9hw@mail.gmail.com
Hi Torikoshi-san!
On 2020/05/21 17:10, Kyotaro Horiguchi wrote:
At Thu, 21 May 2020 12:18:16 +0900, Fujii Masao <masao.fujii@oss.nttdata.com> wrote in
On 2020/05/20 21:56, Atsushi Torikoshi wrote:
On Wed, May 20, 2020 at 1:32 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com <mailto:horikyota.ntt@gmail.com>> wrote:
At Tue, 19 May 2020 22:56:17 +0900, Atsushi Torikoshi
<atorik@gmail.com <mailto:atorik@gmail.com>> wrote inOn Sat, May 16, 2020 at 6:01 PM legrand legrand
<legrand_legrand@hotmail.com <mailto:legrand_legrand@hotmail.com>>
wrote:BTW, I'd also appreciate other opinions about recording the number
of generic and custom plans on pg_stat_statemtents.If you/we just want to know how a prepared statement is executed,
couldn't we show that information in pg_prepared_statements view?
=# select * from pg_prepared_statements;
-[ RECORD 1 ]---+----------------------------------------------------
name | stmt1
statement | prepare stmt1 as select * from t where b = $1;
prepare_time | 2020-05-20 12:01:55.733469+09
parameter_types | {text}
from_sql | t
exec_custom | 5 <- existing num_custom_plans
exec_total | 40 <- new member of CachedPlanSource
Thanks, Horiguchi-san!
Adding counters to pg_prepared_statements seems useful when we want
to know the way prepared statements executed in the current session.I like the idea exposing more CachedPlanSource fields in
pg_prepared_statements. I agree it's useful, e.g., for the debug
purpose.
This is why I implemented the similar feature in my extension.
Please see [1] for details.Thanks. I'm not sure plan_cache_mode should be a part of the view.
Cost numbers would look better if it is cooked a bit. Is it worth
being in core?=# select * from pg_prepared_statements;
-[ RECORD 1 ]---+--------------------------------------------
name | p1
statement | prepare p1 as select a from t where a = $1;
prepare_time | 2020-05-21 15:41:50.419578+09
parameter_types | {integer}
from_sql | t
calls | 7
custom_calls | 5
plan_generation | 6
generic_cost | 4.3100000000000005
custom_cost | 9.31Perhaps plan_generation is not needed there.
I tried to creating PoC patch too, so I share it.
Please find attached file.
# Test case
prepare count as select count(*) from pg_class where oid >$1;
execute count(1); select * from pg_prepared_statements;
-[ RECORD 1 ]---+--------------------------------------------------------------
name | count
statement | prepare count as select count(*) from pg_class where oid >$1;
prepare_time | 2020-05-21 17:41:16.134362+09
parameter_types | {oid}
from_sql | t
is_generic_plan | f <= False
You can see the following result, when you execute it 6 times.
-[ RECORD 1 ]---+--------------------------------------------------------------
name | count
statement | prepare count as select count(*) from pg_class where oid >$1;
prepare_time | 2020-05-21 17:41:16.134362+09
parameter_types | {oid}
from_sql | t
is_generic_plan | t <= True
Thanks,
Tatsuro Yamada
Attachments:
poc_pg_prepared_statements.patchtext/plain; charset=UTF-8; name=poc_pg_prepared_statements.patchDownload
diff --git a/src/backend/commands/prepare.c b/src/backend/commands/prepare.c
index 80d6df8ac1..63de4fdb78 100644
--- a/src/backend/commands/prepare.c
+++ b/src/backend/commands/prepare.c
@@ -723,7 +723,7 @@ pg_prepared_statement(PG_FUNCTION_ARGS)
* build tupdesc for result tuples. This must match the definition of the
* pg_prepared_statements view in system_views.sql
*/
- tupdesc = CreateTemplateTupleDesc(5);
+ tupdesc = CreateTemplateTupleDesc(6);
TupleDescInitEntry(tupdesc, (AttrNumber) 1, "name",
TEXTOID, -1, 0);
TupleDescInitEntry(tupdesc, (AttrNumber) 2, "statement",
@@ -734,6 +734,8 @@ pg_prepared_statement(PG_FUNCTION_ARGS)
REGTYPEARRAYOID, -1, 0);
TupleDescInitEntry(tupdesc, (AttrNumber) 5, "from_sql",
BOOLOID, -1, 0);
+ TupleDescInitEntry(tupdesc, (AttrNumber) 6, "is_generic_plan",
+ BOOLOID, -1, 0);
/*
* We put all the tuples into a tuplestore in one scan of the hashtable.
@@ -755,8 +757,8 @@ pg_prepared_statement(PG_FUNCTION_ARGS)
hash_seq_init(&hash_seq, prepared_queries);
while ((prep_stmt = hash_seq_search(&hash_seq)) != NULL)
{
- Datum values[5];
- bool nulls[5];
+ Datum values[6];
+ bool nulls[6];
MemSet(nulls, 0, sizeof(nulls));
@@ -766,6 +768,7 @@ pg_prepared_statement(PG_FUNCTION_ARGS)
values[3] = build_regtype_array(prep_stmt->plansource->param_types,
prep_stmt->plansource->num_params);
values[4] = BoolGetDatum(prep_stmt->from_sql);
+ values[5] = BoolGetDatum(prep_stmt->plansource->gplan);
tuplestore_putvalues(tupstore, tupdesc, values, nulls);
}
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 61f2c2f5b4..b1d2d4cd37 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -7743,9 +7743,9 @@
{ oid => '2510', descr => 'get the prepared statements for this session',
proname => 'pg_prepared_statement', prorows => '1000', proretset => 't',
provolatile => 's', proparallel => 'r', prorettype => 'record',
- proargtypes => '', proallargtypes => '{text,text,timestamptz,_regtype,bool}',
- proargmodes => '{o,o,o,o,o}',
- proargnames => '{name,statement,prepare_time,parameter_types,from_sql}',
+ proargtypes => '', proallargtypes => '{text,text,timestamptz,_regtype,bool,bool}',
+ proargmodes => '{o,o,o,o,o,o}',
+ proargnames => '{name,statement,prepare_time,parameter_types,from_sql,is_generic_plan}',
prosrc => 'pg_prepared_statement' },
{ oid => '2511', descr => 'get the open cursors for this session',
proname => 'pg_cursor', prorows => '1000', proretset => 't',
Thanks for writing a patch!
On Thu, May 21, 2020 at 5:10 PM Kyotaro Horiguchi <horikyota.ntt@gmail.com>
wrote:
At Thu, 21 May 2020 12:18:16 +0900, Fujii Masao <
masao.fujii@oss.nttdata.com> wrote inI like the idea exposing more CachedPlanSource fields in
pg_prepared_statements. I agree it's useful, e.g., for the debug
purpose.
This is why I implemented the similar feature in my extension.
Please see [1] for details.Thanks. I'm not sure plan_cache_mode should be a part of the view.
Cost numbers would look better if it is cooked a bit. Is it worth
being in core?
I didn't come up with ideas about how to use them.
IMHO they might not so necessary..
=# select * from pg_prepared_statements;
-[ RECORD 1 ]---+--------------------------------------------
name | p1
statement | prepare p1 as select a from t where a = $1;
prepare_time | 2020-05-21 15:41:50.419578+09
parameter_types | {integer}
from_sql | t
calls | 7
custom_calls | 5
plan_generation | 6
generic_cost | 4.3100000000000005
custom_cost | 9.31Perhaps plan_generation is not needed there.
+1.
Now 'calls' is sufficient and 'plan_generation' may confuse users.
BTW, considering 'calls' in pg_stat_statements is the number of times
statements were EXECUTED and 'plans' is the number of times
statements were PLANNED, how about substituting 'calls' for 'plans'?
At Tue, 19 May 2020 22:56:17 +0900, Atsushi Torikoshi <atorik@gmail.com>
wrote in
Instead, I'm now considering using a static hash for prepared queries
(static HTAB *prepared_queries).That might be complex and fragile considering nested query and SPI
calls. I'm not sure, but could we use ActivePortal?
ActivePortal->cplan is a CachedPlan, which can hold the generic/custom
information.
Yes, I once looked for hooks which can get Portal, I couldn't find them.
And it also seems difficult getting keys for HTAB *prepared_queries
in existing executor hooks.
There may be oversights, but I'm now feeling returning to the idea
hook additions.
| Portal includes CachedPlanSource but there seem no hooks to
| take Portal.
| So I'm wondering it's necessary to add a hook to get Portal
| or CachedPlanSource.
| Are these too much change for getting plan types?
On Thu, May 21, 2020 at 5:43 PM Tatsuro Yamada <
tatsuro.yamada.tf@nttcom.co.jp> wrote:
I tried to creating PoC patch too, so I share it.
Please find attached file.
Thanks!
I agree with your idea showing the latest plan is generic or custom.
This patch judges whether the lastest plan was generic based on
plansource->gplan existence, but plansource->gplan can exist even
when the planner chooses custom.
For example, a prepared statement was executed first 6 times and
a generic plan was generated for comparison but the custom plan
won.
Attached another patch showing latest plan based on
'0001-Expose-counters-of-plancache-to-pg_prepared_statemen.patch'.
As I wrote above, I suppose some of the columns might not necessary
and it'd better change some column and variable names, but I left them
for other opinions.
Regards,
--
Atsushi Torikoshi
Attachments:
0002-Expose-counters-of-plancache-to-pg_prepared_statement.patchapplication/octet-stream; name=0002-Expose-counters-of-plancache-to-pg_prepared_statement.patchDownload
diff --git a/src/backend/commands/prepare.c b/src/backend/commands/prepare.c
index 80d6df8..36ff2f9 100644
--- a/src/backend/commands/prepare.c
+++ b/src/backend/commands/prepare.c
@@ -723,7 +723,7 @@ pg_prepared_statement(PG_FUNCTION_ARGS)
* build tupdesc for result tuples. This must match the definition of the
* pg_prepared_statements view in system_views.sql
*/
- tupdesc = CreateTemplateTupleDesc(5);
+ tupdesc = CreateTemplateTupleDesc(11);
TupleDescInitEntry(tupdesc, (AttrNumber) 1, "name",
TEXTOID, -1, 0);
TupleDescInitEntry(tupdesc, (AttrNumber) 2, "statement",
@@ -734,6 +734,18 @@ pg_prepared_statement(PG_FUNCTION_ARGS)
REGTYPEARRAYOID, -1, 0);
TupleDescInitEntry(tupdesc, (AttrNumber) 5, "from_sql",
BOOLOID, -1, 0);
+ TupleDescInitEntry(tupdesc, (AttrNumber) 6, "calls",
+ INT8OID, -1, 0);
+ TupleDescInitEntry(tupdesc, (AttrNumber) 7, "custom_calls",
+ INT8OID, -1, 0);
+ TupleDescInitEntry(tupdesc, (AttrNumber) 8, "plan_generation",
+ INT8OID, -1, 0);
+ TupleDescInitEntry(tupdesc, (AttrNumber) 9, "generic_cost",
+ FLOAT8OID, -1, 0);
+ TupleDescInitEntry(tupdesc, (AttrNumber) 10, "custom_cost",
+ FLOAT8OID, -1, 0);
+ TupleDescInitEntry(tupdesc, (AttrNumber) 11, "last_plan",
+ TEXTOID, -1, 0);
/*
* We put all the tuples into a tuplestore in one scan of the hashtable.
@@ -755,8 +767,8 @@ pg_prepared_statement(PG_FUNCTION_ARGS)
hash_seq_init(&hash_seq, prepared_queries);
while ((prep_stmt = hash_seq_search(&hash_seq)) != NULL)
{
- Datum values[5];
- bool nulls[5];
+ Datum values[11];
+ bool nulls[11];
MemSet(nulls, 0, sizeof(nulls));
@@ -766,6 +778,27 @@ pg_prepared_statement(PG_FUNCTION_ARGS)
values[3] = build_regtype_array(prep_stmt->plansource->param_types,
prep_stmt->plansource->num_params);
values[4] = BoolGetDatum(prep_stmt->from_sql);
+ values[5] = Int64GetDatumFast(prep_stmt->plansource->num_total_calls);
+ values[6] = Int64GetDatumFast(prep_stmt->plansource->num_custom_plans);
+ values[7] = Int64GetDatumFast(prep_stmt->plansource->generation);
+ if (prep_stmt->plansource->generic_cost >= 0.0)
+ values[8] = Float8GetDatum(prep_stmt->plansource->generic_cost);
+ else
+ nulls[8] = true;
+
+ if (prep_stmt->plansource->num_custom_plans > 0)
+ values[9] =
+ Float8GetDatum(prep_stmt->plansource->total_custom_cost /
+ prep_stmt->plansource->num_custom_plans);
+ else
+ nulls[9] = true;
+
+ if (prep_stmt->plansource->last_plan_type == PLAN_CACHE_TYPE_CUSTOM)
+ values[10] = CStringGetTextDatum("custom");
+ else if (prep_stmt->plansource->last_plan_type == PLAN_CACHE_TYPE_GENERIC)
+ values[10] = CStringGetTextDatum("generic");
+ else
+ nulls[10] = true;
tuplestore_putvalues(tupstore, tupdesc, values, nulls);
}
diff --git a/src/backend/utils/cache/plancache.c b/src/backend/utils/cache/plancache.c
index 75b475c..6457fcc 100644
--- a/src/backend/utils/cache/plancache.c
+++ b/src/backend/utils/cache/plancache.c
@@ -219,6 +219,7 @@ CreateCachedPlan(RawStmt *raw_parse_tree,
plansource->generic_cost = -1;
plansource->total_custom_cost = 0;
plansource->num_custom_plans = 0;
+ plansource->last_plan_type = PLAN_CACHE_TYPE_NONE;
MemoryContextSwitchTo(oldcxt);
@@ -286,6 +287,8 @@ CreateOneShotCachedPlan(RawStmt *raw_parse_tree,
plansource->generic_cost = -1;
plansource->total_custom_cost = 0;
plansource->num_custom_plans = 0;
+ plansource->num_total_calls = 0;
+ plansource->last_plan_type = PLAN_CACHE_TYPE_NONE;
return plansource;
}
@@ -1213,13 +1216,15 @@ GetCachedPlan(CachedPlanSource *plansource, ParamListInfo boundParams,
{
/* Build a custom plan */
plan = BuildCachedPlan(plansource, qlist, boundParams, queryEnv);
- /* Accumulate total costs of custom plans, but 'ware overflow */
- if (plansource->num_custom_plans < INT_MAX)
- {
- plansource->total_custom_cost += cached_plan_cost(plan, true);
- plansource->num_custom_plans++;
- }
+ /* Accumulate total costs of custom plans */
+ plansource->total_custom_cost += cached_plan_cost(plan, true);
+ plansource->num_custom_plans++;
+ plansource->last_plan_type = PLAN_CACHE_TYPE_CUSTOM;
}
+ else
+ plansource->last_plan_type = PLAN_CACHE_TYPE_GENERIC;
+
+ plansource->num_total_calls++;
Assert(plan != NULL);
@@ -1575,6 +1580,10 @@ CopyCachedPlan(CachedPlanSource *plansource)
newsource->generic_cost = plansource->generic_cost;
newsource->total_custom_cost = plansource->total_custom_cost;
newsource->num_custom_plans = plansource->num_custom_plans;
+ newsource->num_total_calls = plansource->num_total_calls;
+
+ /* XXX: Should inherit original plansource? */
+ newsource->last_plan_type = PLAN_CACHE_TYPE_NONE;
MemoryContextSwitchTo(oldcxt);
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 4bce3ad..52add8d 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -7745,9 +7745,9 @@
{ oid => '2510', descr => 'get the prepared statements for this session',
proname => 'pg_prepared_statement', prorows => '1000', proretset => 't',
provolatile => 's', proparallel => 'r', prorettype => 'record',
- proargtypes => '', proallargtypes => '{text,text,timestamptz,_regtype,bool}',
- proargmodes => '{o,o,o,o,o}',
- proargnames => '{name,statement,prepare_time,parameter_types,from_sql}',
+ proargtypes => '', proallargtypes => '{text,text,timestamptz,_regtype,bool,int8,int8,int8,float8,float8,text}',
+ proargmodes => '{o,o,o,o,o,o,o,o,o,o,o}',
+ proargnames => '{name,statement,prepare_time,parameter_types,from_sql,calls,custom_calls,plan_generation,generic_cost,custom_cost,last_plan}',
prosrc => 'pg_prepared_statement' },
{ oid => '2511', descr => 'get the open cursors for this session',
proname => 'pg_cursor', prorows => '1000', proretset => 't',
diff --git a/src/include/utils/plancache.h b/src/include/utils/plancache.h
index 522020d..32c86ec 100644
--- a/src/include/utils/plancache.h
+++ b/src/include/utils/plancache.h
@@ -34,6 +34,13 @@ typedef enum
PLAN_CACHE_MODE_FORCE_CUSTOM_PLAN
} PlanCacheMode;
+typedef enum
+{
+ PLAN_CACHE_TYPE_NONE,
+ PLAN_CACHE_TYPE_CUSTOM,
+ PLAN_CACHE_TYPE_GENERIC
+} PlanCacheType;
+
/* GUC parameter */
extern int plan_cache_mode;
@@ -124,13 +131,15 @@ typedef struct CachedPlanSource
bool is_complete; /* has CompleteCachedPlan been done? */
bool is_saved; /* has CachedPlanSource been "saved"? */
bool is_valid; /* is the query_list currently valid? */
- int generation; /* increments each time we create a plan */
+ int64 generation; /* increments each time we create a plan */
/* If CachedPlanSource has been saved, it is a member of a global list */
dlist_node node; /* list link, if is_saved */
/* State kept to help decide whether to use custom or generic plans: */
double generic_cost; /* cost of generic plan, or -1 if not known */
double total_custom_cost; /* total cost of custom plans so far */
- int num_custom_plans; /* number of plans included in total */
+ int64 num_custom_plans; /* # of custom plans included in total */
+ int64 num_total_calls;/* total number of execution */
+ PlanCacheType last_plan_type; /* type of last plan */
} CachedPlanSource;
/*
diff --git a/src/test/regress/expected/prepare.out b/src/test/regress/expected/prepare.out
index 3306c69..e92ce6b 100644
--- a/src/test/regress/expected/prepare.out
+++ b/src/test/regress/expected/prepare.out
@@ -64,6 +64,49 @@ EXECUTE q2('postgres');
postgres | f | t
(1 row)
+EXECUTE q2('postgres');
+ datname | datistemplate | datallowconn
+----------+---------------+--------------
+ postgres | f | t
+(1 row)
+
+EXECUTE q2('postgres');
+ datname | datistemplate | datallowconn
+----------+---------------+--------------
+ postgres | f | t
+(1 row)
+
+EXECUTE q2('postgres');
+ datname | datistemplate | datallowconn
+----------+---------------+--------------
+ postgres | f | t
+(1 row)
+
+EXECUTE q2('postgres');
+ datname | datistemplate | datallowconn
+----------+---------------+--------------
+ postgres | f | t
+(1 row)
+
+EXECUTE q2('postgres');
+ datname | datistemplate | datallowconn
+----------+---------------+--------------
+ postgres | f | t
+(1 row)
+
+EXECUTE q2('postgres');
+ datname | datistemplate | datallowconn
+----------+---------------+--------------
+ postgres | f | t
+(1 row)
+
+-- the view should return the correct counts
+SELECT name, calls, custom_calls, plan_generation, last_plan FROM pg_prepared_statements;
+ name | calls | custom_calls | plan_generation | last_plan
+------+-------+--------------+-----------------+-----------
+ q2 | 7 | 5 | 6 | generic
+(1 row)
+
PREPARE q3(text, int, float, boolean, smallint) AS
SELECT * FROM tenk1 WHERE string4 = $1 AND (four = $2 OR
ten = $3::bigint OR true = $4 OR odd = $5::int)
diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out
index 8876025..adedd01 100644
--- a/src/test/regress/expected/rules.out
+++ b/src/test/regress/expected/rules.out
@@ -1428,8 +1428,14 @@ pg_prepared_statements| SELECT p.name,
p.statement,
p.prepare_time,
p.parameter_types,
- p.from_sql
- FROM pg_prepared_statement() p(name, statement, prepare_time, parameter_types, from_sql);
+ p.from_sql,
+ p.calls,
+ p.custom_calls,
+ p.plan_generation,
+ p.generic_cost,
+ p.custom_cost,
+ p.last_plan
+ FROM pg_prepared_statement() p(name, statement, prepare_time, parameter_types, from_sql, calls, custom_calls, plan_generation, generic_cost, custom_cost, last_plan);
pg_prepared_xacts| SELECT p.transaction,
p.gid,
p.prepared,
diff --git a/src/test/regress/sql/prepare.sql b/src/test/regress/sql/prepare.sql
index 985d0f0..c886558 100644
--- a/src/test/regress/sql/prepare.sql
+++ b/src/test/regress/sql/prepare.sql
@@ -35,6 +35,15 @@ PREPARE q2(text) AS
FROM pg_database WHERE datname = $1;
EXECUTE q2('postgres');
+EXECUTE q2('postgres');
+EXECUTE q2('postgres');
+EXECUTE q2('postgres');
+EXECUTE q2('postgres');
+EXECUTE q2('postgres');
+EXECUTE q2('postgres');
+
+-- the view should return the correct counts
+SELECT name, calls, custom_calls, plan_generation, last_plan FROM pg_prepared_statements;
PREPARE q3(text, int, float, boolean, smallint) AS
SELECT * FROM tenk1 WHERE string4 = $1 AND (four = $2 OR
On Mon, May 25, 2020 at 10:54 AM Atsushi Torikoshi <atorik@gmail.com> wrote:
On Thu, May 21, 2020 at 5:10 PM Kyotaro Horiguchi <horikyota.ntt@gmail.com>
wrote:Cost numbers would look better if it is cooked a bit. Is it worth
being in core?I didn't come up with ideas about how to use them.
IMHO they might not so necessary..
Perhaps plan_generation is not needed there.
+1.
Now 'calls' is sufficient and 'plan_generation' may confuse users.BTW, considering 'calls' in pg_stat_statements is the number of times
statements were EXECUTED and 'plans' is the number of times
statements were PLANNED, how about substituting 'calls' for 'plans'?
I've modified the above points and also exposed the numbers of each
generic plans and custom plans.
I'm now a little bit worried about the below change which removed
the overflow checking for num_custom_plans, which was introduced
in 0001-Expose-counters-of-plancache-to-pg_prepared_statement.patch,
but I've left it because the maximum of int64 seems enough large
for counters.
Also referencing other counters in pg_stat_user_tables, they don't
seem to care about it.
```
- /* Accumulate total costs of custom plans, but 'ware
overflow */
- if (plansource->num_custom_plans < INT_MAX)
- {
- plansource->total_custom_cost +=
cached_plan_cost(plan, true);
- plansource->num_custom_plans++;
- }
```
Regards,
Atsushi Torikoshi
Show quoted text
Attachments:
0003-Expose-counters-of-plancache-to-pg_prepared_statement.patchapplication/octet-stream; name=0003-Expose-counters-of-plancache-to-pg_prepared_statement.patchDownload
From 2ea0bc6caad4ae75046cf286113a39ff30538771 Mon Sep 17 00:00:00 2001
From: Atsushi Torikoshi<torikoshia@oss.nttdata.com>
Date: Thu, 4 Jun 2020 15:42:08 +0900
Subject: [PATCH] Expose counters of plancache to pg_prepared_statements
We didn't have an easy way to find how many times generic or custom
plans of a prepared statement have been executed. This patch exposes
such numbers of both plans and the last plan type in pg_prepared_statements.
---
doc/src/sgml/catalogs.sgml | 28 +++++++++++++++
src/backend/commands/prepare.c | 21 +++++++++--
src/backend/utils/cache/plancache.c | 23 ++++++++----
src/include/catalog/pg_proc.dat | 6 ++--
src/include/utils/plancache.h | 12 ++++++-
src/test/regress/expected/prepare.out | 51 +++++++++++++++++++++++++++
src/test/regress/expected/rules.out | 7 ++--
src/test/regress/sql/prepare.sql | 15 ++++++++
8 files changed, 148 insertions(+), 15 deletions(-)
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 700271fd40..9e96a5518e 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -10829,6 +10829,34 @@ SELECT * FROM pg_locks pl LEFT JOIN pg_prepared_xacts ppx
frontend/backend protocol
</para></entry>
</row>
+
+ <row>
+ <entry role="catalog_table_entry"><para role="column_definition">
+ <structfield>generic_plans</structfield> <type>bigint</type>
+ </para>
+ <para>
+ Number of times generic plan was choosen
+ </para></entry>
+ </row>
+
+ <row>
+ <entry role="catalog_table_entry"><para role="column_definition">
+ <structfield>custom_plans</structfield> <type>bigint</type>
+ </para>
+ <para>
+ Number of times custom plan was choosen
+ </para></entry>
+ </row>
+
+ <row>
+ <entry role="catalog_table_entry"><para role="column_definition">
+ <structfield>last_plan</structfield> <type>text</type>
+ </para>
+ <para>
+ Tells the last plan was generic or custom. If the prepared
+ statement has not executed yet, this field is null
+ </para></entry>
+ </row>
</tbody>
</tgroup>
</table>
diff --git a/src/backend/commands/prepare.c b/src/backend/commands/prepare.c
index 80d6df8ac1..990782e77f 100644
--- a/src/backend/commands/prepare.c
+++ b/src/backend/commands/prepare.c
@@ -723,7 +723,7 @@ pg_prepared_statement(PG_FUNCTION_ARGS)
* build tupdesc for result tuples. This must match the definition of the
* pg_prepared_statements view in system_views.sql
*/
- tupdesc = CreateTemplateTupleDesc(5);
+ tupdesc = CreateTemplateTupleDesc(8);
TupleDescInitEntry(tupdesc, (AttrNumber) 1, "name",
TEXTOID, -1, 0);
TupleDescInitEntry(tupdesc, (AttrNumber) 2, "statement",
@@ -734,6 +734,12 @@ pg_prepared_statement(PG_FUNCTION_ARGS)
REGTYPEARRAYOID, -1, 0);
TupleDescInitEntry(tupdesc, (AttrNumber) 5, "from_sql",
BOOLOID, -1, 0);
+ TupleDescInitEntry(tupdesc, (AttrNumber) 6, "generic_plans",
+ INT8OID, -1, 0);
+ TupleDescInitEntry(tupdesc, (AttrNumber) 7, "custom_plans",
+ INT8OID, -1, 0);
+ TupleDescInitEntry(tupdesc, (AttrNumber) 8, "last_plan",
+ TEXTOID, -1, 0);
/*
* We put all the tuples into a tuplestore in one scan of the hashtable.
@@ -755,8 +761,8 @@ pg_prepared_statement(PG_FUNCTION_ARGS)
hash_seq_init(&hash_seq, prepared_queries);
while ((prep_stmt = hash_seq_search(&hash_seq)) != NULL)
{
- Datum values[5];
- bool nulls[5];
+ Datum values[8];
+ bool nulls[8];
MemSet(nulls, 0, sizeof(nulls));
@@ -766,6 +772,15 @@ pg_prepared_statement(PG_FUNCTION_ARGS)
values[3] = build_regtype_array(prep_stmt->plansource->param_types,
prep_stmt->plansource->num_params);
values[4] = BoolGetDatum(prep_stmt->from_sql);
+ values[5] = Int64GetDatumFast(prep_stmt->plansource->num_generic_plans);
+ values[6] = Int64GetDatumFast(prep_stmt->plansource->num_custom_plans);
+
+ if (prep_stmt->plansource->last_plan_type == PLAN_CACHE_TYPE_CUSTOM)
+ values[7] = CStringGetTextDatum("custom");
+ else if (prep_stmt->plansource->last_plan_type == PLAN_CACHE_TYPE_GENERIC)
+ values[7] = CStringGetTextDatum("generic");
+ else
+ nulls[7] = true;
tuplestore_putvalues(tupstore, tupdesc, values, nulls);
}
diff --git a/src/backend/utils/cache/plancache.c b/src/backend/utils/cache/plancache.c
index 75b475c179..fcba6c1361 100644
--- a/src/backend/utils/cache/plancache.c
+++ b/src/backend/utils/cache/plancache.c
@@ -218,7 +218,9 @@ CreateCachedPlan(RawStmt *raw_parse_tree,
plansource->generation = 0;
plansource->generic_cost = -1;
plansource->total_custom_cost = 0;
+ plansource->num_generic_plans = 0;
plansource->num_custom_plans = 0;
+ plansource->last_plan_type = PLAN_CACHE_TYPE_NONE;
MemoryContextSwitchTo(oldcxt);
@@ -285,7 +287,9 @@ CreateOneShotCachedPlan(RawStmt *raw_parse_tree,
plansource->generation = 0;
plansource->generic_cost = -1;
plansource->total_custom_cost = 0;
+ plansource->num_generic_plans = 0;
plansource->num_custom_plans = 0;
+ plansource->last_plan_type = PLAN_CACHE_TYPE_NONE;
return plansource;
}
@@ -1213,12 +1217,16 @@ GetCachedPlan(CachedPlanSource *plansource, ParamListInfo boundParams,
{
/* Build a custom plan */
plan = BuildCachedPlan(plansource, qlist, boundParams, queryEnv);
- /* Accumulate total costs of custom plans, but 'ware overflow */
- if (plansource->num_custom_plans < INT_MAX)
- {
- plansource->total_custom_cost += cached_plan_cost(plan, true);
- plansource->num_custom_plans++;
- }
+ /* Accumulate total costs of custom plans */
+ plansource->total_custom_cost += cached_plan_cost(plan, true);
+
+ plansource->num_custom_plans++;
+ plansource->last_plan_type = PLAN_CACHE_TYPE_CUSTOM;
+ }
+ else
+ {
+ plansource->num_generic_plans++;
+ plansource->last_plan_type = PLAN_CACHE_TYPE_GENERIC;
}
Assert(plan != NULL);
@@ -1574,8 +1582,11 @@ CopyCachedPlan(CachedPlanSource *plansource)
/* We may as well copy any acquired cost knowledge */
newsource->generic_cost = plansource->generic_cost;
newsource->total_custom_cost = plansource->total_custom_cost;
+ newsource->num_generic_plans = plansource->num_generic_plans;
newsource->num_custom_plans = plansource->num_custom_plans;
+ newsource->last_plan_type = PLAN_CACHE_TYPE_NONE;
+
MemoryContextSwitchTo(oldcxt);
return newsource;
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 61f2c2f5b4..34eb108556 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -7743,9 +7743,9 @@
{ oid => '2510', descr => 'get the prepared statements for this session',
proname => 'pg_prepared_statement', prorows => '1000', proretset => 't',
provolatile => 's', proparallel => 'r', prorettype => 'record',
- proargtypes => '', proallargtypes => '{text,text,timestamptz,_regtype,bool}',
- proargmodes => '{o,o,o,o,o}',
- proargnames => '{name,statement,prepare_time,parameter_types,from_sql}',
+ proargtypes => '', proallargtypes => '{text,text,timestamptz,_regtype,bool,int8,int8,text}',
+ proargmodes => '{o,o,o,o,o,o,o,o}',
+ proargnames => '{name,statement,prepare_time,parameter_types,from_sql,generic_plans,custom_plans,last_plan}',
prosrc => 'pg_prepared_statement' },
{ oid => '2511', descr => 'get the open cursors for this session',
proname => 'pg_cursor', prorows => '1000', proretset => 't',
diff --git a/src/include/utils/plancache.h b/src/include/utils/plancache.h
index 522020d763..8cbdd1685b 100644
--- a/src/include/utils/plancache.h
+++ b/src/include/utils/plancache.h
@@ -34,6 +34,14 @@ typedef enum
PLAN_CACHE_MODE_FORCE_CUSTOM_PLAN
} PlanCacheMode;
+/* possible values for a plan type */
+typedef enum
+{
+ PLAN_CACHE_TYPE_NONE,
+ PLAN_CACHE_TYPE_CUSTOM,
+ PLAN_CACHE_TYPE_GENERIC
+} PlanCacheType;
+
/* GUC parameter */
extern int plan_cache_mode;
@@ -130,7 +138,9 @@ typedef struct CachedPlanSource
/* State kept to help decide whether to use custom or generic plans: */
double generic_cost; /* cost of generic plan, or -1 if not known */
double total_custom_cost; /* total cost of custom plans so far */
- int num_custom_plans; /* number of plans included in total */
+ int64 num_custom_plans; /* # of custom plans included in total */
+ int64 num_generic_plans; /* # of generic plans */
+ PlanCacheType last_plan_type; /* type of the last plan */
} CachedPlanSource;
/*
diff --git a/src/test/regress/expected/prepare.out b/src/test/regress/expected/prepare.out
index 3306c696b1..c67aeed278 100644
--- a/src/test/regress/expected/prepare.out
+++ b/src/test/regress/expected/prepare.out
@@ -58,12 +58,63 @@ SELECT name, statement, parameter_types FROM pg_prepared_statements;
PREPARE q2(text) AS
SELECT datname, datistemplate, datallowconn
FROM pg_database WHERE datname = $1;
+-- the view should return the inital state
+SELECT name, generic_plans, custom_plans, last_plan from pg_prepared_statements;
+ name | generic_plans | custom_plans | last_plan
+------+---------------+--------------+-----------
+ q2 | 0 | 0 |
+(1 row)
+
+EXECUTE q2('postgres');
+ datname | datistemplate | datallowconn
+----------+---------------+--------------
+ postgres | f | t
+(1 row)
+
+EXECUTE q2('postgres');
+ datname | datistemplate | datallowconn
+----------+---------------+--------------
+ postgres | f | t
+(1 row)
+
+EXECUTE q2('postgres');
+ datname | datistemplate | datallowconn
+----------+---------------+--------------
+ postgres | f | t
+(1 row)
+
+EXECUTE q2('postgres');
+ datname | datistemplate | datallowconn
+----------+---------------+--------------
+ postgres | f | t
+(1 row)
+
+EXECUTE q2('postgres');
+ datname | datistemplate | datallowconn
+----------+---------------+--------------
+ postgres | f | t
+(1 row)
+
+-- until here, all the plans are custom
+SELECT name, generic_plans, custom_plans, last_plan from pg_prepared_statements;
+ name | generic_plans | custom_plans | last_plan
+------+---------------+--------------+-----------
+ q2 | 0 | 5 | custom
+(1 row)
+
EXECUTE q2('postgres');
datname | datistemplate | datallowconn
----------+---------------+--------------
postgres | f | t
(1 row)
+-- latest plan shoud be generic
+SELECT name, generic_plans, custom_plans, last_plan from pg_prepared_statements;
+ name | generic_plans | custom_plans | last_plan
+------+---------------+--------------+-----------
+ q2 | 1 | 5 | generic
+(1 row)
+
PREPARE q3(text, int, float, boolean, smallint) AS
SELECT * FROM tenk1 WHERE string4 = $1 AND (four = $2 OR
ten = $3::bigint OR true = $4 OR odd = $5::int)
diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out
index b813e32215..f1ccfe6004 100644
--- a/src/test/regress/expected/rules.out
+++ b/src/test/regress/expected/rules.out
@@ -1428,8 +1428,11 @@ pg_prepared_statements| SELECT p.name,
p.statement,
p.prepare_time,
p.parameter_types,
- p.from_sql
- FROM pg_prepared_statement() p(name, statement, prepare_time, parameter_types, from_sql);
+ p.from_sql,
+ p.generic_plans,
+ p.custom_plans,
+ p.last_plan
+ FROM pg_prepared_statement() p(name, statement, prepare_time, parameter_types, from_sql, generic_plans, custom_plans, last_plan);
pg_prepared_xacts| SELECT p.transaction,
p.gid,
p.prepared,
diff --git a/src/test/regress/sql/prepare.sql b/src/test/regress/sql/prepare.sql
index 985d0f05c9..18b42ac1a4 100644
--- a/src/test/regress/sql/prepare.sql
+++ b/src/test/regress/sql/prepare.sql
@@ -34,8 +34,23 @@ PREPARE q2(text) AS
SELECT datname, datistemplate, datallowconn
FROM pg_database WHERE datname = $1;
+-- the view should return the inital state
+SELECT name, generic_plans, custom_plans, last_plan from pg_prepared_statements;
+
+EXECUTE q2('postgres');
+EXECUTE q2('postgres');
+EXECUTE q2('postgres');
+EXECUTE q2('postgres');
EXECUTE q2('postgres');
+-- until here, all the plans are custom
+SELECT name, generic_plans, custom_plans, last_plan from pg_prepared_statements;
+
+EXECUTE q2('postgres');
+
+-- latest plan shoud be generic
+SELECT name, generic_plans, custom_plans, last_plan from pg_prepared_statements;
+
PREPARE q3(text, int, float, boolean, smallint) AS
SELECT * FROM tenk1 WHERE string4 = $1 AND (four = $2 OR
ten = $3::bigint OR true = $4 OR odd = $5::int)
--
2.18.1
On 2020-06-04 17:04, Atsushi Torikoshi wrote:
On Mon, May 25, 2020 at 10:54 AM Atsushi Torikoshi <atorik@gmail.com>
wrote:On Thu, May 21, 2020 at 5:10 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:Cost numbers would look better if it is cooked a bit. Is it worth
being in core?I didn't come up with ideas about how to use them.
IMHO they might not so necessary..Perhaps plan_generation is not needed there.
+1.
Now 'calls' is sufficient and 'plan_generation' may confuse users.BTW, considering 'calls' in pg_stat_statements is the number of
times
statements were EXECUTED and 'plans' is the number of times
statements were PLANNED, how about substituting 'calls' for
'plans'?I've modified the above points and also exposed the numbers of each
generic plans and custom plans.I'm now a little bit worried about the below change which removed
the overflow checking for num_custom_plans, which was introduced
in 0001-Expose-counters-of-plancache-to-pg_prepared_statement.patch,
but I've left it because the maximum of int64 seems enough large
for counters.
Also referencing other counters in pg_stat_user_tables, they don't
seem to care about it.```
- /* Accumulate total costs of custom plans, but 'ware
overflow */
- if (plansource->num_custom_plans < INT_MAX)
- {
- plansource->total_custom_cost +=
cached_plan_cost(plan, true);
- plansource->num_custom_plans++;
- }```
Regards,
Atsushi Torikoshi
As a user, I think this feature is useful for performance analysis.
I hope that this feature is merged.
BTW, I found that the dependency between function's comments and
the modified code is broken at latest patch. Before this is
committed, please fix it.
```
diff --git a/src/backend/commands/prepare.c
b/src/backend/commands/prepare.c
index 990782e77f..b63d3214df 100644
--- a/src/backend/commands/prepare.c
+++ b/src/backend/commands/prepare.c
@@ -694,7 +694,8 @@ ExplainExecuteQuery(ExecuteStmt *execstmt,
IntoClause *into, ExplainState *es,
/*
* This set returning function reads all the prepared statements and
- * returns a set of (name, statement, prepare_time, param_types,
from_sql).
+ * returns a set of (name, statement, prepare_time, param_types,
from_sql,
+ * generic_plans, custom_plans, last_plan).
*/
Datum
pg_prepared_statement(PG_FUNCTION_ARGS)
```
Regards,
--
Masahiro Ikeda
On 2020-06-08 20:45, Masahiro Ikeda wrote:
BTW, I found that the dependency between function's comments and
the modified code is broken at latest patch. Before this is
committed, please fix it.``` diff --git a/src/backend/commands/prepare.c b/src/backend/commands/prepare.c index 990782e77f..b63d3214df 100644 --- a/src/backend/commands/prepare.c +++ b/src/backend/commands/prepare.c @@ -694,7 +694,8 @@ ExplainExecuteQuery(ExecuteStmt *execstmt, IntoClause *into, ExplainState *es,/* * This set returning function reads all the prepared statements and - * returns a set of (name, statement, prepare_time, param_types, from_sql). + * returns a set of (name, statement, prepare_time, param_types, from_sql, + * generic_plans, custom_plans, last_plan). */ Datum pg_prepared_statement(PG_FUNCTION_ARGS) ```
Thanks for reviewing!
I've fixed it.
Regards,
--
Atsushi Torikoshi
Attachments:
0004-Expose-counters-of-plancache-to-pg_prepared_statement.patchtext/x-diff; name=0004-Expose-counters-of-plancache-to-pg_prepared_statement.patchDownload
From f2155b1a9f3b500eca490827dd044f5c0f704dd3 Mon Sep 17 00:00:00 2001
From: Atsushi Torikoshi<torikoshia@oss.nttdata.com>
Date: Tue, 10 Jun 2020 10:22:20 +0900
Subject: [PATCH] We didn't have an easy way to find how many times generic or
custom plans of a prepared statement have been executed. This patch exposes
such numbers of both plans and the last plan type in pg_prepared_statements.
---
doc/src/sgml/catalogs.sgml | 28 +++++++++++++++
src/backend/commands/prepare.c | 24 ++++++++++---
src/backend/utils/cache/plancache.c | 23 ++++++++----
src/include/catalog/pg_proc.dat | 6 ++--
src/include/utils/plancache.h | 12 ++++++-
src/test/regress/expected/prepare.out | 51 +++++++++++++++++++++++++++
src/test/regress/expected/rules.out | 7 ++--
src/test/regress/sql/prepare.sql | 15 ++++++++
8 files changed, 150 insertions(+), 16 deletions(-)
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 700271fd40..9e96a5518e 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -10829,6 +10829,34 @@ SELECT * FROM pg_locks pl LEFT JOIN pg_prepared_xacts ppx
frontend/backend protocol
</para></entry>
</row>
+
+ <row>
+ <entry role="catalog_table_entry"><para role="column_definition">
+ <structfield>generic_plans</structfield> <type>bigint</type>
+ </para>
+ <para>
+ Number of times generic plan was choosen
+ </para></entry>
+ </row>
+
+ <row>
+ <entry role="catalog_table_entry"><para role="column_definition">
+ <structfield>custom_plans</structfield> <type>bigint</type>
+ </para>
+ <para>
+ Number of times custom plan was choosen
+ </para></entry>
+ </row>
+
+ <row>
+ <entry role="catalog_table_entry"><para role="column_definition">
+ <structfield>last_plan</structfield> <type>text</type>
+ </para>
+ <para>
+ Tells the last plan was generic or custom. If the prepared
+ statement has not executed yet, this field is null
+ </para></entry>
+ </row>
</tbody>
</tgroup>
</table>
diff --git a/src/backend/commands/prepare.c b/src/backend/commands/prepare.c
index 80d6df8ac1..d60cf6f67a 100644
--- a/src/backend/commands/prepare.c
+++ b/src/backend/commands/prepare.c
@@ -694,7 +694,8 @@ ExplainExecuteQuery(ExecuteStmt *execstmt, IntoClause *into, ExplainState *es,
/*
* This set returning function reads all the prepared statements and
- * returns a set of (name, statement, prepare_time, param_types, from_sql).
+ * returns a set of (name, statement, prepare_time, param_types, from_sql,
+ * generic_plans, custom_plans, last_plan).
*/
Datum
pg_prepared_statement(PG_FUNCTION_ARGS)
@@ -723,7 +724,7 @@ pg_prepared_statement(PG_FUNCTION_ARGS)
* build tupdesc for result tuples. This must match the definition of the
* pg_prepared_statements view in system_views.sql
*/
- tupdesc = CreateTemplateTupleDesc(5);
+ tupdesc = CreateTemplateTupleDesc(8);
TupleDescInitEntry(tupdesc, (AttrNumber) 1, "name",
TEXTOID, -1, 0);
TupleDescInitEntry(tupdesc, (AttrNumber) 2, "statement",
@@ -734,6 +735,12 @@ pg_prepared_statement(PG_FUNCTION_ARGS)
REGTYPEARRAYOID, -1, 0);
TupleDescInitEntry(tupdesc, (AttrNumber) 5, "from_sql",
BOOLOID, -1, 0);
+ TupleDescInitEntry(tupdesc, (AttrNumber) 6, "generic_plans",
+ INT8OID, -1, 0);
+ TupleDescInitEntry(tupdesc, (AttrNumber) 7, "custom_plans",
+ INT8OID, -1, 0);
+ TupleDescInitEntry(tupdesc, (AttrNumber) 8, "last_plan",
+ TEXTOID, -1, 0);
/*
* We put all the tuples into a tuplestore in one scan of the hashtable.
@@ -755,8 +762,8 @@ pg_prepared_statement(PG_FUNCTION_ARGS)
hash_seq_init(&hash_seq, prepared_queries);
while ((prep_stmt = hash_seq_search(&hash_seq)) != NULL)
{
- Datum values[5];
- bool nulls[5];
+ Datum values[8];
+ bool nulls[8];
MemSet(nulls, 0, sizeof(nulls));
@@ -766,6 +773,15 @@ pg_prepared_statement(PG_FUNCTION_ARGS)
values[3] = build_regtype_array(prep_stmt->plansource->param_types,
prep_stmt->plansource->num_params);
values[4] = BoolGetDatum(prep_stmt->from_sql);
+ values[5] = Int64GetDatumFast(prep_stmt->plansource->num_generic_plans);
+ values[6] = Int64GetDatumFast(prep_stmt->plansource->num_custom_plans);
+
+ if (prep_stmt->plansource->last_plan_type == PLAN_CACHE_TYPE_CUSTOM)
+ values[7] = CStringGetTextDatum("custom");
+ else if (prep_stmt->plansource->last_plan_type == PLAN_CACHE_TYPE_GENERIC)
+ values[7] = CStringGetTextDatum("generic");
+ else
+ nulls[7] = true;
tuplestore_putvalues(tupstore, tupdesc, values, nulls);
}
diff --git a/src/backend/utils/cache/plancache.c b/src/backend/utils/cache/plancache.c
index 75b475c179..fcba6c1361 100644
--- a/src/backend/utils/cache/plancache.c
+++ b/src/backend/utils/cache/plancache.c
@@ -218,7 +218,9 @@ CreateCachedPlan(RawStmt *raw_parse_tree,
plansource->generation = 0;
plansource->generic_cost = -1;
plansource->total_custom_cost = 0;
+ plansource->num_generic_plans = 0;
plansource->num_custom_plans = 0;
+ plansource->last_plan_type = PLAN_CACHE_TYPE_NONE;
MemoryContextSwitchTo(oldcxt);
@@ -285,7 +287,9 @@ CreateOneShotCachedPlan(RawStmt *raw_parse_tree,
plansource->generation = 0;
plansource->generic_cost = -1;
plansource->total_custom_cost = 0;
+ plansource->num_generic_plans = 0;
plansource->num_custom_plans = 0;
+ plansource->last_plan_type = PLAN_CACHE_TYPE_NONE;
return plansource;
}
@@ -1213,12 +1217,16 @@ GetCachedPlan(CachedPlanSource *plansource, ParamListInfo boundParams,
{
/* Build a custom plan */
plan = BuildCachedPlan(plansource, qlist, boundParams, queryEnv);
- /* Accumulate total costs of custom plans, but 'ware overflow */
- if (plansource->num_custom_plans < INT_MAX)
- {
- plansource->total_custom_cost += cached_plan_cost(plan, true);
- plansource->num_custom_plans++;
- }
+ /* Accumulate total costs of custom plans */
+ plansource->total_custom_cost += cached_plan_cost(plan, true);
+
+ plansource->num_custom_plans++;
+ plansource->last_plan_type = PLAN_CACHE_TYPE_CUSTOM;
+ }
+ else
+ {
+ plansource->num_generic_plans++;
+ plansource->last_plan_type = PLAN_CACHE_TYPE_GENERIC;
}
Assert(plan != NULL);
@@ -1574,8 +1582,11 @@ CopyCachedPlan(CachedPlanSource *plansource)
/* We may as well copy any acquired cost knowledge */
newsource->generic_cost = plansource->generic_cost;
newsource->total_custom_cost = plansource->total_custom_cost;
+ newsource->num_generic_plans = plansource->num_generic_plans;
newsource->num_custom_plans = plansource->num_custom_plans;
+ newsource->last_plan_type = PLAN_CACHE_TYPE_NONE;
+
MemoryContextSwitchTo(oldcxt);
return newsource;
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 61f2c2f5b4..34eb108556 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -7743,9 +7743,9 @@
{ oid => '2510', descr => 'get the prepared statements for this session',
proname => 'pg_prepared_statement', prorows => '1000', proretset => 't',
provolatile => 's', proparallel => 'r', prorettype => 'record',
- proargtypes => '', proallargtypes => '{text,text,timestamptz,_regtype,bool}',
- proargmodes => '{o,o,o,o,o}',
- proargnames => '{name,statement,prepare_time,parameter_types,from_sql}',
+ proargtypes => '', proallargtypes => '{text,text,timestamptz,_regtype,bool,int8,int8,text}',
+ proargmodes => '{o,o,o,o,o,o,o,o}',
+ proargnames => '{name,statement,prepare_time,parameter_types,from_sql,generic_plans,custom_plans,last_plan}',
prosrc => 'pg_prepared_statement' },
{ oid => '2511', descr => 'get the open cursors for this session',
proname => 'pg_cursor', prorows => '1000', proretset => 't',
diff --git a/src/include/utils/plancache.h b/src/include/utils/plancache.h
index 522020d763..8cbdd1685b 100644
--- a/src/include/utils/plancache.h
+++ b/src/include/utils/plancache.h
@@ -34,6 +34,14 @@ typedef enum
PLAN_CACHE_MODE_FORCE_CUSTOM_PLAN
} PlanCacheMode;
+/* possible values for a plan type */
+typedef enum
+{
+ PLAN_CACHE_TYPE_NONE,
+ PLAN_CACHE_TYPE_CUSTOM,
+ PLAN_CACHE_TYPE_GENERIC
+} PlanCacheType;
+
/* GUC parameter */
extern int plan_cache_mode;
@@ -130,7 +138,9 @@ typedef struct CachedPlanSource
/* State kept to help decide whether to use custom or generic plans: */
double generic_cost; /* cost of generic plan, or -1 if not known */
double total_custom_cost; /* total cost of custom plans so far */
- int num_custom_plans; /* number of plans included in total */
+ int64 num_custom_plans; /* # of custom plans included in total */
+ int64 num_generic_plans; /* # of generic plans */
+ PlanCacheType last_plan_type; /* type of the last plan */
} CachedPlanSource;
/*
diff --git a/src/test/regress/expected/prepare.out b/src/test/regress/expected/prepare.out
index 3306c696b1..c67aeed278 100644
--- a/src/test/regress/expected/prepare.out
+++ b/src/test/regress/expected/prepare.out
@@ -58,12 +58,63 @@ SELECT name, statement, parameter_types FROM pg_prepared_statements;
PREPARE q2(text) AS
SELECT datname, datistemplate, datallowconn
FROM pg_database WHERE datname = $1;
+-- the view should return the inital state
+SELECT name, generic_plans, custom_plans, last_plan from pg_prepared_statements;
+ name | generic_plans | custom_plans | last_plan
+------+---------------+--------------+-----------
+ q2 | 0 | 0 |
+(1 row)
+
+EXECUTE q2('postgres');
+ datname | datistemplate | datallowconn
+----------+---------------+--------------
+ postgres | f | t
+(1 row)
+
+EXECUTE q2('postgres');
+ datname | datistemplate | datallowconn
+----------+---------------+--------------
+ postgres | f | t
+(1 row)
+
+EXECUTE q2('postgres');
+ datname | datistemplate | datallowconn
+----------+---------------+--------------
+ postgres | f | t
+(1 row)
+
+EXECUTE q2('postgres');
+ datname | datistemplate | datallowconn
+----------+---------------+--------------
+ postgres | f | t
+(1 row)
+
+EXECUTE q2('postgres');
+ datname | datistemplate | datallowconn
+----------+---------------+--------------
+ postgres | f | t
+(1 row)
+
+-- until here, all the plans are custom
+SELECT name, generic_plans, custom_plans, last_plan from pg_prepared_statements;
+ name | generic_plans | custom_plans | last_plan
+------+---------------+--------------+-----------
+ q2 | 0 | 5 | custom
+(1 row)
+
EXECUTE q2('postgres');
datname | datistemplate | datallowconn
----------+---------------+--------------
postgres | f | t
(1 row)
+-- latest plan shoud be generic
+SELECT name, generic_plans, custom_plans, last_plan from pg_prepared_statements;
+ name | generic_plans | custom_plans | last_plan
+------+---------------+--------------+-----------
+ q2 | 1 | 5 | generic
+(1 row)
+
PREPARE q3(text, int, float, boolean, smallint) AS
SELECT * FROM tenk1 WHERE string4 = $1 AND (four = $2 OR
ten = $3::bigint OR true = $4 OR odd = $5::int)
diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out
index b813e32215..f1ccfe6004 100644
--- a/src/test/regress/expected/rules.out
+++ b/src/test/regress/expected/rules.out
@@ -1428,8 +1428,11 @@ pg_prepared_statements| SELECT p.name,
p.statement,
p.prepare_time,
p.parameter_types,
- p.from_sql
- FROM pg_prepared_statement() p(name, statement, prepare_time, parameter_types, from_sql);
+ p.from_sql,
+ p.generic_plans,
+ p.custom_plans,
+ p.last_plan
+ FROM pg_prepared_statement() p(name, statement, prepare_time, parameter_types, from_sql, generic_plans, custom_plans, last_plan);
pg_prepared_xacts| SELECT p.transaction,
p.gid,
p.prepared,
diff --git a/src/test/regress/sql/prepare.sql b/src/test/regress/sql/prepare.sql
index 985d0f05c9..18b42ac1a4 100644
--- a/src/test/regress/sql/prepare.sql
+++ b/src/test/regress/sql/prepare.sql
@@ -34,8 +34,23 @@ PREPARE q2(text) AS
SELECT datname, datistemplate, datallowconn
FROM pg_database WHERE datname = $1;
+-- the view should return the inital state
+SELECT name, generic_plans, custom_plans, last_plan from pg_prepared_statements;
+
+EXECUTE q2('postgres');
+EXECUTE q2('postgres');
+EXECUTE q2('postgres');
+EXECUTE q2('postgres');
EXECUTE q2('postgres');
+-- until here, all the plans are custom
+SELECT name, generic_plans, custom_plans, last_plan from pg_prepared_statements;
+
+EXECUTE q2('postgres');
+
+-- latest plan shoud be generic
+SELECT name, generic_plans, custom_plans, last_plan from pg_prepared_statements;
+
PREPARE q3(text, int, float, boolean, smallint) AS
SELECT * FROM tenk1 WHERE string4 = $1 AND (four = $2 OR
ten = $3::bigint OR true = $4 OR odd = $5::int)
--
2.18.1
At Wed, 10 Jun 2020 10:50:58 +0900, torikoshia <torikoshia@oss.nttdata.com> wrote in
On 2020-06-08 20:45, Masahiro Ikeda wrote:
BTW, I found that the dependency between function's comments and the modified code is broken at latest patch. Before this is committed, please fix it. ``` diff --git a/src/backend/commands/prepare.c b/src/backend/commands/prepare.c index 990782e77f..b63d3214df 100644 --- a/src/backend/commands/prepare.c +++ b/src/backend/commands/prepare.c @@ -694,7 +694,8 @@ ExplainExecuteQuery(ExecuteStmt *execstmt, IntoClause *into, ExplainState *es, /* * This set returning function reads all the prepared statements and - * returns a set of (name, statement, prepare_time, param_types, - * from_sql). + * returns a set of (name, statement, prepare_time, param_types, from_sql, + * generic_plans, custom_plans, last_plan). */ Datum pg_prepared_statement(PG_FUNCTION_ARGS) ```Thanks for reviewing!
I've fixed it.
+ TupleDescInitEntry(tupdesc, (AttrNumber) 8, "last_plan",
This could be a problem if we showed the last plan in this view. I
think "last_plan_type" would be better.
+ if (prep_stmt->plansource->last_plan_type == PLAN_CACHE_TYPE_CUSTOM)
+ values[7] = CStringGetTextDatum("custom");
+ else if (prep_stmt->plansource->last_plan_type == PLAN_CACHE_TYPE_GENERIC)
+ values[7] = CStringGetTextDatum("generic");
+ else
+ nulls[7] = true;
Using swith-case prevents future additional type (if any) from being
unhandled. I think we are recommending that as a convension.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
On 2020-06-10 18:00, Kyotaro Horiguchi wrote:
+ TupleDescInitEntry(tupdesc, (AttrNumber) 8, "last_plan",
This could be a problem if we showed the last plan in this view. I
think "last_plan_type" would be better.+ if (prep_stmt->plansource->last_plan_type == PLAN_CACHE_TYPE_CUSTOM) + values[7] = CStringGetTextDatum("custom"); + else if (prep_stmt->plansource->last_plan_type == PLAN_CACHE_TYPE_GENERIC) + values[7] = CStringGetTextDatum("generic"); + else + nulls[7] = true;Using swith-case prevents future additional type (if any) from being
unhandled. I think we are recommending that as a convension.
Thanks for your reviewing!
I've attached a patch that reflects your comments.
Regards,
--
Atsushi Torikoshi
Attachments:
0005-Expose-counters-of-plancache-to-pg_prepared_statement.patchtext/x-diff; name=0005-Expose-counters-of-plancache-to-pg_prepared_statement.patchDownload
From f2155b1a9f3b500eca490827dd044f5c0f704dd3 Mon Sep 17 00:00:00 2001
From: Atsushi Torikoshi<torikoshia@oss.nttdata.com>
Date: Thu, 11 Jun 2020 11:35:25 +0900
Subject: [PATCH] We didn't have an easy way to find how many times generic or
custom plans of a prepared statement have been executed. This patch exposes
such numbers of both plans and the last plan type in pg_prepared_statements.
fixed comments
---
doc/src/sgml/catalogs.sgml | 28 +++++++++++++++
src/backend/commands/prepare.c | 29 ++++++++++++---
src/backend/utils/cache/plancache.c | 23 ++++++++----
src/include/catalog/pg_proc.dat | 6 ++--
src/include/utils/plancache.h | 12 ++++++-
src/test/regress/expected/prepare.out | 51 +++++++++++++++++++++++++++
src/test/regress/expected/rules.out | 7 ++--
src/test/regress/sql/prepare.sql | 15 ++++++++
8 files changed, 155 insertions(+), 16 deletions(-)
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 700271fd40..ac9e56d5b3 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -10829,6 +10829,34 @@ SELECT * FROM pg_locks pl LEFT JOIN pg_prepared_xacts ppx
frontend/backend protocol
</para></entry>
</row>
+
+ <row>
+ <entry role="catalog_table_entry"><para role="column_definition">
+ <structfield>generic_plans</structfield> <type>bigint</type>
+ </para>
+ <para>
+ Number of times generic plan was choosen
+ </para></entry>
+ </row>
+
+ <row>
+ <entry role="catalog_table_entry"><para role="column_definition">
+ <structfield>custom_plans</structfield> <type>bigint</type>
+ </para>
+ <para>
+ Number of times custom plan was choosen
+ </para></entry>
+ </row>
+
+ <row>
+ <entry role="catalog_table_entry"><para role="column_definition">
+ <structfield>last_plan_type</structfield> <type>text</type>
+ </para>
+ <para>
+ Tells the last plan type was generic or custom. If the prepared
+ statement has not executed yet, this field is null
+ </para></entry>
+ </row>
</tbody>
</tgroup>
</table>
diff --git a/src/backend/commands/prepare.c b/src/backend/commands/prepare.c
index 80d6df8ac1..b4b1865d48 100644
--- a/src/backend/commands/prepare.c
+++ b/src/backend/commands/prepare.c
@@ -694,7 +694,8 @@ ExplainExecuteQuery(ExecuteStmt *execstmt, IntoClause *into, ExplainState *es,
/*
* This set returning function reads all the prepared statements and
- * returns a set of (name, statement, prepare_time, param_types, from_sql).
+ * returns a set of (name, statement, prepare_time, param_types, from_sql,
+ * generic_plans, custom_plans, last_plan_type).
*/
Datum
pg_prepared_statement(PG_FUNCTION_ARGS)
@@ -723,7 +724,7 @@ pg_prepared_statement(PG_FUNCTION_ARGS)
* build tupdesc for result tuples. This must match the definition of the
* pg_prepared_statements view in system_views.sql
*/
- tupdesc = CreateTemplateTupleDesc(5);
+ tupdesc = CreateTemplateTupleDesc(8);
TupleDescInitEntry(tupdesc, (AttrNumber) 1, "name",
TEXTOID, -1, 0);
TupleDescInitEntry(tupdesc, (AttrNumber) 2, "statement",
@@ -734,6 +735,12 @@ pg_prepared_statement(PG_FUNCTION_ARGS)
REGTYPEARRAYOID, -1, 0);
TupleDescInitEntry(tupdesc, (AttrNumber) 5, "from_sql",
BOOLOID, -1, 0);
+ TupleDescInitEntry(tupdesc, (AttrNumber) 6, "generic_plans",
+ INT8OID, -1, 0);
+ TupleDescInitEntry(tupdesc, (AttrNumber) 7, "custom_plans",
+ INT8OID, -1, 0);
+ TupleDescInitEntry(tupdesc, (AttrNumber) 8, "last_plan_type",
+ TEXTOID, -1, 0);
/*
* We put all the tuples into a tuplestore in one scan of the hashtable.
@@ -755,8 +762,8 @@ pg_prepared_statement(PG_FUNCTION_ARGS)
hash_seq_init(&hash_seq, prepared_queries);
while ((prep_stmt = hash_seq_search(&hash_seq)) != NULL)
{
- Datum values[5];
- bool nulls[5];
+ Datum values[8];
+ bool nulls[8];
MemSet(nulls, 0, sizeof(nulls));
@@ -766,6 +773,20 @@ pg_prepared_statement(PG_FUNCTION_ARGS)
values[3] = build_regtype_array(prep_stmt->plansource->param_types,
prep_stmt->plansource->num_params);
values[4] = BoolGetDatum(prep_stmt->from_sql);
+ values[5] = Int64GetDatumFast(prep_stmt->plansource->num_generic_plans);
+ values[6] = Int64GetDatumFast(prep_stmt->plansource->num_custom_plans);
+
+ switch (prep_stmt->plansource->last_plan_type)
+ {
+ case PLAN_CACHE_TYPE_CUSTOM:
+ values[7] = CStringGetTextDatum("custom");
+ break;
+ case PLAN_CACHE_TYPE_GENERIC:
+ values[7] = CStringGetTextDatum("generic");
+ break;
+ default:
+ nulls[7] = true;
+ }
tuplestore_putvalues(tupstore, tupdesc, values, nulls);
}
diff --git a/src/backend/utils/cache/plancache.c b/src/backend/utils/cache/plancache.c
index 75b475c179..fcba6c1361 100644
--- a/src/backend/utils/cache/plancache.c
+++ b/src/backend/utils/cache/plancache.c
@@ -218,7 +218,9 @@ CreateCachedPlan(RawStmt *raw_parse_tree,
plansource->generation = 0;
plansource->generic_cost = -1;
plansource->total_custom_cost = 0;
+ plansource->num_generic_plans = 0;
plansource->num_custom_plans = 0;
+ plansource->last_plan_type = PLAN_CACHE_TYPE_NONE;
MemoryContextSwitchTo(oldcxt);
@@ -285,7 +287,9 @@ CreateOneShotCachedPlan(RawStmt *raw_parse_tree,
plansource->generation = 0;
plansource->generic_cost = -1;
plansource->total_custom_cost = 0;
+ plansource->num_generic_plans = 0;
plansource->num_custom_plans = 0;
+ plansource->last_plan_type = PLAN_CACHE_TYPE_NONE;
return plansource;
}
@@ -1213,12 +1217,16 @@ GetCachedPlan(CachedPlanSource *plansource, ParamListInfo boundParams,
{
/* Build a custom plan */
plan = BuildCachedPlan(plansource, qlist, boundParams, queryEnv);
- /* Accumulate total costs of custom plans, but 'ware overflow */
- if (plansource->num_custom_plans < INT_MAX)
- {
- plansource->total_custom_cost += cached_plan_cost(plan, true);
- plansource->num_custom_plans++;
- }
+ /* Accumulate total costs of custom plans */
+ plansource->total_custom_cost += cached_plan_cost(plan, true);
+
+ plansource->num_custom_plans++;
+ plansource->last_plan_type = PLAN_CACHE_TYPE_CUSTOM;
+ }
+ else
+ {
+ plansource->num_generic_plans++;
+ plansource->last_plan_type = PLAN_CACHE_TYPE_GENERIC;
}
Assert(plan != NULL);
@@ -1574,8 +1582,11 @@ CopyCachedPlan(CachedPlanSource *plansource)
/* We may as well copy any acquired cost knowledge */
newsource->generic_cost = plansource->generic_cost;
newsource->total_custom_cost = plansource->total_custom_cost;
+ newsource->num_generic_plans = plansource->num_generic_plans;
newsource->num_custom_plans = plansource->num_custom_plans;
+ newsource->last_plan_type = PLAN_CACHE_TYPE_NONE;
+
MemoryContextSwitchTo(oldcxt);
return newsource;
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 61f2c2f5b4..31a568d0cf 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -7743,9 +7743,9 @@
{ oid => '2510', descr => 'get the prepared statements for this session',
proname => 'pg_prepared_statement', prorows => '1000', proretset => 't',
provolatile => 's', proparallel => 'r', prorettype => 'record',
- proargtypes => '', proallargtypes => '{text,text,timestamptz,_regtype,bool}',
- proargmodes => '{o,o,o,o,o}',
- proargnames => '{name,statement,prepare_time,parameter_types,from_sql}',
+ proargtypes => '', proallargtypes => '{text,text,timestamptz,_regtype,bool,int8,int8,text}',
+ proargmodes => '{o,o,o,o,o,o,o,o}',
+ proargnames => '{name,statement,prepare_time,parameter_types,from_sql,generic_plans,custom_plans,last_plan_type}',
prosrc => 'pg_prepared_statement' },
{ oid => '2511', descr => 'get the open cursors for this session',
proname => 'pg_cursor', prorows => '1000', proretset => 't',
diff --git a/src/include/utils/plancache.h b/src/include/utils/plancache.h
index 522020d763..8cbdd1685b 100644
--- a/src/include/utils/plancache.h
+++ b/src/include/utils/plancache.h
@@ -34,6 +34,14 @@ typedef enum
PLAN_CACHE_MODE_FORCE_CUSTOM_PLAN
} PlanCacheMode;
+/* possible values for a plan type */
+typedef enum
+{
+ PLAN_CACHE_TYPE_NONE,
+ PLAN_CACHE_TYPE_CUSTOM,
+ PLAN_CACHE_TYPE_GENERIC
+} PlanCacheType;
+
/* GUC parameter */
extern int plan_cache_mode;
@@ -130,7 +138,9 @@ typedef struct CachedPlanSource
/* State kept to help decide whether to use custom or generic plans: */
double generic_cost; /* cost of generic plan, or -1 if not known */
double total_custom_cost; /* total cost of custom plans so far */
- int num_custom_plans; /* number of plans included in total */
+ int64 num_custom_plans; /* # of custom plans included in total */
+ int64 num_generic_plans; /* # of generic plans */
+ PlanCacheType last_plan_type; /* type of the last plan */
} CachedPlanSource;
/*
diff --git a/src/test/regress/expected/prepare.out b/src/test/regress/expected/prepare.out
index 3306c696b1..00e5e71fc1 100644
--- a/src/test/regress/expected/prepare.out
+++ b/src/test/regress/expected/prepare.out
@@ -58,12 +58,63 @@ SELECT name, statement, parameter_types FROM pg_prepared_statements;
PREPARE q2(text) AS
SELECT datname, datistemplate, datallowconn
FROM pg_database WHERE datname = $1;
+-- the view should return the inital state
+SELECT name, generic_plans, custom_plans, last_plan_type from pg_prepared_statements;
+ name | generic_plans | custom_plans | last_plan_type
+------+---------------+--------------+----------------
+ q2 | 0 | 0 |
+(1 row)
+
+EXECUTE q2('postgres');
+ datname | datistemplate | datallowconn
+----------+---------------+--------------
+ postgres | f | t
+(1 row)
+
+EXECUTE q2('postgres');
+ datname | datistemplate | datallowconn
+----------+---------------+--------------
+ postgres | f | t
+(1 row)
+
+EXECUTE q2('postgres');
+ datname | datistemplate | datallowconn
+----------+---------------+--------------
+ postgres | f | t
+(1 row)
+
+EXECUTE q2('postgres');
+ datname | datistemplate | datallowconn
+----------+---------------+--------------
+ postgres | f | t
+(1 row)
+
+EXECUTE q2('postgres');
+ datname | datistemplate | datallowconn
+----------+---------------+--------------
+ postgres | f | t
+(1 row)
+
+-- until here, all the plans are custom
+SELECT name, generic_plans, custom_plans, last_plan_type from pg_prepared_statements;
+ name | generic_plans | custom_plans | last_plan_type
+------+---------------+--------------+----------------
+ q2 | 0 | 5 | custom
+(1 row)
+
EXECUTE q2('postgres');
datname | datistemplate | datallowconn
----------+---------------+--------------
postgres | f | t
(1 row)
+-- latest plan shoud be generic
+SELECT name, generic_plans, custom_plans, last_plan_type from pg_prepared_statements;
+ name | generic_plans | custom_plans | last_plan_type
+------+---------------+--------------+----------------
+ q2 | 1 | 5 | generic
+(1 row)
+
PREPARE q3(text, int, float, boolean, smallint) AS
SELECT * FROM tenk1 WHERE string4 = $1 AND (four = $2 OR
ten = $3::bigint OR true = $4 OR odd = $5::int)
diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out
index b813e32215..a180e9fd2c 100644
--- a/src/test/regress/expected/rules.out
+++ b/src/test/regress/expected/rules.out
@@ -1428,8 +1428,11 @@ pg_prepared_statements| SELECT p.name,
p.statement,
p.prepare_time,
p.parameter_types,
- p.from_sql
- FROM pg_prepared_statement() p(name, statement, prepare_time, parameter_types, from_sql);
+ p.from_sql,
+ p.generic_plans,
+ p.custom_plans,
+ p.last_plan_type
+ FROM pg_prepared_statement() p(name, statement, prepare_time, parameter_types, from_sql, generic_plans, custom_plans, last_plan_type);
pg_prepared_xacts| SELECT p.transaction,
p.gid,
p.prepared,
diff --git a/src/test/regress/sql/prepare.sql b/src/test/regress/sql/prepare.sql
index 985d0f05c9..8112f8f6cd 100644
--- a/src/test/regress/sql/prepare.sql
+++ b/src/test/regress/sql/prepare.sql
@@ -34,8 +34,23 @@ PREPARE q2(text) AS
SELECT datname, datistemplate, datallowconn
FROM pg_database WHERE datname = $1;
+-- the view should return the inital state
+SELECT name, generic_plans, custom_plans, last_plan_type from pg_prepared_statements;
+
+EXECUTE q2('postgres');
+EXECUTE q2('postgres');
+EXECUTE q2('postgres');
+EXECUTE q2('postgres');
EXECUTE q2('postgres');
+-- until here, all the plans are custom
+SELECT name, generic_plans, custom_plans, last_plan_type from pg_prepared_statements;
+
+EXECUTE q2('postgres');
+
+-- latest plan shoud be generic
+SELECT name, generic_plans, custom_plans, last_plan_type from pg_prepared_statements;
+
PREPARE q3(text, int, float, boolean, smallint) AS
SELECT * FROM tenk1 WHERE string4 = $1 AND (four = $2 OR
ten = $3::bigint OR true = $4 OR odd = $5::int)
--
2.18.1
On 2020/06/11 14:59, torikoshia wrote:
On 2020-06-10 18:00, Kyotaro Horiguchi wrote:
+��� TupleDescInitEntry(tupdesc, (AttrNumber) 8, "last_plan",
This could be a problem if we showed the last plan in this view.� I
think "last_plan_type" would be better.+����������� if (prep_stmt->plansource->last_plan_type == PLAN_CACHE_TYPE_CUSTOM) +��������������� values[7] = CStringGetTextDatum("custom"); +����������� else if (prep_stmt->plansource->last_plan_type == PLAN_CACHE_TYPE_GENERIC) +��������������� values[7] = CStringGetTextDatum("generic"); +����������� else +��������������� nulls[7] = true;Using swith-case prevents future additional type (if any) from being
unhandled.� I think we are recommending that as a convension.Thanks for your reviewing!
I've attached a patch that reflects your comments.
Thanks for the patch! Here are the comments.
+ Number of times generic plan was choosen
+ Number of times custom plan was choosen
Typo: "choosen" should be "chosen"?
+ <entry role="catalog_table_entry"><para role="column_definition">
+ <structfield>last_plan_type</structfield> <type>text</type>
+ </para>
+ <para>
+ Tells the last plan type was generic or custom. If the prepared
+ statement has not executed yet, this field is null
+ </para></entry>
Could you tell me how this information is expected to be used?
I think that generic_plans and custom_plans are useful when investigating
the cause of performance drop by cached plan mode. But I failed to get
how much useful last_plan_type is.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
On 2020-07-06 22:16, Fujii Masao wrote:
On 2020/06/11 14:59, torikoshia wrote:
On 2020-06-10 18:00, Kyotaro Horiguchi wrote:
+ TupleDescInitEntry(tupdesc, (AttrNumber) 8, "last_plan",
This could be a problem if we showed the last plan in this view. I
think "last_plan_type" would be better.+ if (prep_stmt->plansource->last_plan_type == PLAN_CACHE_TYPE_CUSTOM) + values[7] = CStringGetTextDatum("custom"); + else if (prep_stmt->plansource->last_plan_type == PLAN_CACHE_TYPE_GENERIC) + values[7] = CStringGetTextDatum("generic"); + else + nulls[7] = true;Using swith-case prevents future additional type (if any) from being
unhandled. I think we are recommending that as a convension.Thanks for your reviewing!
I've attached a patch that reflects your comments.
Thanks for the patch! Here are the comments.
Thanks for your review!
+ Number of times generic plan was choosen + Number of times custom plan was choosenTypo: "choosen" should be "chosen"?
Thanks, fixed them.
+ <entry role="catalog_table_entry"><para role="column_definition"> + <structfield>last_plan_type</structfield> <type>text</type> + </para> + <para> + Tells the last plan type was generic or custom. If the prepared + statement has not executed yet, this field is null + </para></entry>Could you tell me how this information is expected to be used?
I think that generic_plans and custom_plans are useful when
investigating
the cause of performance drop by cached plan mode. But I failed to get
how much useful last_plan_type is.
This may be an exceptional case, but I once had a case needed
to ensure whether generic or custom plan was chosen for specific
queries in a development environment.
Of course, we can know it from adding EXPLAIN and ensuring whether $n
is contained in the plan, but I feel using the view is easier to use
and understand.
Regards,
--
Atsushi Torikoshi
NTT DATA CORPORATION
Attachments:
0006-Expose-counters-of-plancache-to-pg_prepared_statement.patchtext/x-diff; name=0006-Expose-counters-of-plancache-to-pg_prepared_statement.patchDownload
From f2155b1a9f3b500eca490827dd044f5c0f704dd3 Mon Sep 17 00:00:00 2001
From: Atsushi Torikoshi<torikoshia@oss.nttdata.com>
Date: Wed, 8 Jul 2020 09:17:22 +0900
Subject: [PATCH] We didn't have an easy way to find how many times generic or
custom plans of a prepared statement have been executed. This patch exposes
such numbers of both plans and the last plan type in pg_prepared_statements.
From 08183ee7ef827761f1ed38d2ab62b4f8f748baca Mon Sep 17 00:00:00 2001
---
doc/src/sgml/catalogs.sgml | 28 +++++++++++++++
src/backend/commands/prepare.c | 29 ++++++++++++---
src/backend/utils/cache/plancache.c | 23 ++++++++----
src/include/catalog/pg_proc.dat | 6 ++--
src/include/utils/plancache.h | 12 ++++++-
src/test/regress/expected/prepare.out | 51 +++++++++++++++++++++++++++
src/test/regress/expected/rules.out | 7 ++--
src/test/regress/sql/prepare.sql | 15 ++++++++
8 files changed, 155 insertions(+), 16 deletions(-)
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 003d278370..146dc2208b 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -10829,6 +10829,34 @@ SELECT * FROM pg_locks pl LEFT JOIN pg_prepared_xacts ppx
frontend/backend protocol
</para></entry>
</row>
+
+ <row>
+ <entry role="catalog_table_entry"><para role="column_definition">
+ <structfield>generic_plans</structfield> <type>bigint</type>
+ </para>
+ <para>
+ Number of times generic plan was chosen
+ </para></entry>
+ </row>
+
+ <row>
+ <entry role="catalog_table_entry"><para role="column_definition">
+ <structfield>custom_plans</structfield> <type>bigint</type>
+ </para>
+ <para>
+ Number of times custom plan was chosen
+ </para></entry>
+ </row>
+
+ <row>
+ <entry role="catalog_table_entry"><para role="column_definition">
+ <structfield>last_plan_type</structfield> <type>text</type>
+ </para>
+ <para>
+ Tells the last plan type was generic or custom. If the prepared
+ statement has not executed yet, this field is null
+ </para></entry>
+ </row>
</tbody>
</tgroup>
</table>
diff --git a/src/backend/commands/prepare.c b/src/backend/commands/prepare.c
index 80d6df8ac1..b4b1865d48 100644
--- a/src/backend/commands/prepare.c
+++ b/src/backend/commands/prepare.c
@@ -694,7 +694,8 @@ ExplainExecuteQuery(ExecuteStmt *execstmt, IntoClause *into, ExplainState *es,
/*
* This set returning function reads all the prepared statements and
- * returns a set of (name, statement, prepare_time, param_types, from_sql).
+ * returns a set of (name, statement, prepare_time, param_types, from_sql,
+ * generic_plans, custom_plans, last_plan_type).
*/
Datum
pg_prepared_statement(PG_FUNCTION_ARGS)
@@ -723,7 +724,7 @@ pg_prepared_statement(PG_FUNCTION_ARGS)
* build tupdesc for result tuples. This must match the definition of the
* pg_prepared_statements view in system_views.sql
*/
- tupdesc = CreateTemplateTupleDesc(5);
+ tupdesc = CreateTemplateTupleDesc(8);
TupleDescInitEntry(tupdesc, (AttrNumber) 1, "name",
TEXTOID, -1, 0);
TupleDescInitEntry(tupdesc, (AttrNumber) 2, "statement",
@@ -734,6 +735,12 @@ pg_prepared_statement(PG_FUNCTION_ARGS)
REGTYPEARRAYOID, -1, 0);
TupleDescInitEntry(tupdesc, (AttrNumber) 5, "from_sql",
BOOLOID, -1, 0);
+ TupleDescInitEntry(tupdesc, (AttrNumber) 6, "generic_plans",
+ INT8OID, -1, 0);
+ TupleDescInitEntry(tupdesc, (AttrNumber) 7, "custom_plans",
+ INT8OID, -1, 0);
+ TupleDescInitEntry(tupdesc, (AttrNumber) 8, "last_plan_type",
+ TEXTOID, -1, 0);
/*
* We put all the tuples into a tuplestore in one scan of the hashtable.
@@ -755,8 +762,8 @@ pg_prepared_statement(PG_FUNCTION_ARGS)
hash_seq_init(&hash_seq, prepared_queries);
while ((prep_stmt = hash_seq_search(&hash_seq)) != NULL)
{
- Datum values[5];
- bool nulls[5];
+ Datum values[8];
+ bool nulls[8];
MemSet(nulls, 0, sizeof(nulls));
@@ -766,6 +773,20 @@ pg_prepared_statement(PG_FUNCTION_ARGS)
values[3] = build_regtype_array(prep_stmt->plansource->param_types,
prep_stmt->plansource->num_params);
values[4] = BoolGetDatum(prep_stmt->from_sql);
+ values[5] = Int64GetDatumFast(prep_stmt->plansource->num_generic_plans);
+ values[6] = Int64GetDatumFast(prep_stmt->plansource->num_custom_plans);
+
+ switch (prep_stmt->plansource->last_plan_type)
+ {
+ case PLAN_CACHE_TYPE_CUSTOM:
+ values[7] = CStringGetTextDatum("custom");
+ break;
+ case PLAN_CACHE_TYPE_GENERIC:
+ values[7] = CStringGetTextDatum("generic");
+ break;
+ default:
+ nulls[7] = true;
+ }
tuplestore_putvalues(tupstore, tupdesc, values, nulls);
}
diff --git a/src/backend/utils/cache/plancache.c b/src/backend/utils/cache/plancache.c
index 75b475c179..fcba6c1361 100644
--- a/src/backend/utils/cache/plancache.c
+++ b/src/backend/utils/cache/plancache.c
@@ -218,7 +218,9 @@ CreateCachedPlan(RawStmt *raw_parse_tree,
plansource->generation = 0;
plansource->generic_cost = -1;
plansource->total_custom_cost = 0;
+ plansource->num_generic_plans = 0;
plansource->num_custom_plans = 0;
+ plansource->last_plan_type = PLAN_CACHE_TYPE_NONE;
MemoryContextSwitchTo(oldcxt);
@@ -285,7 +287,9 @@ CreateOneShotCachedPlan(RawStmt *raw_parse_tree,
plansource->generation = 0;
plansource->generic_cost = -1;
plansource->total_custom_cost = 0;
+ plansource->num_generic_plans = 0;
plansource->num_custom_plans = 0;
+ plansource->last_plan_type = PLAN_CACHE_TYPE_NONE;
return plansource;
}
@@ -1213,12 +1217,16 @@ GetCachedPlan(CachedPlanSource *plansource, ParamListInfo boundParams,
{
/* Build a custom plan */
plan = BuildCachedPlan(plansource, qlist, boundParams, queryEnv);
- /* Accumulate total costs of custom plans, but 'ware overflow */
- if (plansource->num_custom_plans < INT_MAX)
- {
- plansource->total_custom_cost += cached_plan_cost(plan, true);
- plansource->num_custom_plans++;
- }
+ /* Accumulate total costs of custom plans */
+ plansource->total_custom_cost += cached_plan_cost(plan, true);
+
+ plansource->num_custom_plans++;
+ plansource->last_plan_type = PLAN_CACHE_TYPE_CUSTOM;
+ }
+ else
+ {
+ plansource->num_generic_plans++;
+ plansource->last_plan_type = PLAN_CACHE_TYPE_GENERIC;
}
Assert(plan != NULL);
@@ -1574,8 +1582,11 @@ CopyCachedPlan(CachedPlanSource *plansource)
/* We may as well copy any acquired cost knowledge */
newsource->generic_cost = plansource->generic_cost;
newsource->total_custom_cost = plansource->total_custom_cost;
+ newsource->num_generic_plans = plansource->num_generic_plans;
newsource->num_custom_plans = plansource->num_custom_plans;
+ newsource->last_plan_type = PLAN_CACHE_TYPE_NONE;
+
MemoryContextSwitchTo(oldcxt);
return newsource;
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 38295aca48..61ef8a1560 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -7746,9 +7746,9 @@
{ oid => '2510', descr => 'get the prepared statements for this session',
proname => 'pg_prepared_statement', prorows => '1000', proretset => 't',
provolatile => 's', proparallel => 'r', prorettype => 'record',
- proargtypes => '', proallargtypes => '{text,text,timestamptz,_regtype,bool}',
- proargmodes => '{o,o,o,o,o}',
- proargnames => '{name,statement,prepare_time,parameter_types,from_sql}',
+ proargtypes => '', proallargtypes => '{text,text,timestamptz,_regtype,bool,int8,int8,text}',
+ proargmodes => '{o,o,o,o,o,o,o,o}',
+ proargnames => '{name,statement,prepare_time,parameter_types,from_sql,generic_plans,custom_plans,last_plan_type}',
prosrc => 'pg_prepared_statement' },
{ oid => '2511', descr => 'get the open cursors for this session',
proname => 'pg_cursor', prorows => '1000', proretset => 't',
diff --git a/src/include/utils/plancache.h b/src/include/utils/plancache.h
index 522020d763..8cbdd1685b 100644
--- a/src/include/utils/plancache.h
+++ b/src/include/utils/plancache.h
@@ -34,6 +34,14 @@ typedef enum
PLAN_CACHE_MODE_FORCE_CUSTOM_PLAN
} PlanCacheMode;
+/* possible values for a plan type */
+typedef enum
+{
+ PLAN_CACHE_TYPE_NONE,
+ PLAN_CACHE_TYPE_CUSTOM,
+ PLAN_CACHE_TYPE_GENERIC
+} PlanCacheType;
+
/* GUC parameter */
extern int plan_cache_mode;
@@ -130,7 +138,9 @@ typedef struct CachedPlanSource
/* State kept to help decide whether to use custom or generic plans: */
double generic_cost; /* cost of generic plan, or -1 if not known */
double total_custom_cost; /* total cost of custom plans so far */
- int num_custom_plans; /* number of plans included in total */
+ int64 num_custom_plans; /* # of custom plans included in total */
+ int64 num_generic_plans; /* # of generic plans */
+ PlanCacheType last_plan_type; /* type of the last plan */
} CachedPlanSource;
/*
diff --git a/src/test/regress/expected/prepare.out b/src/test/regress/expected/prepare.out
index 3306c696b1..00e5e71fc1 100644
--- a/src/test/regress/expected/prepare.out
+++ b/src/test/regress/expected/prepare.out
@@ -58,12 +58,63 @@ SELECT name, statement, parameter_types FROM pg_prepared_statements;
PREPARE q2(text) AS
SELECT datname, datistemplate, datallowconn
FROM pg_database WHERE datname = $1;
+-- the view should return the inital state
+SELECT name, generic_plans, custom_plans, last_plan_type from pg_prepared_statements;
+ name | generic_plans | custom_plans | last_plan_type
+------+---------------+--------------+----------------
+ q2 | 0 | 0 |
+(1 row)
+
+EXECUTE q2('postgres');
+ datname | datistemplate | datallowconn
+----------+---------------+--------------
+ postgres | f | t
+(1 row)
+
+EXECUTE q2('postgres');
+ datname | datistemplate | datallowconn
+----------+---------------+--------------
+ postgres | f | t
+(1 row)
+
+EXECUTE q2('postgres');
+ datname | datistemplate | datallowconn
+----------+---------------+--------------
+ postgres | f | t
+(1 row)
+
+EXECUTE q2('postgres');
+ datname | datistemplate | datallowconn
+----------+---------------+--------------
+ postgres | f | t
+(1 row)
+
+EXECUTE q2('postgres');
+ datname | datistemplate | datallowconn
+----------+---------------+--------------
+ postgres | f | t
+(1 row)
+
+-- until here, all the plans are custom
+SELECT name, generic_plans, custom_plans, last_plan_type from pg_prepared_statements;
+ name | generic_plans | custom_plans | last_plan_type
+------+---------------+--------------+----------------
+ q2 | 0 | 5 | custom
+(1 row)
+
EXECUTE q2('postgres');
datname | datistemplate | datallowconn
----------+---------------+--------------
postgres | f | t
(1 row)
+-- latest plan shoud be generic
+SELECT name, generic_plans, custom_plans, last_plan_type from pg_prepared_statements;
+ name | generic_plans | custom_plans | last_plan_type
+------+---------------+--------------+----------------
+ q2 | 1 | 5 | generic
+(1 row)
+
PREPARE q3(text, int, float, boolean, smallint) AS
SELECT * FROM tenk1 WHERE string4 = $1 AND (four = $2 OR
ten = $3::bigint OR true = $4 OR odd = $5::int)
diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out
index b813e32215..a180e9fd2c 100644
--- a/src/test/regress/expected/rules.out
+++ b/src/test/regress/expected/rules.out
@@ -1428,8 +1428,11 @@ pg_prepared_statements| SELECT p.name,
p.statement,
p.prepare_time,
p.parameter_types,
- p.from_sql
- FROM pg_prepared_statement() p(name, statement, prepare_time, parameter_types, from_sql);
+ p.from_sql,
+ p.generic_plans,
+ p.custom_plans,
+ p.last_plan_type
+ FROM pg_prepared_statement() p(name, statement, prepare_time, parameter_types, from_sql, generic_plans, custom_plans, last_plan_type);
pg_prepared_xacts| SELECT p.transaction,
p.gid,
p.prepared,
diff --git a/src/test/regress/sql/prepare.sql b/src/test/regress/sql/prepare.sql
index 985d0f05c9..8112f8f6cd 100644
--- a/src/test/regress/sql/prepare.sql
+++ b/src/test/regress/sql/prepare.sql
@@ -34,8 +34,23 @@ PREPARE q2(text) AS
SELECT datname, datistemplate, datallowconn
FROM pg_database WHERE datname = $1;
+-- the view should return the inital state
+SELECT name, generic_plans, custom_plans, last_plan_type from pg_prepared_statements;
+
+EXECUTE q2('postgres');
+EXECUTE q2('postgres');
+EXECUTE q2('postgres');
+EXECUTE q2('postgres');
EXECUTE q2('postgres');
+-- until here, all the plans are custom
+SELECT name, generic_plans, custom_plans, last_plan_type from pg_prepared_statements;
+
+EXECUTE q2('postgres');
+
+-- latest plan shoud be generic
+SELECT name, generic_plans, custom_plans, last_plan_type from pg_prepared_statements;
+
PREPARE q3(text, int, float, boolean, smallint) AS
SELECT * FROM tenk1 WHERE string4 = $1 AND (four = $2 OR
ten = $3::bigint OR true = $4 OR odd = $5::int)
--
2.18.1
On 2020/07/08 10:14, torikoshia wrote:
On 2020-07-06 22:16, Fujii Masao wrote:
On 2020/06/11 14:59, torikoshia wrote:
On 2020-06-10 18:00, Kyotaro Horiguchi wrote:
+ TupleDescInitEntry(tupdesc, (AttrNumber) 8, "last_plan",
This could be a problem if we showed the last plan in this view. I
think "last_plan_type" would be better.+ if (prep_stmt->plansource->last_plan_type == PLAN_CACHE_TYPE_CUSTOM) + values[7] = CStringGetTextDatum("custom"); + else if (prep_stmt->plansource->last_plan_type == PLAN_CACHE_TYPE_GENERIC) + values[7] = CStringGetTextDatum("generic"); + else + nulls[7] = true;Using swith-case prevents future additional type (if any) from being
unhandled. I think we are recommending that as a convension.Thanks for your reviewing!
I've attached a patch that reflects your comments.
Thanks for the patch! Here are the comments.
Thanks for your review!
+ Number of times generic plan was choosen + Number of times custom plan was choosenTypo: "choosen" should be "chosen"?
Thanks, fixed them.
+ <entry role="catalog_table_entry"><para role="column_definition"> + <structfield>last_plan_type</structfield> <type>text</type> + </para> + <para> + Tells the last plan type was generic or custom. If the prepared + statement has not executed yet, this field is null + </para></entry>Could you tell me how this information is expected to be used?
I think that generic_plans and custom_plans are useful when investigating
the cause of performance drop by cached plan mode. But I failed to get
how much useful last_plan_type is.This may be an exceptional case, but I once had a case needed
to ensure whether generic or custom plan was chosen for specific
queries in a development environment.
In your case, probably you had to ensure that the last multiple (or every)
executions chose generic or custom plan? If yes, I'm afraid that displaying
only the last plan mode is not enough for your case. No?
So it seems better to check generic_plans or custom_plans columns in the
view rather than last_plan_type even in your case. Thought?
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
On 2020-07-08 16:41, Fujii Masao wrote:
On 2020/07/08 10:14, torikoshia wrote:
On 2020-07-06 22:16, Fujii Masao wrote:
On 2020/06/11 14:59, torikoshia wrote:
On 2020-06-10 18:00, Kyotaro Horiguchi wrote:
+ TupleDescInitEntry(tupdesc, (AttrNumber) 8, "last_plan",
This could be a problem if we showed the last plan in this view. I
think "last_plan_type" would be better.+ if (prep_stmt->plansource->last_plan_type == PLAN_CACHE_TYPE_CUSTOM) + values[7] = CStringGetTextDatum("custom"); + else if (prep_stmt->plansource->last_plan_type == PLAN_CACHE_TYPE_GENERIC) + values[7] = CStringGetTextDatum("generic"); + else + nulls[7] = true;Using swith-case prevents future additional type (if any) from
being
unhandled. I think we are recommending that as a convension.Thanks for your reviewing!
I've attached a patch that reflects your comments.
Thanks for the patch! Here are the comments.
Thanks for your review!
+ Number of times generic plan was choosen + Number of times custom plan was choosenTypo: "choosen" should be "chosen"?
Thanks, fixed them.
+ <entry role="catalog_table_entry"><para role="column_definition"> + <structfield>last_plan_type</structfield> <type>text</type> + </para> + <para> + Tells the last plan type was generic or custom. If the prepared + statement has not executed yet, this field is null + </para></entry>Could you tell me how this information is expected to be used?
I think that generic_plans and custom_plans are useful when
investigating
the cause of performance drop by cached plan mode. But I failed to
get
how much useful last_plan_type is.This may be an exceptional case, but I once had a case needed
to ensure whether generic or custom plan was chosen for specific
queries in a development environment.In your case, probably you had to ensure that the last multiple (or
every)
executions chose generic or custom plan? If yes, I'm afraid that
displaying
only the last plan mode is not enough for your case. No?
So it seems better to check generic_plans or custom_plans columns in
the
view rather than last_plan_type even in your case. Thought?
Yeah, I now feel last_plan is not so necessary and only the numbers of
generic/custom plan is enough.
If there are no objections, I'm going to remove this column and related
codes.
Regards,
--
Atsushi Torikoshi
NTT DATA CORPORATION
On 2020-07-10 10:49, torikoshia wrote:
On 2020-07-08 16:41, Fujii Masao wrote:
On 2020/07/08 10:14, torikoshia wrote:
On 2020-07-06 22:16, Fujii Masao wrote:
On 2020/06/11 14:59, torikoshia wrote:
On 2020-06-10 18:00, Kyotaro Horiguchi wrote:
+ TupleDescInitEntry(tupdesc, (AttrNumber) 8, "last_plan",
This could be a problem if we showed the last plan in this view.
I
think "last_plan_type" would be better.+ if (prep_stmt->plansource->last_plan_type == PLAN_CACHE_TYPE_CUSTOM) + values[7] = CStringGetTextDatum("custom"); + else if (prep_stmt->plansource->last_plan_type == PLAN_CACHE_TYPE_GENERIC) + values[7] = CStringGetTextDatum("generic"); + else + nulls[7] = true;Using swith-case prevents future additional type (if any) from
being
unhandled. I think we are recommending that as a convension.Thanks for your reviewing!
I've attached a patch that reflects your comments.
Thanks for the patch! Here are the comments.
Thanks for your review!
+ Number of times generic plan was choosen + Number of times custom plan was choosenTypo: "choosen" should be "chosen"?
Thanks, fixed them.
+ <entry role="catalog_table_entry"><para role="column_definition"> + <structfield>last_plan_type</structfield> <type>text</type> + </para> + <para> + Tells the last plan type was generic or custom. If the prepared + statement has not executed yet, this field is null + </para></entry>Could you tell me how this information is expected to be used?
I think that generic_plans and custom_plans are useful when
investigating
the cause of performance drop by cached plan mode. But I failed to
get
how much useful last_plan_type is.This may be an exceptional case, but I once had a case needed
to ensure whether generic or custom plan was chosen for specific
queries in a development environment.In your case, probably you had to ensure that the last multiple (or
every)
executions chose generic or custom plan? If yes, I'm afraid that
displaying
only the last plan mode is not enough for your case. No?
So it seems better to check generic_plans or custom_plans columns in
the
view rather than last_plan_type even in your case. Thought?Yeah, I now feel last_plan is not so necessary and only the numbers of
generic/custom plan is enough.If there are no objections, I'm going to remove this column and related
codes.
As mentioned, I removed last_plan column.
Regards,
--
Atsushi Torikoshi
NTT DATA CORPORATION
Attachments:
0007-Expose-counters-of-plancache-to-pg_prepared_statement.patchtext/x-diff; name=0007-Expose-counters-of-plancache-to-pg_prepared_statement.patchDownload
From e417fc58d51ab0c06de34a563d580c7d4017a1db Mon Sep 17 00:00:00 2001
From: Atsushi Torikoshi <torikoshia@oss.nttdata.com>
Date: Tue, 14 Jul 2020 20:57:16 +0900
Subject: [PATCH] We didn't have an easy way to find how many times generic and
custom plans of a prepared statement have been executed. This patch exposes
the numbers of both plans in pg_prepared_statements.
---
doc/src/sgml/catalogs.sgml | 18 ++++++++++
src/backend/commands/prepare.c | 15 +++++---
src/backend/utils/cache/plancache.c | 17 +++++----
src/include/catalog/pg_proc.dat | 6 ++--
src/include/utils/plancache.h | 3 +-
src/test/regress/expected/prepare.out | 51 +++++++++++++++++++++++++++
src/test/regress/expected/rules.out | 6 ++--
src/test/regress/sql/prepare.sql | 15 ++++++++
8 files changed, 115 insertions(+), 16 deletions(-)
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index e9cdff4864..8bc6d685f4 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -10830,6 +10830,24 @@ SELECT * FROM pg_locks pl LEFT JOIN pg_prepared_xacts ppx
frontend/backend protocol
</para></entry>
</row>
+
+ <row>
+ <entry role="catalog_table_entry"><para role="column_definition">
+ <structfield>generic_plans</structfield> <type>int8</type>
+ </para>
+ <para>
+ Number of times generic plan was chosen
+ </para></entry>
+ </row>
+
+ <row>
+ <entry role="catalog_table_entry"><para role="column_definition">
+ <structfield>custom_plans</structfield> <type>int8</type>
+ </para>
+ <para>
+ Number of times custom plan was chosen
+ </para></entry>
+ </row>
</tbody>
</tgroup>
</table>
diff --git a/src/backend/commands/prepare.c b/src/backend/commands/prepare.c
index 80d6df8ac1..4b18be5b27 100644
--- a/src/backend/commands/prepare.c
+++ b/src/backend/commands/prepare.c
@@ -694,7 +694,8 @@ ExplainExecuteQuery(ExecuteStmt *execstmt, IntoClause *into, ExplainState *es,
/*
* This set returning function reads all the prepared statements and
- * returns a set of (name, statement, prepare_time, param_types, from_sql).
+ * returns a set of (name, statement, prepare_time, param_types, from_sql,
+ * generic_plans, custom_plans).
*/
Datum
pg_prepared_statement(PG_FUNCTION_ARGS)
@@ -723,7 +724,7 @@ pg_prepared_statement(PG_FUNCTION_ARGS)
* build tupdesc for result tuples. This must match the definition of the
* pg_prepared_statements view in system_views.sql
*/
- tupdesc = CreateTemplateTupleDesc(5);
+ tupdesc = CreateTemplateTupleDesc(7);
TupleDescInitEntry(tupdesc, (AttrNumber) 1, "name",
TEXTOID, -1, 0);
TupleDescInitEntry(tupdesc, (AttrNumber) 2, "statement",
@@ -734,6 +735,10 @@ pg_prepared_statement(PG_FUNCTION_ARGS)
REGTYPEARRAYOID, -1, 0);
TupleDescInitEntry(tupdesc, (AttrNumber) 5, "from_sql",
BOOLOID, -1, 0);
+ TupleDescInitEntry(tupdesc, (AttrNumber) 6, "generic_plans",
+ INT8OID, -1, 0);
+ TupleDescInitEntry(tupdesc, (AttrNumber) 7, "custom_plans",
+ INT8OID, -1, 0);
/*
* We put all the tuples into a tuplestore in one scan of the hashtable.
@@ -755,8 +760,8 @@ pg_prepared_statement(PG_FUNCTION_ARGS)
hash_seq_init(&hash_seq, prepared_queries);
while ((prep_stmt = hash_seq_search(&hash_seq)) != NULL)
{
- Datum values[5];
- bool nulls[5];
+ Datum values[7];
+ bool nulls[7];
MemSet(nulls, 0, sizeof(nulls));
@@ -766,6 +771,8 @@ pg_prepared_statement(PG_FUNCTION_ARGS)
values[3] = build_regtype_array(prep_stmt->plansource->param_types,
prep_stmt->plansource->num_params);
values[4] = BoolGetDatum(prep_stmt->from_sql);
+ values[5] = Int64GetDatumFast(prep_stmt->plansource->num_generic_plans);
+ values[6] = Int64GetDatumFast(prep_stmt->plansource->num_custom_plans);
tuplestore_putvalues(tupstore, tupdesc, values, nulls);
}
diff --git a/src/backend/utils/cache/plancache.c b/src/backend/utils/cache/plancache.c
index 75b475c179..50d6ad28b4 100644
--- a/src/backend/utils/cache/plancache.c
+++ b/src/backend/utils/cache/plancache.c
@@ -218,6 +218,7 @@ CreateCachedPlan(RawStmt *raw_parse_tree,
plansource->generation = 0;
plansource->generic_cost = -1;
plansource->total_custom_cost = 0;
+ plansource->num_generic_plans = 0;
plansource->num_custom_plans = 0;
MemoryContextSwitchTo(oldcxt);
@@ -285,6 +286,7 @@ CreateOneShotCachedPlan(RawStmt *raw_parse_tree,
plansource->generation = 0;
plansource->generic_cost = -1;
plansource->total_custom_cost = 0;
+ plansource->num_generic_plans = 0;
plansource->num_custom_plans = 0;
return plansource;
@@ -1213,12 +1215,14 @@ GetCachedPlan(CachedPlanSource *plansource, ParamListInfo boundParams,
{
/* Build a custom plan */
plan = BuildCachedPlan(plansource, qlist, boundParams, queryEnv);
- /* Accumulate total costs of custom plans, but 'ware overflow */
- if (plansource->num_custom_plans < INT_MAX)
- {
- plansource->total_custom_cost += cached_plan_cost(plan, true);
- plansource->num_custom_plans++;
- }
+ /* Accumulate total costs of custom plans */
+ plansource->total_custom_cost += cached_plan_cost(plan, true);
+
+ plansource->num_custom_plans++;
+ }
+ else
+ {
+ plansource->num_generic_plans++;
}
Assert(plan != NULL);
@@ -1574,6 +1578,7 @@ CopyCachedPlan(CachedPlanSource *plansource)
/* We may as well copy any acquired cost knowledge */
newsource->generic_cost = plansource->generic_cost;
newsource->total_custom_cost = plansource->total_custom_cost;
+ newsource->num_generic_plans = plansource->num_generic_plans;
newsource->num_custom_plans = plansource->num_custom_plans;
MemoryContextSwitchTo(oldcxt);
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 95604e988a..4b5af32440 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -7755,9 +7755,9 @@
{ oid => '2510', descr => 'get the prepared statements for this session',
proname => 'pg_prepared_statement', prorows => '1000', proretset => 't',
provolatile => 's', proparallel => 'r', prorettype => 'record',
- proargtypes => '', proallargtypes => '{text,text,timestamptz,_regtype,bool}',
- proargmodes => '{o,o,o,o,o}',
- proargnames => '{name,statement,prepare_time,parameter_types,from_sql}',
+ proargtypes => '', proallargtypes => '{text,text,timestamptz,_regtype,bool,int8,int8}',
+ proargmodes => '{o,o,o,o,o,o,o}',
+ proargnames => '{name,statement,prepare_time,parameter_types,from_sql,generic_plans,custom_plans}',
prosrc => 'pg_prepared_statement' },
{ oid => '2511', descr => 'get the open cursors for this session',
proname => 'pg_cursor', prorows => '1000', proretset => 't',
diff --git a/src/include/utils/plancache.h b/src/include/utils/plancache.h
index 522020d763..4901568553 100644
--- a/src/include/utils/plancache.h
+++ b/src/include/utils/plancache.h
@@ -130,7 +130,8 @@ typedef struct CachedPlanSource
/* State kept to help decide whether to use custom or generic plans: */
double generic_cost; /* cost of generic plan, or -1 if not known */
double total_custom_cost; /* total cost of custom plans so far */
- int num_custom_plans; /* number of plans included in total */
+ int64 num_custom_plans; /* # of custom plans included in total */
+ int64 num_generic_plans; /* # of generic plans */
} CachedPlanSource;
/*
diff --git a/src/test/regress/expected/prepare.out b/src/test/regress/expected/prepare.out
index 3306c696b1..57b5272574 100644
--- a/src/test/regress/expected/prepare.out
+++ b/src/test/regress/expected/prepare.out
@@ -58,12 +58,63 @@ SELECT name, statement, parameter_types FROM pg_prepared_statements;
PREPARE q2(text) AS
SELECT datname, datistemplate, datallowconn
FROM pg_database WHERE datname = $1;
+-- the view should return the inital state
+SELECT name, generic_plans, custom_plans from pg_prepared_statements;
+ name | generic_plans | custom_plans
+------+---------------+--------------
+ q2 | 0 | 0
+(1 row)
+
+EXECUTE q2('postgres');
+ datname | datistemplate | datallowconn
+----------+---------------+--------------
+ postgres | f | t
+(1 row)
+
+EXECUTE q2('postgres');
+ datname | datistemplate | datallowconn
+----------+---------------+--------------
+ postgres | f | t
+(1 row)
+
+EXECUTE q2('postgres');
+ datname | datistemplate | datallowconn
+----------+---------------+--------------
+ postgres | f | t
+(1 row)
+
+EXECUTE q2('postgres');
+ datname | datistemplate | datallowconn
+----------+---------------+--------------
+ postgres | f | t
+(1 row)
+
+EXECUTE q2('postgres');
+ datname | datistemplate | datallowconn
+----------+---------------+--------------
+ postgres | f | t
+(1 row)
+
+-- until here, all the plans are custom
+SELECT name, generic_plans, custom_plans from pg_prepared_statements;
+ name | generic_plans | custom_plans
+------+---------------+--------------
+ q2 | 0 | 5
+(1 row)
+
EXECUTE q2('postgres');
datname | datistemplate | datallowconn
----------+---------------+--------------
postgres | f | t
(1 row)
+-- latest plan shoud be generic
+SELECT name, generic_plans, custom_plans from pg_prepared_statements;
+ name | generic_plans | custom_plans
+------+---------------+--------------
+ q2 | 1 | 5
+(1 row)
+
PREPARE q3(text, int, float, boolean, smallint) AS
SELECT * FROM tenk1 WHERE string4 = $1 AND (four = $2 OR
ten = $3::bigint OR true = $4 OR odd = $5::int)
diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out
index fa436f2caa..601734a6f1 100644
--- a/src/test/regress/expected/rules.out
+++ b/src/test/regress/expected/rules.out
@@ -1428,8 +1428,10 @@ pg_prepared_statements| SELECT p.name,
p.statement,
p.prepare_time,
p.parameter_types,
- p.from_sql
- FROM pg_prepared_statement() p(name, statement, prepare_time, parameter_types, from_sql);
+ p.from_sql,
+ p.generic_plans,
+ p.custom_plans
+ FROM pg_prepared_statement() p(name, statement, prepare_time, parameter_types, from_sql, generic_plans, custom_plans);
pg_prepared_xacts| SELECT p.transaction,
p.gid,
p.prepared,
diff --git a/src/test/regress/sql/prepare.sql b/src/test/regress/sql/prepare.sql
index 985d0f05c9..8a4b546b97 100644
--- a/src/test/regress/sql/prepare.sql
+++ b/src/test/regress/sql/prepare.sql
@@ -34,8 +34,23 @@ PREPARE q2(text) AS
SELECT datname, datistemplate, datallowconn
FROM pg_database WHERE datname = $1;
+-- the view should return the inital state
+SELECT name, generic_plans, custom_plans from pg_prepared_statements;
+
+EXECUTE q2('postgres');
+EXECUTE q2('postgres');
+EXECUTE q2('postgres');
+EXECUTE q2('postgres');
EXECUTE q2('postgres');
+-- until here, all the plans are custom
+SELECT name, generic_plans, custom_plans from pg_prepared_statements;
+
+EXECUTE q2('postgres');
+
+-- latest plan shoud be generic
+SELECT name, generic_plans, custom_plans from pg_prepared_statements;
+
PREPARE q3(text, int, float, boolean, smallint) AS
SELECT * FROM tenk1 WHERE string4 = $1 AND (four = $2 OR
ten = $3::bigint OR true = $4 OR odd = $5::int)
--
2.18.1
On 2020/07/14 21:24, torikoshia wrote:
On 2020-07-10 10:49, torikoshia wrote:
On 2020-07-08 16:41, Fujii Masao wrote:
On 2020/07/08 10:14, torikoshia wrote:
On 2020-07-06 22:16, Fujii Masao wrote:
On 2020/06/11 14:59, torikoshia wrote:
On 2020-06-10 18:00, Kyotaro Horiguchi wrote:
+ TupleDescInitEntry(tupdesc, (AttrNumber) 8, "last_plan",
This could be a problem if we showed the last plan in this view. I
think "last_plan_type" would be better.+ if (prep_stmt->plansource->last_plan_type == PLAN_CACHE_TYPE_CUSTOM) + values[7] = CStringGetTextDatum("custom"); + else if (prep_stmt->plansource->last_plan_type == PLAN_CACHE_TYPE_GENERIC) + values[7] = CStringGetTextDatum("generic"); + else + nulls[7] = true;Using swith-case prevents future additional type (if any) from being
unhandled. I think we are recommending that as a convension.Thanks for your reviewing!
I've attached a patch that reflects your comments.
Thanks for the patch! Here are the comments.
Thanks for your review!
+ Number of times generic plan was choosen + Number of times custom plan was choosenTypo: "choosen" should be "chosen"?
Thanks, fixed them.
+ <entry role="catalog_table_entry"><para role="column_definition"> + <structfield>last_plan_type</structfield> <type>text</type> + </para> + <para> + Tells the last plan type was generic or custom. If the prepared + statement has not executed yet, this field is null + </para></entry>Could you tell me how this information is expected to be used?
I think that generic_plans and custom_plans are useful when investigating
the cause of performance drop by cached plan mode. But I failed to get
how much useful last_plan_type is.This may be an exceptional case, but I once had a case needed
to ensure whether generic or custom plan was chosen for specific
queries in a development environment.In your case, probably you had to ensure that the last multiple (or every)
executions chose generic or custom plan? If yes, I'm afraid that displaying
only the last plan mode is not enough for your case. No?
So it seems better to check generic_plans or custom_plans columns in the
view rather than last_plan_type even in your case. Thought?Yeah, I now feel last_plan is not so necessary and only the numbers of
generic/custom plan is enough.If there are no objections, I'm going to remove this column and related codes.
As mentioned, I removed last_plan column.
Thanks for updating the patch! It basically looks good to me.
I have one comment; you added the regression tests for generic and
custom plans into prepare.sql. But the similar tests already exist in
plancache.sql. So isn't it better to add the tests for generic_plans and
custom_plans columns, into plancache.sql?
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
On 2020-07-15 11:44, Fujii Masao wrote:
On 2020/07/14 21:24, torikoshia wrote:
On 2020-07-10 10:49, torikoshia wrote:
On 2020-07-08 16:41, Fujii Masao wrote:
On 2020/07/08 10:14, torikoshia wrote:
On 2020-07-06 22:16, Fujii Masao wrote:
On 2020/06/11 14:59, torikoshia wrote:
On 2020-06-10 18:00, Kyotaro Horiguchi wrote:
+ TupleDescInitEntry(tupdesc, (AttrNumber) 8, "last_plan",
This could be a problem if we showed the last plan in this view.
I
think "last_plan_type" would be better.+ if (prep_stmt->plansource->last_plan_type == PLAN_CACHE_TYPE_CUSTOM) + values[7] = CStringGetTextDatum("custom"); + else if (prep_stmt->plansource->last_plan_type == PLAN_CACHE_TYPE_GENERIC) + values[7] = CStringGetTextDatum("generic"); + else + nulls[7] = true;Using swith-case prevents future additional type (if any) from
being
unhandled. I think we are recommending that as a convension.Thanks for your reviewing!
I've attached a patch that reflects your comments.
Thanks for the patch! Here are the comments.
Thanks for your review!
+ Number of times generic plan was choosen + Number of times custom plan was choosenTypo: "choosen" should be "chosen"?
Thanks, fixed them.
+ <entry role="catalog_table_entry"><para role="column_definition"> + <structfield>last_plan_type</structfield> <type>text</type> + </para> + <para> + Tells the last plan type was generic or custom. If the prepared + statement has not executed yet, this field is null + </para></entry>Could you tell me how this information is expected to be used?
I think that generic_plans and custom_plans are useful when
investigating
the cause of performance drop by cached plan mode. But I failed to
get
how much useful last_plan_type is.This may be an exceptional case, but I once had a case needed
to ensure whether generic or custom plan was chosen for specific
queries in a development environment.In your case, probably you had to ensure that the last multiple (or
every)
executions chose generic or custom plan? If yes, I'm afraid that
displaying
only the last plan mode is not enough for your case. No?
So it seems better to check generic_plans or custom_plans columns in
the
view rather than last_plan_type even in your case. Thought?Yeah, I now feel last_plan is not so necessary and only the numbers
of
generic/custom plan is enough.If there are no objections, I'm going to remove this column and
related codes.As mentioned, I removed last_plan column.
Thanks for updating the patch! It basically looks good to me.
I have one comment; you added the regression tests for generic and
custom plans into prepare.sql. But the similar tests already exist in
plancache.sql. So isn't it better to add the tests for generic_plans
and
custom_plans columns, into plancache.sql?
Thanks for your comments!
Agreed.
I removed tests on prepare.sql and added them to plancache.sql.
Thoughts?
Regards,
--
Atsushi Torikoshi
NTT DATA CORPORATION
Attachments:
0008-Expose-counters-of-plancache-to-pg_prepared_statement.patchtext/x-diff; name=0008-Expose-counters-of-plancache-to-pg_prepared_statement.patchDownload
From e3517479cea76e88f3ab8626d14452b36288dcb5 Mon Sep 17 00:00:00 2001
From: Atsushi Torikoshi <torikoshia@oss.nttdata.com>
Date: Tue, 14 Jul 2020 10:27:32 +0900
Subject: [PATCH] We didn't have an easy way to find how many times generic and
ustom plans of a prepared statement have been executed. This patch exposes
the numbers of both plans in pg_prepared_statements.
---
doc/src/sgml/catalogs.sgml | 18 ++++++++++++
src/backend/commands/prepare.c | 15 +++++++---
src/backend/utils/cache/plancache.c | 17 ++++++++----
src/include/catalog/pg_proc.dat | 6 ++--
src/include/utils/plancache.h | 3 +-
src/test/regress/expected/plancache.out | 37 ++++++++++++++++++++++++-
src/test/regress/expected/rules.out | 6 ++--
src/test/regress/sql/plancache.sql | 12 +++++++-
8 files changed, 96 insertions(+), 18 deletions(-)
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index e9cdff4864..8bc6d685f4 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -10830,6 +10830,24 @@ SELECT * FROM pg_locks pl LEFT JOIN pg_prepared_xacts ppx
frontend/backend protocol
</para></entry>
</row>
+
+ <row>
+ <entry role="catalog_table_entry"><para role="column_definition">
+ <structfield>generic_plans</structfield> <type>int8</type>
+ </para>
+ <para>
+ Number of times generic plan was chosen
+ </para></entry>
+ </row>
+
+ <row>
+ <entry role="catalog_table_entry"><para role="column_definition">
+ <structfield>custom_plans</structfield> <type>int8</type>
+ </para>
+ <para>
+ Number of times custom plan was chosen
+ </para></entry>
+ </row>
</tbody>
</tgroup>
</table>
diff --git a/src/backend/commands/prepare.c b/src/backend/commands/prepare.c
index 80d6df8ac1..4b18be5b27 100644
--- a/src/backend/commands/prepare.c
+++ b/src/backend/commands/prepare.c
@@ -694,7 +694,8 @@ ExplainExecuteQuery(ExecuteStmt *execstmt, IntoClause *into, ExplainState *es,
/*
* This set returning function reads all the prepared statements and
- * returns a set of (name, statement, prepare_time, param_types, from_sql).
+ * returns a set of (name, statement, prepare_time, param_types, from_sql,
+ * generic_plans, custom_plans).
*/
Datum
pg_prepared_statement(PG_FUNCTION_ARGS)
@@ -723,7 +724,7 @@ pg_prepared_statement(PG_FUNCTION_ARGS)
* build tupdesc for result tuples. This must match the definition of the
* pg_prepared_statements view in system_views.sql
*/
- tupdesc = CreateTemplateTupleDesc(5);
+ tupdesc = CreateTemplateTupleDesc(7);
TupleDescInitEntry(tupdesc, (AttrNumber) 1, "name",
TEXTOID, -1, 0);
TupleDescInitEntry(tupdesc, (AttrNumber) 2, "statement",
@@ -734,6 +735,10 @@ pg_prepared_statement(PG_FUNCTION_ARGS)
REGTYPEARRAYOID, -1, 0);
TupleDescInitEntry(tupdesc, (AttrNumber) 5, "from_sql",
BOOLOID, -1, 0);
+ TupleDescInitEntry(tupdesc, (AttrNumber) 6, "generic_plans",
+ INT8OID, -1, 0);
+ TupleDescInitEntry(tupdesc, (AttrNumber) 7, "custom_plans",
+ INT8OID, -1, 0);
/*
* We put all the tuples into a tuplestore in one scan of the hashtable.
@@ -755,8 +760,8 @@ pg_prepared_statement(PG_FUNCTION_ARGS)
hash_seq_init(&hash_seq, prepared_queries);
while ((prep_stmt = hash_seq_search(&hash_seq)) != NULL)
{
- Datum values[5];
- bool nulls[5];
+ Datum values[7];
+ bool nulls[7];
MemSet(nulls, 0, sizeof(nulls));
@@ -766,6 +771,8 @@ pg_prepared_statement(PG_FUNCTION_ARGS)
values[3] = build_regtype_array(prep_stmt->plansource->param_types,
prep_stmt->plansource->num_params);
values[4] = BoolGetDatum(prep_stmt->from_sql);
+ values[5] = Int64GetDatumFast(prep_stmt->plansource->num_generic_plans);
+ values[6] = Int64GetDatumFast(prep_stmt->plansource->num_custom_plans);
tuplestore_putvalues(tupstore, tupdesc, values, nulls);
}
diff --git a/src/backend/utils/cache/plancache.c b/src/backend/utils/cache/plancache.c
index 75b475c179..50d6ad28b4 100644
--- a/src/backend/utils/cache/plancache.c
+++ b/src/backend/utils/cache/plancache.c
@@ -218,6 +218,7 @@ CreateCachedPlan(RawStmt *raw_parse_tree,
plansource->generation = 0;
plansource->generic_cost = -1;
plansource->total_custom_cost = 0;
+ plansource->num_generic_plans = 0;
plansource->num_custom_plans = 0;
MemoryContextSwitchTo(oldcxt);
@@ -285,6 +286,7 @@ CreateOneShotCachedPlan(RawStmt *raw_parse_tree,
plansource->generation = 0;
plansource->generic_cost = -1;
plansource->total_custom_cost = 0;
+ plansource->num_generic_plans = 0;
plansource->num_custom_plans = 0;
return plansource;
@@ -1213,12 +1215,14 @@ GetCachedPlan(CachedPlanSource *plansource, ParamListInfo boundParams,
{
/* Build a custom plan */
plan = BuildCachedPlan(plansource, qlist, boundParams, queryEnv);
- /* Accumulate total costs of custom plans, but 'ware overflow */
- if (plansource->num_custom_plans < INT_MAX)
- {
- plansource->total_custom_cost += cached_plan_cost(plan, true);
- plansource->num_custom_plans++;
- }
+ /* Accumulate total costs of custom plans */
+ plansource->total_custom_cost += cached_plan_cost(plan, true);
+
+ plansource->num_custom_plans++;
+ }
+ else
+ {
+ plansource->num_generic_plans++;
}
Assert(plan != NULL);
@@ -1574,6 +1578,7 @@ CopyCachedPlan(CachedPlanSource *plansource)
/* We may as well copy any acquired cost knowledge */
newsource->generic_cost = plansource->generic_cost;
newsource->total_custom_cost = plansource->total_custom_cost;
+ newsource->num_generic_plans = plansource->num_generic_plans;
newsource->num_custom_plans = plansource->num_custom_plans;
MemoryContextSwitchTo(oldcxt);
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 95604e988a..4b5af32440 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -7755,9 +7755,9 @@
{ oid => '2510', descr => 'get the prepared statements for this session',
proname => 'pg_prepared_statement', prorows => '1000', proretset => 't',
provolatile => 's', proparallel => 'r', prorettype => 'record',
- proargtypes => '', proallargtypes => '{text,text,timestamptz,_regtype,bool}',
- proargmodes => '{o,o,o,o,o}',
- proargnames => '{name,statement,prepare_time,parameter_types,from_sql}',
+ proargtypes => '', proallargtypes => '{text,text,timestamptz,_regtype,bool,int8,int8}',
+ proargmodes => '{o,o,o,o,o,o,o}',
+ proargnames => '{name,statement,prepare_time,parameter_types,from_sql,generic_plans,custom_plans}',
prosrc => 'pg_prepared_statement' },
{ oid => '2511', descr => 'get the open cursors for this session',
proname => 'pg_cursor', prorows => '1000', proretset => 't',
diff --git a/src/include/utils/plancache.h b/src/include/utils/plancache.h
index 522020d763..4901568553 100644
--- a/src/include/utils/plancache.h
+++ b/src/include/utils/plancache.h
@@ -130,7 +130,8 @@ typedef struct CachedPlanSource
/* State kept to help decide whether to use custom or generic plans: */
double generic_cost; /* cost of generic plan, or -1 if not known */
double total_custom_cost; /* total cost of custom plans so far */
- int num_custom_plans; /* number of plans included in total */
+ int64 num_custom_plans; /* # of custom plans included in total */
+ int64 num_generic_plans; /* # of generic plans */
} CachedPlanSource;
/*
diff --git a/src/test/regress/expected/plancache.out b/src/test/regress/expected/plancache.out
index 7d289b8c5e..c2513306fe 100644
--- a/src/test/regress/expected/plancache.out
+++ b/src/test/regress/expected/plancache.out
@@ -278,12 +278,19 @@ drop table pc_list_part_1;
execute pstmt_def_insert(1);
drop table pc_list_parted, pc_list_part_null;
deallocate pstmt_def_insert;
--- Test plan_cache_mode
+-- Test plan_cache_mode and the numbers of custom and generic plan
create table test_mode (a int);
insert into test_mode select 1 from generate_series(1,1000) union all select 2;
create index on test_mode (a);
analyze test_mode;
prepare test_mode_pp (int) as select count(*) from test_mode where a = $1;
+select name, generic_plans, custom_plans from pg_prepared_statements
+ where name = 'test_mode_pp';
+ name | generic_plans | custom_plans
+--------------+---------------+--------------
+ test_mode_pp | 0 | 0
+(1 row)
+
-- up to 5 executions, custom plan is used
explain (costs off) execute test_mode_pp(2);
QUERY PLAN
@@ -293,6 +300,13 @@ explain (costs off) execute test_mode_pp(2);
Index Cond: (a = 2)
(3 rows)
+select name, generic_plans, custom_plans from pg_prepared_statements
+ where name = 'test_mode_pp';
+ name | generic_plans | custom_plans
+--------------+---------------+--------------
+ test_mode_pp | 0 | 1
+(1 row)
+
-- force generic plan
set plan_cache_mode to force_generic_plan;
explain (costs off) execute test_mode_pp(2);
@@ -303,6 +317,13 @@ explain (costs off) execute test_mode_pp(2);
Filter: (a = $1)
(3 rows)
+select name, generic_plans, custom_plans from pg_prepared_statements
+ where name = 'test_mode_pp';
+ name | generic_plans | custom_plans
+--------------+---------------+--------------
+ test_mode_pp | 1 | 1
+(1 row)
+
-- get to generic plan by 5 executions
set plan_cache_mode to auto;
execute test_mode_pp(1); -- 1x
@@ -329,12 +350,26 @@ execute test_mode_pp(1); -- 4x
1000
(1 row)
+select name, generic_plans, custom_plans from pg_prepared_statements
+ where name = 'test_mode_pp';
+ name | generic_plans | custom_plans
+--------------+---------------+--------------
+ test_mode_pp | 1 | 5
+(1 row)
+
execute test_mode_pp(1); -- 5x
count
-------
1000
(1 row)
+select name, generic_plans, custom_plans from pg_prepared_statements
+ where name = 'test_mode_pp';
+ name | generic_plans | custom_plans
+--------------+---------------+--------------
+ test_mode_pp | 2 | 5
+(1 row)
+
-- we should now get a really bad plan
explain (costs off) execute test_mode_pp(2);
QUERY PLAN
diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out
index fa436f2caa..601734a6f1 100644
--- a/src/test/regress/expected/rules.out
+++ b/src/test/regress/expected/rules.out
@@ -1428,8 +1428,10 @@ pg_prepared_statements| SELECT p.name,
p.statement,
p.prepare_time,
p.parameter_types,
- p.from_sql
- FROM pg_prepared_statement() p(name, statement, prepare_time, parameter_types, from_sql);
+ p.from_sql,
+ p.generic_plans,
+ p.custom_plans
+ FROM pg_prepared_statement() p(name, statement, prepare_time, parameter_types, from_sql, generic_plans, custom_plans);
pg_prepared_xacts| SELECT p.transaction,
p.gid,
p.prepared,
diff --git a/src/test/regress/sql/plancache.sql b/src/test/regress/sql/plancache.sql
index fa218c8d21..d176cb66bd 100644
--- a/src/test/regress/sql/plancache.sql
+++ b/src/test/regress/sql/plancache.sql
@@ -178,7 +178,7 @@ execute pstmt_def_insert(1);
drop table pc_list_parted, pc_list_part_null;
deallocate pstmt_def_insert;
--- Test plan_cache_mode
+-- Test plan_cache_mode and the numbers of custom and generic plan
create table test_mode (a int);
insert into test_mode select 1 from generate_series(1,1000) union all select 2;
@@ -186,13 +186,19 @@ create index on test_mode (a);
analyze test_mode;
prepare test_mode_pp (int) as select count(*) from test_mode where a = $1;
+select name, generic_plans, custom_plans from pg_prepared_statements
+ where name = 'test_mode_pp';
-- up to 5 executions, custom plan is used
explain (costs off) execute test_mode_pp(2);
+select name, generic_plans, custom_plans from pg_prepared_statements
+ where name = 'test_mode_pp';
-- force generic plan
set plan_cache_mode to force_generic_plan;
explain (costs off) execute test_mode_pp(2);
+select name, generic_plans, custom_plans from pg_prepared_statements
+ where name = 'test_mode_pp';
-- get to generic plan by 5 executions
set plan_cache_mode to auto;
@@ -200,7 +206,11 @@ execute test_mode_pp(1); -- 1x
execute test_mode_pp(1); -- 2x
execute test_mode_pp(1); -- 3x
execute test_mode_pp(1); -- 4x
+select name, generic_plans, custom_plans from pg_prepared_statements
+ where name = 'test_mode_pp';
execute test_mode_pp(1); -- 5x
+select name, generic_plans, custom_plans from pg_prepared_statements
+ where name = 'test_mode_pp';
-- we should now get a really bad plan
explain (costs off) execute test_mode_pp(2);
--
2.18.1
On 2020/07/16 11:50, torikoshia wrote:
On 2020-07-15 11:44, Fujii Masao wrote:
On 2020/07/14 21:24, torikoshia wrote:
On 2020-07-10 10:49, torikoshia wrote:
On 2020-07-08 16:41, Fujii Masao wrote:
On 2020/07/08 10:14, torikoshia wrote:
On 2020-07-06 22:16, Fujii Masao wrote:
On 2020/06/11 14:59, torikoshia wrote:
On 2020-06-10 18:00, Kyotaro Horiguchi wrote:
+ TupleDescInitEntry(tupdesc, (AttrNumber) 8, "last_plan",
This could be a problem if we showed the last plan in this view. I
think "last_plan_type" would be better.+ if (prep_stmt->plansource->last_plan_type == PLAN_CACHE_TYPE_CUSTOM) + values[7] = CStringGetTextDatum("custom"); + else if (prep_stmt->plansource->last_plan_type == PLAN_CACHE_TYPE_GENERIC) + values[7] = CStringGetTextDatum("generic"); + else + nulls[7] = true;Using swith-case prevents future additional type (if any) from being
unhandled. I think we are recommending that as a convension.Thanks for your reviewing!
I've attached a patch that reflects your comments.
Thanks for the patch! Here are the comments.
Thanks for your review!
+ Number of times generic plan was choosen + Number of times custom plan was choosenTypo: "choosen" should be "chosen"?
Thanks, fixed them.
+ <entry role="catalog_table_entry"><para role="column_definition"> + <structfield>last_plan_type</structfield> <type>text</type> + </para> + <para> + Tells the last plan type was generic or custom. If the prepared + statement has not executed yet, this field is null + </para></entry>Could you tell me how this information is expected to be used?
I think that generic_plans and custom_plans are useful when investigating
the cause of performance drop by cached plan mode. But I failed to get
how much useful last_plan_type is.This may be an exceptional case, but I once had a case needed
to ensure whether generic or custom plan was chosen for specific
queries in a development environment.In your case, probably you had to ensure that the last multiple (or every)
executions chose generic or custom plan? If yes, I'm afraid that displaying
only the last plan mode is not enough for your case. No?
So it seems better to check generic_plans or custom_plans columns in the
view rather than last_plan_type even in your case. Thought?Yeah, I now feel last_plan is not so necessary and only the numbers of
generic/custom plan is enough.If there are no objections, I'm going to remove this column and related codes.
As mentioned, I removed last_plan column.
Thanks for updating the patch! It basically looks good to me.
I have one comment; you added the regression tests for generic and
custom plans into prepare.sql. But the similar tests already exist in
plancache.sql. So isn't it better to add the tests for generic_plans and
custom_plans columns, into plancache.sql?Thanks for your comments!
Agreed.
I removed tests on prepare.sql and added them to plancache.sql.
Thoughts?
Thanks for updating the patch!
I also applied the following minor changes to the patch.
- Number of times generic plan was chosen
+ Number of times generic plan was chosen
- Number of times custom plan was chosen
+ Number of times custom plan was chosen
I got rid of one space character before those descriptions because
they should start from the position of 7th character.
-- but we can force a custom plan
set plan_cache_mode to force_custom_plan;
explain (costs off) execute test_mode_pp(2);
+select name, generic_plans, custom_plans from pg_prepared_statements
+ where name = 'test_mode_pp';
In the regression test, I added the execution of pg_prepared_statements
after the last execution of test query, to confirm that custom plan is used
when force_custom_plan is set, by checking from pg_prepared_statements.
I changed the status of this patch to "Ready for Committer" in CF.
Barring any objection, I will commit this patch.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
Attachments:
0008-Expose-counters-of-plancache-to-pg_prepared_statement_fujii.patchtext/plain; charset=UTF-8; name=0008-Expose-counters-of-plancache-to-pg_prepared_statement_fujii.patch; x-mac-creator=0; x-mac-type=0Download
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index e9cdff4864..de69487bec 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -10830,6 +10830,24 @@ SELECT * FROM pg_locks pl LEFT JOIN pg_prepared_xacts ppx
frontend/backend protocol
</para></entry>
</row>
+
+ <row>
+ <entry role="catalog_table_entry"><para role="column_definition">
+ <structfield>generic_plans</structfield> <type>int8</type>
+ </para>
+ <para>
+ Number of times generic plan was chosen
+ </para></entry>
+ </row>
+
+ <row>
+ <entry role="catalog_table_entry"><para role="column_definition">
+ <structfield>custom_plans</structfield> <type>int8</type>
+ </para>
+ <para>
+ Number of times custom plan was chosen
+ </para></entry>
+ </row>
</tbody>
</tgroup>
</table>
diff --git a/src/backend/commands/prepare.c b/src/backend/commands/prepare.c
index 80d6df8ac1..4b18be5b27 100644
--- a/src/backend/commands/prepare.c
+++ b/src/backend/commands/prepare.c
@@ -694,7 +694,8 @@ ExplainExecuteQuery(ExecuteStmt *execstmt, IntoClause *into, ExplainState *es,
/*
* This set returning function reads all the prepared statements and
- * returns a set of (name, statement, prepare_time, param_types, from_sql).
+ * returns a set of (name, statement, prepare_time, param_types, from_sql,
+ * generic_plans, custom_plans).
*/
Datum
pg_prepared_statement(PG_FUNCTION_ARGS)
@@ -723,7 +724,7 @@ pg_prepared_statement(PG_FUNCTION_ARGS)
* build tupdesc for result tuples. This must match the definition of the
* pg_prepared_statements view in system_views.sql
*/
- tupdesc = CreateTemplateTupleDesc(5);
+ tupdesc = CreateTemplateTupleDesc(7);
TupleDescInitEntry(tupdesc, (AttrNumber) 1, "name",
TEXTOID, -1, 0);
TupleDescInitEntry(tupdesc, (AttrNumber) 2, "statement",
@@ -734,6 +735,10 @@ pg_prepared_statement(PG_FUNCTION_ARGS)
REGTYPEARRAYOID, -1, 0);
TupleDescInitEntry(tupdesc, (AttrNumber) 5, "from_sql",
BOOLOID, -1, 0);
+ TupleDescInitEntry(tupdesc, (AttrNumber) 6, "generic_plans",
+ INT8OID, -1, 0);
+ TupleDescInitEntry(tupdesc, (AttrNumber) 7, "custom_plans",
+ INT8OID, -1, 0);
/*
* We put all the tuples into a tuplestore in one scan of the hashtable.
@@ -755,8 +760,8 @@ pg_prepared_statement(PG_FUNCTION_ARGS)
hash_seq_init(&hash_seq, prepared_queries);
while ((prep_stmt = hash_seq_search(&hash_seq)) != NULL)
{
- Datum values[5];
- bool nulls[5];
+ Datum values[7];
+ bool nulls[7];
MemSet(nulls, 0, sizeof(nulls));
@@ -766,6 +771,8 @@ pg_prepared_statement(PG_FUNCTION_ARGS)
values[3] = build_regtype_array(prep_stmt->plansource->param_types,
prep_stmt->plansource->num_params);
values[4] = BoolGetDatum(prep_stmt->from_sql);
+ values[5] = Int64GetDatumFast(prep_stmt->plansource->num_generic_plans);
+ values[6] = Int64GetDatumFast(prep_stmt->plansource->num_custom_plans);
tuplestore_putvalues(tupstore, tupdesc, values, nulls);
}
diff --git a/src/backend/utils/cache/plancache.c b/src/backend/utils/cache/plancache.c
index 75b475c179..50d6ad28b4 100644
--- a/src/backend/utils/cache/plancache.c
+++ b/src/backend/utils/cache/plancache.c
@@ -218,6 +218,7 @@ CreateCachedPlan(RawStmt *raw_parse_tree,
plansource->generation = 0;
plansource->generic_cost = -1;
plansource->total_custom_cost = 0;
+ plansource->num_generic_plans = 0;
plansource->num_custom_plans = 0;
MemoryContextSwitchTo(oldcxt);
@@ -285,6 +286,7 @@ CreateOneShotCachedPlan(RawStmt *raw_parse_tree,
plansource->generation = 0;
plansource->generic_cost = -1;
plansource->total_custom_cost = 0;
+ plansource->num_generic_plans = 0;
plansource->num_custom_plans = 0;
return plansource;
@@ -1213,12 +1215,14 @@ GetCachedPlan(CachedPlanSource *plansource, ParamListInfo boundParams,
{
/* Build a custom plan */
plan = BuildCachedPlan(plansource, qlist, boundParams, queryEnv);
- /* Accumulate total costs of custom plans, but 'ware overflow */
- if (plansource->num_custom_plans < INT_MAX)
- {
- plansource->total_custom_cost += cached_plan_cost(plan, true);
- plansource->num_custom_plans++;
- }
+ /* Accumulate total costs of custom plans */
+ plansource->total_custom_cost += cached_plan_cost(plan, true);
+
+ plansource->num_custom_plans++;
+ }
+ else
+ {
+ plansource->num_generic_plans++;
}
Assert(plan != NULL);
@@ -1574,6 +1578,7 @@ CopyCachedPlan(CachedPlanSource *plansource)
/* We may as well copy any acquired cost knowledge */
newsource->generic_cost = plansource->generic_cost;
newsource->total_custom_cost = plansource->total_custom_cost;
+ newsource->num_generic_plans = plansource->num_generic_plans;
newsource->num_custom_plans = plansource->num_custom_plans;
MemoryContextSwitchTo(oldcxt);
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 95604e988a..4b5af32440 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -7755,9 +7755,9 @@
{ oid => '2510', descr => 'get the prepared statements for this session',
proname => 'pg_prepared_statement', prorows => '1000', proretset => 't',
provolatile => 's', proparallel => 'r', prorettype => 'record',
- proargtypes => '', proallargtypes => '{text,text,timestamptz,_regtype,bool}',
- proargmodes => '{o,o,o,o,o}',
- proargnames => '{name,statement,prepare_time,parameter_types,from_sql}',
+ proargtypes => '', proallargtypes => '{text,text,timestamptz,_regtype,bool,int8,int8}',
+ proargmodes => '{o,o,o,o,o,o,o}',
+ proargnames => '{name,statement,prepare_time,parameter_types,from_sql,generic_plans,custom_plans}',
prosrc => 'pg_prepared_statement' },
{ oid => '2511', descr => 'get the open cursors for this session',
proname => 'pg_cursor', prorows => '1000', proretset => 't',
diff --git a/src/include/utils/plancache.h b/src/include/utils/plancache.h
index 522020d763..4901568553 100644
--- a/src/include/utils/plancache.h
+++ b/src/include/utils/plancache.h
@@ -130,7 +130,8 @@ typedef struct CachedPlanSource
/* State kept to help decide whether to use custom or generic plans: */
double generic_cost; /* cost of generic plan, or -1 if not known */
double total_custom_cost; /* total cost of custom plans so far */
- int num_custom_plans; /* number of plans included in total */
+ int64 num_custom_plans; /* # of custom plans included in total */
+ int64 num_generic_plans; /* # of generic plans */
} CachedPlanSource;
/*
diff --git a/src/test/regress/expected/plancache.out b/src/test/regress/expected/plancache.out
index 7d289b8c5e..4e59188196 100644
--- a/src/test/regress/expected/plancache.out
+++ b/src/test/regress/expected/plancache.out
@@ -284,7 +284,15 @@ insert into test_mode select 1 from generate_series(1,1000) union all select 2;
create index on test_mode (a);
analyze test_mode;
prepare test_mode_pp (int) as select count(*) from test_mode where a = $1;
+select name, generic_plans, custom_plans from pg_prepared_statements
+ where name = 'test_mode_pp';
+ name | generic_plans | custom_plans
+--------------+---------------+--------------
+ test_mode_pp | 0 | 0
+(1 row)
+
-- up to 5 executions, custom plan is used
+set plan_cache_mode to auto;
explain (costs off) execute test_mode_pp(2);
QUERY PLAN
----------------------------------------------------------
@@ -293,6 +301,13 @@ explain (costs off) execute test_mode_pp(2);
Index Cond: (a = 2)
(3 rows)
+select name, generic_plans, custom_plans from pg_prepared_statements
+ where name = 'test_mode_pp';
+ name | generic_plans | custom_plans
+--------------+---------------+--------------
+ test_mode_pp | 0 | 1
+(1 row)
+
-- force generic plan
set plan_cache_mode to force_generic_plan;
explain (costs off) execute test_mode_pp(2);
@@ -303,6 +318,13 @@ explain (costs off) execute test_mode_pp(2);
Filter: (a = $1)
(3 rows)
+select name, generic_plans, custom_plans from pg_prepared_statements
+ where name = 'test_mode_pp';
+ name | generic_plans | custom_plans
+--------------+---------------+--------------
+ test_mode_pp | 1 | 1
+(1 row)
+
-- get to generic plan by 5 executions
set plan_cache_mode to auto;
execute test_mode_pp(1); -- 1x
@@ -329,12 +351,26 @@ execute test_mode_pp(1); -- 4x
1000
(1 row)
+select name, generic_plans, custom_plans from pg_prepared_statements
+ where name = 'test_mode_pp';
+ name | generic_plans | custom_plans
+--------------+---------------+--------------
+ test_mode_pp | 1 | 5
+(1 row)
+
execute test_mode_pp(1); -- 5x
count
-------
1000
(1 row)
+select name, generic_plans, custom_plans from pg_prepared_statements
+ where name = 'test_mode_pp';
+ name | generic_plans | custom_plans
+--------------+---------------+--------------
+ test_mode_pp | 2 | 5
+(1 row)
+
-- we should now get a really bad plan
explain (costs off) execute test_mode_pp(2);
QUERY PLAN
@@ -354,4 +390,11 @@ explain (costs off) execute test_mode_pp(2);
Index Cond: (a = 2)
(3 rows)
+select name, generic_plans, custom_plans from pg_prepared_statements
+ where name = 'test_mode_pp';
+ name | generic_plans | custom_plans
+--------------+---------------+--------------
+ test_mode_pp | 3 | 6
+(1 row)
+
drop table test_mode;
diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out
index fa436f2caa..601734a6f1 100644
--- a/src/test/regress/expected/rules.out
+++ b/src/test/regress/expected/rules.out
@@ -1428,8 +1428,10 @@ pg_prepared_statements| SELECT p.name,
p.statement,
p.prepare_time,
p.parameter_types,
- p.from_sql
- FROM pg_prepared_statement() p(name, statement, prepare_time, parameter_types, from_sql);
+ p.from_sql,
+ p.generic_plans,
+ p.custom_plans
+ FROM pg_prepared_statement() p(name, statement, prepare_time, parameter_types, from_sql, generic_plans, custom_plans);
pg_prepared_xacts| SELECT p.transaction,
p.gid,
p.prepared,
diff --git a/src/test/regress/sql/plancache.sql b/src/test/regress/sql/plancache.sql
index fa218c8d21..4b2f11dcc6 100644
--- a/src/test/regress/sql/plancache.sql
+++ b/src/test/regress/sql/plancache.sql
@@ -186,13 +186,20 @@ create index on test_mode (a);
analyze test_mode;
prepare test_mode_pp (int) as select count(*) from test_mode where a = $1;
+select name, generic_plans, custom_plans from pg_prepared_statements
+ where name = 'test_mode_pp';
-- up to 5 executions, custom plan is used
+set plan_cache_mode to auto;
explain (costs off) execute test_mode_pp(2);
+select name, generic_plans, custom_plans from pg_prepared_statements
+ where name = 'test_mode_pp';
-- force generic plan
set plan_cache_mode to force_generic_plan;
explain (costs off) execute test_mode_pp(2);
+select name, generic_plans, custom_plans from pg_prepared_statements
+ where name = 'test_mode_pp';
-- get to generic plan by 5 executions
set plan_cache_mode to auto;
@@ -200,7 +207,11 @@ execute test_mode_pp(1); -- 1x
execute test_mode_pp(1); -- 2x
execute test_mode_pp(1); -- 3x
execute test_mode_pp(1); -- 4x
+select name, generic_plans, custom_plans from pg_prepared_statements
+ where name = 'test_mode_pp';
execute test_mode_pp(1); -- 5x
+select name, generic_plans, custom_plans from pg_prepared_statements
+ where name = 'test_mode_pp';
-- we should now get a really bad plan
explain (costs off) execute test_mode_pp(2);
@@ -208,5 +219,7 @@ explain (costs off) execute test_mode_pp(2);
-- but we can force a custom plan
set plan_cache_mode to force_custom_plan;
explain (costs off) execute test_mode_pp(2);
+select name, generic_plans, custom_plans from pg_prepared_statements
+ where name = 'test_mode_pp';
drop table test_mode;
On 2020/07/17 16:25, Fujii Masao wrote:
On 2020/07/16 11:50, torikoshia wrote:
On 2020-07-15 11:44, Fujii Masao wrote:
On 2020/07/14 21:24, torikoshia wrote:
On 2020-07-10 10:49, torikoshia wrote:
On 2020-07-08 16:41, Fujii Masao wrote:
On 2020/07/08 10:14, torikoshia wrote:
On 2020-07-06 22:16, Fujii Masao wrote:
On 2020/06/11 14:59, torikoshia wrote:
On 2020-06-10 18:00, Kyotaro Horiguchi wrote:
+ TupleDescInitEntry(tupdesc, (AttrNumber) 8, "last_plan",
This could be a problem if we showed the last plan in this view. I
think "last_plan_type" would be better.+ if (prep_stmt->plansource->last_plan_type == PLAN_CACHE_TYPE_CUSTOM) + values[7] = CStringGetTextDatum("custom"); + else if (prep_stmt->plansource->last_plan_type == PLAN_CACHE_TYPE_GENERIC) + values[7] = CStringGetTextDatum("generic"); + else + nulls[7] = true;Using swith-case prevents future additional type (if any) from being
unhandled. I think we are recommending that as a convension.Thanks for your reviewing!
I've attached a patch that reflects your comments.
Thanks for the patch! Here are the comments.
Thanks for your review!
+ Number of times generic plan was choosen + Number of times custom plan was choosenTypo: "choosen" should be "chosen"?
Thanks, fixed them.
+ <entry role="catalog_table_entry"><para role="column_definition"> + <structfield>last_plan_type</structfield> <type>text</type> + </para> + <para> + Tells the last plan type was generic or custom. If the prepared + statement has not executed yet, this field is null + </para></entry>Could you tell me how this information is expected to be used?
I think that generic_plans and custom_plans are useful when investigating
the cause of performance drop by cached plan mode. But I failed to get
how much useful last_plan_type is.This may be an exceptional case, but I once had a case needed
to ensure whether generic or custom plan was chosen for specific
queries in a development environment.In your case, probably you had to ensure that the last multiple (or every)
executions chose generic or custom plan? If yes, I'm afraid that displaying
only the last plan mode is not enough for your case. No?
So it seems better to check generic_plans or custom_plans columns in the
view rather than last_plan_type even in your case. Thought?Yeah, I now feel last_plan is not so necessary and only the numbers of
generic/custom plan is enough.If there are no objections, I'm going to remove this column and related codes.
As mentioned, I removed last_plan column.
Thanks for updating the patch! It basically looks good to me.
I have one comment; you added the regression tests for generic and
custom plans into prepare.sql. But the similar tests already exist in
plancache.sql. So isn't it better to add the tests for generic_plans and
custom_plans columns, into plancache.sql?Thanks for your comments!
Agreed.
I removed tests on prepare.sql and added them to plancache.sql.
Thoughts?Thanks for updating the patch!
I also applied the following minor changes to the patch.- Number of times generic plan was chosen + Number of times generic plan was chosen - Number of times custom plan was chosen + Number of times custom plan was chosenI got rid of one space character before those descriptions because
they should start from the position of 7th character.-- but we can force a custom plan set plan_cache_mode to force_custom_plan; explain (costs off) execute test_mode_pp(2); +select name, generic_plans, custom_plans from pg_prepared_statements + where name = 'test_mode_pp';In the regression test, I added the execution of pg_prepared_statements
after the last execution of test query, to confirm that custom plan is used
when force_custom_plan is set, by checking from pg_prepared_statements.I changed the status of this patch to "Ready for Committer" in CF.
Barring any objection, I will commit this patch.
Committed. Thanks!
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
On 2020-07-20 11:57, Fujii Masao wrote:
On 2020/07/17 16:25, Fujii Masao wrote:
On 2020/07/16 11:50, torikoshia wrote:
On 2020-07-15 11:44, Fujii Masao wrote:
On 2020/07/14 21:24, torikoshia wrote:
On 2020-07-10 10:49, torikoshia wrote:
On 2020-07-08 16:41, Fujii Masao wrote:
On 2020/07/08 10:14, torikoshia wrote:
On 2020-07-06 22:16, Fujii Masao wrote:
On 2020/06/11 14:59, torikoshia wrote:
On 2020-06-10 18:00, Kyotaro Horiguchi wrote:
+ TupleDescInitEntry(tupdesc, (AttrNumber) 8, "last_plan",
This could be a problem if we showed the last plan in this
view. I
think "last_plan_type" would be better.+ if (prep_stmt->plansource->last_plan_type == PLAN_CACHE_TYPE_CUSTOM) + values[7] = CStringGetTextDatum("custom"); + else if (prep_stmt->plansource->last_plan_type == PLAN_CACHE_TYPE_GENERIC) + values[7] = CStringGetTextDatum("generic"); + else + nulls[7] = true;Using swith-case prevents future additional type (if any)
from being
unhandled. I think we are recommending that as a convension.Thanks for your reviewing!
I've attached a patch that reflects your comments.
Thanks for the patch! Here are the comments.
Thanks for your review!
+ Number of times generic plan was choosen + Number of times custom plan was choosenTypo: "choosen" should be "chosen"?
Thanks, fixed them.
+ <entry role="catalog_table_entry"><para role="column_definition"> + <structfield>last_plan_type</structfield> <type>text</type> + </para> + <para> + Tells the last plan type was generic or custom. If the prepared + statement has not executed yet, this field is null + </para></entry>Could you tell me how this information is expected to be used?
I think that generic_plans and custom_plans are useful when
investigating
the cause of performance drop by cached plan mode. But I failed
to get
how much useful last_plan_type is.This may be an exceptional case, but I once had a case needed
to ensure whether generic or custom plan was chosen for specific
queries in a development environment.In your case, probably you had to ensure that the last multiple
(or every)
executions chose generic or custom plan? If yes, I'm afraid that
displaying
only the last plan mode is not enough for your case. No?
So it seems better to check generic_plans or custom_plans columns
in the
view rather than last_plan_type even in your case. Thought?Yeah, I now feel last_plan is not so necessary and only the
numbers of
generic/custom plan is enough.If there are no objections, I'm going to remove this column and
related codes.As mentioned, I removed last_plan column.
Thanks for updating the patch! It basically looks good to me.
I have one comment; you added the regression tests for generic and
custom plans into prepare.sql. But the similar tests already exist
in
plancache.sql. So isn't it better to add the tests for generic_plans
and
custom_plans columns, into plancache.sql?Thanks for your comments!
Agreed.
I removed tests on prepare.sql and added them to plancache.sql.
Thoughts?Thanks for updating the patch!
I also applied the following minor changes to the patch.- Number of times generic plan was chosen + Number of times generic plan was chosen - Number of times custom plan was chosen + Number of times custom plan was chosenI got rid of one space character before those descriptions because
they should start from the position of 7th character.-- but we can force a custom plan set plan_cache_mode to force_custom_plan; explain (costs off) execute test_mode_pp(2); +select name, generic_plans, custom_plans from pg_prepared_statements + where name = 'test_mode_pp';In the regression test, I added the execution of
pg_prepared_statements
after the last execution of test query, to confirm that custom plan is
used
when force_custom_plan is set, by checking from
pg_prepared_statements.I changed the status of this patch to "Ready for Committer" in CF.
Barring any objection, I will commit this patch.
Committed. Thanks!
Thanks!
As I proposed earlier in this thread, I'm now trying to add information
about generic/cudstom plan to pg_stat_statements.
I'll share the idea and the poc patch soon.
Regards,
--
Atsushi Torikoshi
NTT DATA CORPORATION
On 2020-07-20 13:57, torikoshia wrote:
As I proposed earlier in this thread, I'm now trying to add information
about generic/cudstom plan to pg_stat_statements.
I'll share the idea and the poc patch soon.
Attached a poc patch.
Main purpose is to decide (1) the user interface and (2) the
way to get the plan type from pg_stat_statements.
(1) the user interface
I added a new boolean column 'generic_plan' to both
pg_stat_statements view and the member of the hash key of
pg_stat_statements.
This is because as Legrand pointed out the feature seems
useful under the condition of differentiating all the
counters for a queryid using a generic plan and the one
using a custom one.
I thought it might be preferable to make a GUC to enable
or disable this feature, but changing the hash key makes
it harder.
(2) way to get the plan type from pg_stat_statements
To know whether the plan is generic or not, I added a
member to CachedPlan and get it in the ExecutorStart_hook
from ActivePortal.
I wished to do it in the ExecutorEnd_hook, but the
ActivePortal is not available on executorEnd, so I keep
it on a global variable newly defined in pg_stat_statements.
Any thoughts?
This is a poc patch and I'm going to do below things later:
- update pg_stat_statements version
- change default value for the newly added parameter in
pg_stat_statements_reset() from -1 to 0(since default for
other parameters are all 0)
- add regression tests and update docs
Regards,
--
Atsushi Torikoshi
NTT DATA CORPORATION
Attachments:
0001-POC-add-plan-type-to-pgss.patchtext/x-diff; name=0001-POC-add-plan-type-to-pgss.patchDownload
From 793eafad8e988b6754c9d89e0ea14b64b07eef81 Mon Sep 17 00:00:00 2001
From: Atsushi Torikoshi<torikoshia@oss.nttdata.com>
Date: Wed, 22 Jul 2020 16:00:04 +0900
Subject: [PATCH] [poc] Previously the number of custom and generic plans are
recoreded only in pg_prepared_statements, meaning we could only track them
regarding current session. This patch records them in pg_stat_statements and
it enables to track them regarding all sessions of the PostgreSQL instance.
---
.../pg_stat_statements--1.6--1.7.sql | 3 +-
.../pg_stat_statements--1.7--1.8.sql | 1 +
.../pg_stat_statements/pg_stat_statements.c | 44 +++++++++++++++----
src/backend/utils/cache/plancache.c | 2 +
src/include/utils/plancache.h | 1 +
5 files changed, 41 insertions(+), 10 deletions(-)
diff --git a/contrib/pg_stat_statements/pg_stat_statements--1.6--1.7.sql b/contrib/pg_stat_statements/pg_stat_statements--1.6--1.7.sql
index 6fc3fed4c9..5ab0a26b77 100644
--- a/contrib/pg_stat_statements/pg_stat_statements--1.6--1.7.sql
+++ b/contrib/pg_stat_statements/pg_stat_statements--1.6--1.7.sql
@@ -12,7 +12,8 @@ DROP FUNCTION pg_stat_statements_reset();
/* Now redefine */
CREATE FUNCTION pg_stat_statements_reset(IN userid Oid DEFAULT 0,
IN dbid Oid DEFAULT 0,
- IN queryid bigint DEFAULT 0
+ IN queryid bigint DEFAULT 0,
+ IN generic_plan int DEFAULT -1
)
RETURNS void
AS 'MODULE_PATHNAME', 'pg_stat_statements_reset_1_7'
diff --git a/contrib/pg_stat_statements/pg_stat_statements--1.7--1.8.sql b/contrib/pg_stat_statements/pg_stat_statements--1.7--1.8.sql
index 0f63f08f7e..0d7c4e7343 100644
--- a/contrib/pg_stat_statements/pg_stat_statements--1.7--1.8.sql
+++ b/contrib/pg_stat_statements/pg_stat_statements--1.7--1.8.sql
@@ -15,6 +15,7 @@ DROP FUNCTION pg_stat_statements(boolean);
CREATE FUNCTION pg_stat_statements(IN showtext boolean,
OUT userid oid,
OUT dbid oid,
+ OUT generic_plan bool,
OUT queryid bigint,
OUT query text,
OUT plans int8,
diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index 14cad19afb..5d74dc04cd 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -78,9 +78,11 @@
#include "storage/ipc.h"
#include "storage/spin.h"
#include "tcop/utility.h"
+#include "tcop/pquery.h"
#include "utils/acl.h"
#include "utils/builtins.h"
#include "utils/memutils.h"
+#include "utils/plancache.h"
PG_MODULE_MAGIC;
@@ -156,6 +158,7 @@ typedef struct pgssHashKey
Oid userid; /* user OID */
Oid dbid; /* database OID */
uint64 queryid; /* query identifier */
+ bool is_generic_plan;
} pgssHashKey;
/*
@@ -266,6 +269,9 @@ static int exec_nested_level = 0;
/* Current nesting depth of planner calls */
static int plan_nested_level = 0;
+/* Current plan type */
+static bool is_generic_plan = false;
+
/* Saved hook values in case of unload */
static shmem_startup_hook_type prev_shmem_startup_hook = NULL;
static post_parse_analyze_hook_type prev_post_parse_analyze_hook = NULL;
@@ -367,7 +373,7 @@ static char *qtext_fetch(Size query_offset, int query_len,
char *buffer, Size buffer_size);
static bool need_gc_qtexts(void);
static void gc_qtexts(void);
-static void entry_reset(Oid userid, Oid dbid, uint64 queryid);
+static void entry_reset(Oid userid, Oid dbid, uint64 queryid, int generic_plan);
static void AppendJumble(pgssJumbleState *jstate,
const unsigned char *item, Size size);
static void JumbleQuery(pgssJumbleState *jstate, Query *query);
@@ -1013,6 +1019,16 @@ pgss_ExecutorStart(QueryDesc *queryDesc, int eflags)
*/
if (pgss_enabled(exec_nested_level) && queryDesc->plannedstmt->queryId != UINT64CONST(0))
{
+ /*
+ * ActivePortal is not available at ExecutorEnd. We preserve
+ * the plan type from ActivePortal here.
+ */
+ Assert(ActivePortal);
+ if(ActivePortal->cplan != NULL && ActivePortal->cplan->is_generic)
+ is_generic_plan = true;
+ else
+ is_generic_plan = false;
+
/*
* Set up to track total elapsed time in ExecutorRun. Make sure the
* space is allocated in the per-query context so it will go away at
@@ -1100,6 +1116,8 @@ pgss_ExecutorEnd(QueryDesc *queryDesc)
&queryDesc->totaltime->walusage,
NULL);
}
+ /* reset the plan type. */
+ is_generic_plan = false;
if (prev_ExecutorEnd)
prev_ExecutorEnd(queryDesc);
@@ -1299,9 +1317,11 @@ pgss_store(const char *query, uint64 queryId,
}
/* Set up key for hashtable search */
+ memset(&key, 0, sizeof(key));
key.userid = GetUserId();
key.dbid = MyDatabaseId;
key.queryid = queryId;
+ key.is_generic_plan = is_generic_plan;
/* Lookup the hash table entry with shared lock. */
LWLockAcquire(pgss->lock, LW_SHARED);
@@ -1455,12 +1475,14 @@ pg_stat_statements_reset_1_7(PG_FUNCTION_ARGS)
Oid userid;
Oid dbid;
uint64 queryid;
+ int generic_plan;
userid = PG_GETARG_OID(0);
dbid = PG_GETARG_OID(1);
queryid = (uint64) PG_GETARG_INT64(2);
+ generic_plan = (uint32) PG_GETARG_INT32(3);
- entry_reset(userid, dbid, queryid);
+ entry_reset(userid, dbid, queryid, generic_plan);
PG_RETURN_VOID();
}
@@ -1471,7 +1493,7 @@ pg_stat_statements_reset_1_7(PG_FUNCTION_ARGS)
Datum
pg_stat_statements_reset(PG_FUNCTION_ARGS)
{
- entry_reset(0, 0, 0);
+ entry_reset(0, 0, 0, -1);
PG_RETURN_VOID();
}
@@ -1481,8 +1503,8 @@ pg_stat_statements_reset(PG_FUNCTION_ARGS)
#define PG_STAT_STATEMENTS_COLS_V1_1 18
#define PG_STAT_STATEMENTS_COLS_V1_2 19
#define PG_STAT_STATEMENTS_COLS_V1_3 23
-#define PG_STAT_STATEMENTS_COLS_V1_8 32
-#define PG_STAT_STATEMENTS_COLS 32 /* maximum of above */
+#define PG_STAT_STATEMENTS_COLS_V1_8 33
+#define PG_STAT_STATEMENTS_COLS 33 /* maximum of above */
/*
* Retrieve statement statistics.
@@ -1704,6 +1726,7 @@ pg_stat_statements_internal(FunctionCallInfo fcinfo,
values[i++] = ObjectIdGetDatum(entry->key.userid);
values[i++] = ObjectIdGetDatum(entry->key.dbid);
+ values[i++] = BoolGetDatum(entry->key.is_generic_plan);
if (is_allowed_role || entry->key.userid == userid)
{
@@ -2443,7 +2466,7 @@ gc_fail:
* Release entries corresponding to parameters passed.
*/
static void
-entry_reset(Oid userid, Oid dbid, uint64 queryid)
+entry_reset(Oid userid, Oid dbid, uint64 queryid, int generic_plan)
{
HASH_SEQ_STATUS hash_seq;
pgssEntry *entry;
@@ -2460,19 +2483,21 @@ entry_reset(Oid userid, Oid dbid, uint64 queryid)
LWLockAcquire(pgss->lock, LW_EXCLUSIVE);
num_entries = hash_get_num_entries(pgss_hash);
- if (userid != 0 && dbid != 0 && queryid != UINT64CONST(0))
+ if (userid != 0 && dbid != 0 && queryid != UINT64CONST(0) && generic_plan != -1)
{
/* If all the parameters are available, use the fast path. */
+ memset(&key, 0, sizeof(key));
key.userid = userid;
key.dbid = dbid;
key.queryid = queryid;
+ key.is_generic_plan = generic_plan;
/* Remove the key if exists */
entry = (pgssEntry *) hash_search(pgss_hash, &key, HASH_REMOVE, NULL);
if (entry) /* found */
num_remove++;
}
- else if (userid != 0 || dbid != 0 || queryid != UINT64CONST(0))
+ else if (userid != 0 || dbid != 0 || queryid != UINT64CONST(0) || generic_plan != -1)
{
/* Remove entries corresponding to valid parameters. */
hash_seq_init(&hash_seq, pgss_hash);
@@ -2480,7 +2505,8 @@ entry_reset(Oid userid, Oid dbid, uint64 queryid)
{
if ((!userid || entry->key.userid == userid) &&
(!dbid || entry->key.dbid == dbid) &&
- (!queryid || entry->key.queryid == queryid))
+ (!queryid || entry->key.queryid == queryid) &&
+ (generic_plan == -1 || entry->key.is_generic_plan == generic_plan))
{
hash_search(pgss_hash, &entry->key, HASH_REMOVE, NULL);
num_remove++;
diff --git a/src/backend/utils/cache/plancache.c b/src/backend/utils/cache/plancache.c
index 50d6ad28b4..62e0539370 100644
--- a/src/backend/utils/cache/plancache.c
+++ b/src/backend/utils/cache/plancache.c
@@ -1219,10 +1219,12 @@ GetCachedPlan(CachedPlanSource *plansource, ParamListInfo boundParams,
plansource->total_custom_cost += cached_plan_cost(plan, true);
plansource->num_custom_plans++;
+ plan->is_generic = false;
}
else
{
plansource->num_generic_plans++;
+ plan->is_generic = true;
}
Assert(plan != NULL);
diff --git a/src/include/utils/plancache.h b/src/include/utils/plancache.h
index 4901568553..e4f076fdb3 100644
--- a/src/include/utils/plancache.h
+++ b/src/include/utils/plancache.h
@@ -158,6 +158,7 @@ typedef struct CachedPlan
int generation; /* parent's generation number for this plan */
int refcount; /* count of live references to this struct */
MemoryContext context; /* context containing this CachedPlan */
+ bool is_generic; /* is this plan generic or not */
} CachedPlan;
/*
--
2.18.1
On 2020/07/22 16:49, torikoshia wrote:
On 2020-07-20 13:57, torikoshia wrote:
As I proposed earlier in this thread, I'm now trying to add information
about generic/cudstom plan to pg_stat_statements.
I'll share the idea and the poc patch soon.Attached a poc patch.
Thanks for the POC patch!
With the patch, when I ran "CREATE EXTENSION pg_stat_statements",
I got the following error.
ERROR: function pg_stat_statements_reset(oid, oid, bigint) does not exist
Main purpose is to decide (1) the user interface and (2) the
way to get the plan type from pg_stat_statements.(1) the user interface
I added a new boolean column 'generic_plan' to both
pg_stat_statements view and the member of the hash key of
pg_stat_statements.This is because as Legrand pointed out the feature seems
useful under the condition of differentiating all the
counters for a queryid using a generic plan and the one
using a custom one.
I don't like this because this may double the number of entries in pgss.
Which means that the number of entries can more easily reach
pg_stat_statements.max and some entries will be discarded.
I thought it might be preferable to make a GUC to enable
or disable this feature, but changing the hash key makes
it harder.
What happens if the server was running with this option enabled and then
restarted with the option disabled? Firstly two entries for the same query
were stored in pgss because the option was enabled. But when it's disabled
and the server is restarted, those two entries should be merged into one
at the startup of server? If so, that's problematic because it may take
a long time.
Therefore I think that it's better and simple to just expose the number of
times generic/custom plan was chosen for each query.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
Main purpose is to decide (1) the user interface and (2) the
way to get the plan type from pg_stat_statements.(1) the user interface
I added a new boolean column 'generic_plan' to both
pg_stat_statements view and the member of the hash key of
pg_stat_statements.This is because as Legrand pointed out the feature seems
useful under the condition of differentiating all the
counters for a queryid using a generic plan and the one
using a custom one.
I don't like this because this may double the number of entries in pgss.
Which means that the number of entries can more easily reach
pg_stat_statements.max and some entries will be discarded.
Not all the entries will be doubled, only the ones that are prepared.
And even if auto prepare was implemented, having 5000, 10000 or 20000
max entries seems not a problem to me.
I thought it might be preferable to make a GUC to enable
or disable this feature, but changing the hash key makes
it harder.
What happens if the server was running with this option enabled and then
restarted with the option disabled? Firstly two entries for the same query
were stored in pgss because the option was enabled. But when it's disabled
and the server is restarted, those two entries should be merged into one
at the startup of server? If so, that's problematic because it may take
a long time.
What would you think about a third value for this flag to handle disabled
case (no need to merge entries in this situation) ?
Therefore I think that it's better and simple to just expose the number of
times generic/custom plan was chosen for each query.
I thought this feature was mainly needed to identifiy "under optimal generic
plans". Without differentiating execution counters, this will be simpler but
useless in this case ... isn't it ?
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
Regards
PAscal
On 2020-07-30 14:31, Fujii Masao wrote:
On 2020/07/22 16:49, torikoshia wrote:
On 2020-07-20 13:57, torikoshia wrote:
As I proposed earlier in this thread, I'm now trying to add
information
about generic/cudstom plan to pg_stat_statements.
I'll share the idea and the poc patch soon.Attached a poc patch.
Thanks for the POC patch!
With the patch, when I ran "CREATE EXTENSION pg_stat_statements",
I got the following error.ERROR: function pg_stat_statements_reset(oid, oid, bigint) does not
exist
Oops, sorry about that.
I just fixed it there for now.
Main purpose is to decide (1) the user interface and (2) the
way to get the plan type from pg_stat_statements.(1) the user interface
I added a new boolean column 'generic_plan' to both
pg_stat_statements view and the member of the hash key of
pg_stat_statements.This is because as Legrand pointed out the feature seems
useful under the condition of differentiating all the
counters for a queryid using a generic plan and the one
using a custom one.I don't like this because this may double the number of entries in
pgss.
Which means that the number of entries can more easily reach
pg_stat_statements.max and some entries will be discarded.I thought it might be preferable to make a GUC to enable
or disable this feature, but changing the hash key makes
it harder.What happens if the server was running with this option enabled and
then
restarted with the option disabled? Firstly two entries for the same
query
were stored in pgss because the option was enabled. But when it's
disabled
and the server is restarted, those two entries should be merged into
one
at the startup of server? If so, that's problematic because it may take
a long time.Therefore I think that it's better and simple to just expose the number
of
times generic/custom plan was chosen for each query.Regards,
Regards,
--
Atsushi Torikoshi
NTT DATA CORPORATION
Attachments:
0002-POC-add-plan-type-to-pgss.patchtext/x-diff; name=0002-POC-add-plan-type-to-pgss.patchDownload
From 793eafad8e988b6754c9d89e0ea14b64b07eef81 Mon Sep 17 00:00:00 2001
From: Atsushi Torikoshi<torikoshia@oss.nttdata.com>
Date: Fri, 31 Jul 2020 17:52:14 +0900
Subject: [PATCH] Previously the number of custom and generic plans are
recoreded only in pg_prepared_statements, meaning we could only track them
regarding current session. This patch records them in pg_stat_statements and
it enables to track them regarding all sessions of the PostgreSQL instance.
---
.../pg_stat_statements--1.6--1.7.sql | 5 ++-
.../pg_stat_statements--1.7--1.8.sql | 1 +
.../pg_stat_statements/pg_stat_statements.c | 44 +++++++++++++++----
src/backend/utils/cache/plancache.c | 2 +
src/include/utils/plancache.h | 1 +
5 files changed, 42 insertions(+), 11 deletions(-)
diff --git a/contrib/pg_stat_statements/pg_stat_statements--1.6--1.7.sql b/contrib/pg_stat_statements/pg_stat_statements--1.6--1.7.sql
index 6fc3fed4c9..fd7aa05c92 100644
--- a/contrib/pg_stat_statements/pg_stat_statements--1.6--1.7.sql
+++ b/contrib/pg_stat_statements/pg_stat_statements--1.6--1.7.sql
@@ -12,11 +12,12 @@ DROP FUNCTION pg_stat_statements_reset();
/* Now redefine */
CREATE FUNCTION pg_stat_statements_reset(IN userid Oid DEFAULT 0,
IN dbid Oid DEFAULT 0,
- IN queryid bigint DEFAULT 0
+ IN queryid bigint DEFAULT 0,
+ IN generic_plan int DEFAULT -1
)
RETURNS void
AS 'MODULE_PATHNAME', 'pg_stat_statements_reset_1_7'
LANGUAGE C STRICT PARALLEL SAFE;
-- Don't want this to be available to non-superusers.
-REVOKE ALL ON FUNCTION pg_stat_statements_reset(Oid, Oid, bigint) FROM PUBLIC;
+REVOKE ALL ON FUNCTION pg_stat_statements_reset(Oid, Oid, bigint, int) FROM PUBLIC;
diff --git a/contrib/pg_stat_statements/pg_stat_statements--1.7--1.8.sql b/contrib/pg_stat_statements/pg_stat_statements--1.7--1.8.sql
index 0f63f08f7e..0d7c4e7343 100644
--- a/contrib/pg_stat_statements/pg_stat_statements--1.7--1.8.sql
+++ b/contrib/pg_stat_statements/pg_stat_statements--1.7--1.8.sql
@@ -15,6 +15,7 @@ DROP FUNCTION pg_stat_statements(boolean);
CREATE FUNCTION pg_stat_statements(IN showtext boolean,
OUT userid oid,
OUT dbid oid,
+ OUT generic_plan bool,
OUT queryid bigint,
OUT query text,
OUT plans int8,
diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index 6b91c62c31..14c580a95e 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -78,9 +78,11 @@
#include "storage/ipc.h"
#include "storage/spin.h"
#include "tcop/utility.h"
+#include "tcop/pquery.h"
#include "utils/acl.h"
#include "utils/builtins.h"
#include "utils/memutils.h"
+#include "utils/plancache.h"
PG_MODULE_MAGIC;
@@ -156,6 +158,7 @@ typedef struct pgssHashKey
Oid userid; /* user OID */
Oid dbid; /* database OID */
uint64 queryid; /* query identifier */
+ bool is_generic_plan;
} pgssHashKey;
/*
@@ -266,6 +269,9 @@ static int exec_nested_level = 0;
/* Current nesting depth of planner calls */
static int plan_nested_level = 0;
+/* Current plan type */
+static bool is_generic_plan = false;
+
/* Saved hook values in case of unload */
static shmem_startup_hook_type prev_shmem_startup_hook = NULL;
static post_parse_analyze_hook_type prev_post_parse_analyze_hook = NULL;
@@ -367,7 +373,7 @@ static char *qtext_fetch(Size query_offset, int query_len,
char *buffer, Size buffer_size);
static bool need_gc_qtexts(void);
static void gc_qtexts(void);
-static void entry_reset(Oid userid, Oid dbid, uint64 queryid);
+static void entry_reset(Oid userid, Oid dbid, uint64 queryid, int generic_plan);
static void AppendJumble(pgssJumbleState *jstate,
const unsigned char *item, Size size);
static void JumbleQuery(pgssJumbleState *jstate, Query *query);
@@ -1013,6 +1019,16 @@ pgss_ExecutorStart(QueryDesc *queryDesc, int eflags)
*/
if (pgss_enabled(exec_nested_level) && queryDesc->plannedstmt->queryId != UINT64CONST(0))
{
+ /*
+ * ActivePortal is not available at ExecutorEnd. We preserve
+ * the plan type from ActivePortal here.
+ */
+ Assert(ActivePortal);
+ if(ActivePortal->cplan != NULL && ActivePortal->cplan->is_generic)
+ is_generic_plan = true;
+ else
+ is_generic_plan = false;
+
/*
* Set up to track total elapsed time in ExecutorRun. Make sure the
* space is allocated in the per-query context so it will go away at
@@ -1100,6 +1116,8 @@ pgss_ExecutorEnd(QueryDesc *queryDesc)
&queryDesc->totaltime->walusage,
NULL);
}
+ /* reset the plan type. */
+ is_generic_plan = false;
if (prev_ExecutorEnd)
prev_ExecutorEnd(queryDesc);
@@ -1307,9 +1325,11 @@ pgss_store(const char *query, uint64 queryId,
}
/* Set up key for hashtable search */
+ memset(&key, 0, sizeof(key));
key.userid = GetUserId();
key.dbid = MyDatabaseId;
key.queryid = queryId;
+ key.is_generic_plan = is_generic_plan;
/* Lookup the hash table entry with shared lock. */
LWLockAcquire(pgss->lock, LW_SHARED);
@@ -1463,12 +1483,14 @@ pg_stat_statements_reset_1_7(PG_FUNCTION_ARGS)
Oid userid;
Oid dbid;
uint64 queryid;
+ int generic_plan;
userid = PG_GETARG_OID(0);
dbid = PG_GETARG_OID(1);
queryid = (uint64) PG_GETARG_INT64(2);
+ generic_plan = (uint32) PG_GETARG_INT32(3);
- entry_reset(userid, dbid, queryid);
+ entry_reset(userid, dbid, queryid, generic_plan);
PG_RETURN_VOID();
}
@@ -1479,7 +1501,7 @@ pg_stat_statements_reset_1_7(PG_FUNCTION_ARGS)
Datum
pg_stat_statements_reset(PG_FUNCTION_ARGS)
{
- entry_reset(0, 0, 0);
+ entry_reset(0, 0, 0, -1);
PG_RETURN_VOID();
}
@@ -1489,8 +1511,8 @@ pg_stat_statements_reset(PG_FUNCTION_ARGS)
#define PG_STAT_STATEMENTS_COLS_V1_1 18
#define PG_STAT_STATEMENTS_COLS_V1_2 19
#define PG_STAT_STATEMENTS_COLS_V1_3 23
-#define PG_STAT_STATEMENTS_COLS_V1_8 32
-#define PG_STAT_STATEMENTS_COLS 32 /* maximum of above */
+#define PG_STAT_STATEMENTS_COLS_V1_8 33
+#define PG_STAT_STATEMENTS_COLS 33 /* maximum of above */
/*
* Retrieve statement statistics.
@@ -1712,6 +1734,7 @@ pg_stat_statements_internal(FunctionCallInfo fcinfo,
values[i++] = ObjectIdGetDatum(entry->key.userid);
values[i++] = ObjectIdGetDatum(entry->key.dbid);
+ values[i++] = BoolGetDatum(entry->key.is_generic_plan);
if (is_allowed_role || entry->key.userid == userid)
{
@@ -2451,7 +2474,7 @@ gc_fail:
* Release entries corresponding to parameters passed.
*/
static void
-entry_reset(Oid userid, Oid dbid, uint64 queryid)
+entry_reset(Oid userid, Oid dbid, uint64 queryid, int generic_plan)
{
HASH_SEQ_STATUS hash_seq;
pgssEntry *entry;
@@ -2468,19 +2491,21 @@ entry_reset(Oid userid, Oid dbid, uint64 queryid)
LWLockAcquire(pgss->lock, LW_EXCLUSIVE);
num_entries = hash_get_num_entries(pgss_hash);
- if (userid != 0 && dbid != 0 && queryid != UINT64CONST(0))
+ if (userid != 0 && dbid != 0 && queryid != UINT64CONST(0) && generic_plan != -1)
{
/* If all the parameters are available, use the fast path. */
+ memset(&key, 0, sizeof(key));
key.userid = userid;
key.dbid = dbid;
key.queryid = queryid;
+ key.is_generic_plan = generic_plan;
/* Remove the key if exists */
entry = (pgssEntry *) hash_search(pgss_hash, &key, HASH_REMOVE, NULL);
if (entry) /* found */
num_remove++;
}
- else if (userid != 0 || dbid != 0 || queryid != UINT64CONST(0))
+ else if (userid != 0 || dbid != 0 || queryid != UINT64CONST(0) || generic_plan != -1)
{
/* Remove entries corresponding to valid parameters. */
hash_seq_init(&hash_seq, pgss_hash);
@@ -2488,7 +2513,8 @@ entry_reset(Oid userid, Oid dbid, uint64 queryid)
{
if ((!userid || entry->key.userid == userid) &&
(!dbid || entry->key.dbid == dbid) &&
- (!queryid || entry->key.queryid == queryid))
+ (!queryid || entry->key.queryid == queryid) &&
+ (generic_plan == -1 || entry->key.is_generic_plan == generic_plan))
{
hash_search(pgss_hash, &entry->key, HASH_REMOVE, NULL);
num_remove++;
diff --git a/src/backend/utils/cache/plancache.c b/src/backend/utils/cache/plancache.c
index 50d6ad28b4..62e0539370 100644
--- a/src/backend/utils/cache/plancache.c
+++ b/src/backend/utils/cache/plancache.c
@@ -1219,10 +1219,12 @@ GetCachedPlan(CachedPlanSource *plansource, ParamListInfo boundParams,
plansource->total_custom_cost += cached_plan_cost(plan, true);
plansource->num_custom_plans++;
+ plan->is_generic = false;
}
else
{
plansource->num_generic_plans++;
+ plan->is_generic = true;
}
Assert(plan != NULL);
diff --git a/src/include/utils/plancache.h b/src/include/utils/plancache.h
index 4901568553..e4f076fdb3 100644
--- a/src/include/utils/plancache.h
+++ b/src/include/utils/plancache.h
@@ -158,6 +158,7 @@ typedef struct CachedPlan
int generation; /* parent's generation number for this plan */
int refcount; /* count of live references to this struct */
MemoryContext context; /* context containing this CachedPlan */
+ bool is_generic; /* is this plan generic or not */
} CachedPlan;
/*
--
2.18.1
On 2020/07/30 16:34, legrand legrand wrote:
Main purpose is to decide (1) the user interface and (2) the
way to get the plan type from pg_stat_statements.(1) the user interface
I added a new boolean column 'generic_plan' to both
pg_stat_statements view and the member of the hash key of
pg_stat_statements.This is because as Legrand pointed out the feature seems
useful under the condition of differentiating all the
counters for a queryid using a generic plan and the one
using a custom one.I don't like this because this may double the number of entries in pgss.
Which means that the number of entries can more easily reach
pg_stat_statements.max and some entries will be discarded.Not all the entries will be doubled, only the ones that are prepared.
And even if auto prepare was implemented, having 5000, 10000 or 20000
max entries seems not a problem to me.I thought it might be preferable to make a GUC to enable
or disable this feature, but changing the hash key makes
it harder.What happens if the server was running with this option enabled and then
restarted with the option disabled? Firstly two entries for the same query
were stored in pgss because the option was enabled. But when it's disabled
and the server is restarted, those two entries should be merged into one
at the startup of server? If so, that's problematic because it may take
a long time.What would you think about a third value for this flag to handle disabled
case (no need to merge entries in this situation) ?
Sorry I failed to understand your point. You mean that we can have another flag
to specify whether to merge the entries for the same query at that case or not?
If those entries are not merged, what does pg_stat_statements return?
It returns the sum of those entries? Or either generic or custom entry is
returned?
Therefore I think that it's better and simple to just expose the number of
times generic/custom plan was chosen for each query.I thought this feature was mainly needed to identifiy "under optimal generic
plans". Without differentiating execution counters, this will be simpler but
useless in this case ... isn't it ?
Could you elaborate "under optimal generic plans"? Sorry, I failed to
understand your point.. But maybe you're thinking to use this feature to
check which generic or custom plan is better for each query?
I was just thinking that this feature was useful to detect the case where
the query was executed with unpected plan mode. That is, we can detect
the unexpected case where the query that should be executed with generic
plan is actually executed with custom plan, and vice versa.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
I thought it might be preferable to make a GUC to enable
or disable this feature, but changing the hash key makes
it harder.What happens if the server was running with this option enabled and then
restarted with the option disabled? Firstly two entries for the same query
were stored in pgss because the option was enabled. But when it's disabled
and the server is restarted, those two entries should be merged into one
at the startup of server? If so, that's problematic because it may take
a long time.What would you think about a third value for this flag to handle disabled
case (no need to merge entries in this situation) ?Sorry I failed to understand your point. You mean that we can have another flag
to specify whether to merge the entries for the same query at that case or not?If those entries are not merged, what does pg_stat_statements return?
It returns the sum of those entries? Or either generic or custom entry is
returned?
The idea is to use a plan_type enum with 'generic','custom','unknown' or 'unset'.
if tracking plan_type is disabled, then plan_type='unknown',
else plan_type can take 'generic' or 'custom' value.
I'm not proposing to merge results for plan_type when disabling or enabling its tracking.
Therefore I think that it's better and simple to just expose the number of
times generic/custom plan was chosen for each query.I thought this feature was mainly needed to identifiy "under optimal generic
plans". Without differentiating execution counters, this will be simpler but
useless in this case ... isn't it ?
Could you elaborate "under optimal generic plans"? Sorry, I failed to
understand your point.. But maybe you're thinking to use this feature to
check which generic or custom plan is better for each query?
I was just thinking that this feature was useful to detect the case where
the query was executed with unpected plan mode. That is, we can detect
the unexpected case where the query that should be executed with generic
plan is actually executed with custom plan, and vice versa.
There are many exemples in pg lists, where users comes saying that sometimes
their query takes a (very) longer time than before, without understand why.
I some of some exemples, it is that there is a switch from custom to generic after
n executions, and it takes a longer time because generic plan is not as good as
custom one (I call them under optimal generic plans). If pgss keeps counters
aggregated for both plan_types, I don't see how this (under optimal) can be shown.
If there is a line in pgss for custom and an other for generic, then it would be easier
to compare.
Does this makes sence ?
Regards
PAscal
Show quoted text
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
On Fri, Jul 31, 2020 at 06:47:48PM +0900, torikoshia wrote:
Oops, sorry about that.
I just fixed it there for now.
The regression tests of the patch look unstable, and the CF bot is
reporting a failure here:
https://travis-ci.org/github/postgresql-cfbot/postgresql/builds/727833416
--
Michael
On 2020-09-17 13:46, Michael Paquier wrote:
On Fri, Jul 31, 2020 at 06:47:48PM +0900, torikoshia wrote:
Oops, sorry about that.
I just fixed it there for now.The regression tests of the patch look unstable, and the CF bot is
reporting a failure here:
https://travis-ci.org/github/postgresql-cfbot/postgresql/builds/727833416
--
Michael
Thank you for letting me know!
I'd like to reach a basic agreement on how we expose the
generic/custom plan information in pgss first.
Given the discussion so far, adding a new attribute to pgss key
is not appropriate since it can easily increase the number of
entries in pgss.
OTOH, just exposing the number of times generic/custom plan was
chosen seems not enough to know whether performance is degraded.
I'm now thinking about exposing not only the number of times
generic/custom plan was chosen but also some performance
metrics like 'total_time' for both generic and custom plans.
Attached a poc patch which exposes total, min, max, mean and
stddev time for both generic and custom plans.
=# SELECT * FROM =# SELECT * FROM pg_stat_statements;
-[ RECORD 1
]-------+---------------------------------------------------------
userid | 10
dbid | 12878
queryid | 4617094108938234366
query | PREPARE pr1 AS SELECT * FROM pg_class WHERE
relname = $1
plans | 0
total_plan_time | 0
min_plan_time | 0
max_plan_time | 0
mean_plan_time | 0
stddev_plan_time | 0
calls | 6
total_exec_time | 0.46600699999999995
min_exec_time | 0.029376000000000003
max_exec_time | 0.237413
mean_exec_time | 0.07766783333333334
stddev_exec_time | 0.07254973134206326
generic_calls | 1
total_generic_time | 0.045334000000000006
min_generic_time | 0.045334000000000006
max_generic_time | 0.045334000000000006
mean_generic_time | 0.045334000000000006
stddev_generic_time | 0
custom_calls | 5
total_custom_time | 0.42067299999999996
min_custom_time | 0.029376000000000003
max_custom_time | 0.237413
mean_custom_time | 0.0841346
stddev_custom_time | 0.07787966226583164
...
In this patch, exposing new columns is mandatory, but I think
it's better to make it optional by adding a GUC something
like 'pgss.track_general_custom_plans.
I also feel it makes the number of columns too many.
Just adding the total time may be sufficient.
Any thoughts?
Regards,
--
Atsushi Torikoshi
Attachments:
0003-POC-add-plan-type-to-pgss.patchtext/x-diff; name=0003-POC-add-plan-type-to-pgss.patchDownload
diff --git a/contrib/pg_stat_statements/pg_stat_statements--1.7--1.8.sql b/contrib/pg_stat_statements/pg_stat_statements--1.7--1.8.sql
index 0f63f08f7e..d118422b2a 100644
--- a/contrib/pg_stat_statements/pg_stat_statements--1.7--1.8.sql
+++ b/contrib/pg_stat_statements/pg_stat_statements--1.7--1.8.sql
@@ -29,6 +29,18 @@ CREATE FUNCTION pg_stat_statements(IN showtext boolean,
OUT max_exec_time float8,
OUT mean_exec_time float8,
OUT stddev_exec_time float8,
+ OUT generic_calls int8,
+ OUT total_generic_time float8,
+ OUT min_generic_time float8,
+ OUT max_generic_time float8,
+ OUT mean_generic_time float8,
+ OUT stddev_generic_time float8,
+ OUT custom_calls int8,
+ OUT total_custom_time float8,
+ OUT min_custom_time float8,
+ OUT max_custom_time float8,
+ OUT mean_custom_time float8,
+ OUT stddev_custom_time float8,
OUT rows int8,
OUT shared_blks_hit int8,
OUT shared_blks_read int8,
diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index 1eac9edaee..73d82a632d 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -78,9 +78,11 @@
#include "storage/ipc.h"
#include "storage/spin.h"
#include "tcop/utility.h"
+#include "tcop/pquery.h"
#include "utils/acl.h"
#include "utils/builtins.h"
#include "utils/memutils.h"
+#include "utils/plancache.h"
PG_MODULE_MAGIC;
@@ -138,6 +140,8 @@ typedef enum pgssStoreKind
*/
PGSS_PLAN = 0,
PGSS_EXEC,
+ PGSS_EXEC_GENERAL,
+ PGSS_EXEC_CUSTOM,
PGSS_NUMKIND /* Must be last value of this enum */
} pgssStoreKind;
@@ -258,6 +262,13 @@ typedef struct pgssJumbleState
int highest_extern_param_id;
} pgssJumbleState;
+typedef enum pgssPlanType
+{
+ PGSS_PLAN_TYPE_NONE,
+ PGSS_PLAN_TYPE_GENERIC,
+ PGSS_PLAN_TYPE_CUSTOM,
+} pgssPlanType;
+
/*---- Local variables ----*/
/* Current nesting depth of ExecutorRun+ProcessUtility calls */
@@ -266,6 +277,9 @@ static int exec_nested_level = 0;
/* Current nesting depth of planner calls */
static int plan_nested_level = 0;
+/* Current plan type */
+static pgssPlanType plantype = PGSS_PLAN_TYPE_NONE;
+
/* Saved hook values in case of unload */
static shmem_startup_hook_type prev_shmem_startup_hook = NULL;
static post_parse_analyze_hook_type prev_post_parse_analyze_hook = NULL;
@@ -1013,6 +1027,20 @@ pgss_ExecutorStart(QueryDesc *queryDesc, int eflags)
*/
if (pgss_enabled(exec_nested_level) && queryDesc->plannedstmt->queryId != UINT64CONST(0))
{
+ /*
+ * Since ActivePortal is not available at ExecutorEnd, We
+ * preserve the plan type here.
+ */
+ Assert(ActivePortal);
+
+ if (ActivePortal->cplan)
+ {
+ if (ActivePortal->cplan->is_generic)
+ plantype = PGSS_PLAN_TYPE_GENERIC;
+ else
+ plantype = PGSS_PLAN_TYPE_CUSTOM;
+ }
+
/*
* Set up to track total elapsed time in ExecutorRun. Make sure the
* space is allocated in the per-query context so it will go away at
@@ -1100,6 +1128,8 @@ pgss_ExecutorEnd(QueryDesc *queryDesc)
&queryDesc->totaltime->walusage,
NULL);
}
+ /* Reset the plan type. */
+ plantype = PGSS_PLAN_TYPE_NONE;
if (prev_ExecutorEnd)
prev_ExecutorEnd(queryDesc);
@@ -1224,6 +1254,41 @@ pgss_hash_string(const char *str, int len)
len, 0));
}
+/* Note: Caller must acquire SpinLock on pgssEntry before using this function. */
+static void
+kind_dependent_accumulation(volatile pgssEntry *e, pgssStoreKind kind, double total_time)
+{
+ e->counters.calls[kind] += 1;
+ e->counters.total_time[kind] += total_time;
+
+ if (e->counters.calls[kind] == 1)
+ {
+ e->counters.min_time[kind] = total_time;
+ e->counters.max_time[kind] = total_time;
+ e->counters.mean_time[kind] = total_time;
+ }
+ else
+ {
+ /*
+ * Welford's method for accurately computing variance. See
+ * <http://www.johndcook.com/blog/standard_deviation/>
+ */
+ double old_mean = e->counters.mean_time[kind];
+
+ e->counters.mean_time[kind] +=
+ (total_time - old_mean) / e->counters.calls[kind];
+ e->counters.sum_var_time[kind] +=
+ (total_time - old_mean) * (total_time - e->counters.mean_time[kind]);
+
+ /* calculate min and max time */
+ if (e->counters.min_time[kind] > total_time)
+ e->counters.min_time[kind] = total_time;
+ if (e->counters.max_time[kind] < total_time)
+ e->counters.max_time[kind] = total_time;
+ }
+}
+
+
/*
* Store some statistics for a statement.
*
@@ -1396,34 +1461,20 @@ pgss_store(const char *query, uint64 queryId,
if (IS_STICKY(e->counters))
e->counters.usage = USAGE_INIT;
- e->counters.calls[kind] += 1;
- e->counters.total_time[kind] += total_time;
+ kind_dependent_accumulation(e, kind, total_time);
- if (e->counters.calls[kind] == 1)
- {
- e->counters.min_time[kind] = total_time;
- e->counters.max_time[kind] = total_time;
- e->counters.mean_time[kind] = total_time;
- }
- else
+ /*
+ * Accumulate information about general and custom plan.
+ * TODO: It would be better to be optional.
+ */
+ if (kind == PGSS_EXEC)
{
- /*
- * Welford's method for accurately computing variance. See
- * <http://www.johndcook.com/blog/standard_deviation/>
- */
- double old_mean = e->counters.mean_time[kind];
-
- e->counters.mean_time[kind] +=
- (total_time - old_mean) / e->counters.calls[kind];
- e->counters.sum_var_time[kind] +=
- (total_time - old_mean) * (total_time - e->counters.mean_time[kind]);
-
- /* calculate min and max time */
- if (e->counters.min_time[kind] > total_time)
- e->counters.min_time[kind] = total_time;
- if (e->counters.max_time[kind] < total_time)
- e->counters.max_time[kind] = total_time;
+ if (plantype == PGSS_PLAN_TYPE_GENERIC)
+ kind_dependent_accumulation(e, PGSS_EXEC_GENERAL, total_time);
+ else if (plantype == PGSS_PLAN_TYPE_CUSTOM)
+ kind_dependent_accumulation(e, PGSS_EXEC_CUSTOM, total_time);
}
+
e->counters.rows += rows;
e->counters.shared_blks_hit += bufusage->shared_blks_hit;
e->counters.shared_blks_read += bufusage->shared_blks_read;
@@ -1488,8 +1539,8 @@ pg_stat_statements_reset(PG_FUNCTION_ARGS)
#define PG_STAT_STATEMENTS_COLS_V1_1 18
#define PG_STAT_STATEMENTS_COLS_V1_2 19
#define PG_STAT_STATEMENTS_COLS_V1_3 23
-#define PG_STAT_STATEMENTS_COLS_V1_8 32
-#define PG_STAT_STATEMENTS_COLS 32 /* maximum of above */
+#define PG_STAT_STATEMENTS_COLS_V1_8 44
+#define PG_STAT_STATEMENTS_COLS 44 /* maximum of above */
/*
* Retrieve statement statistics.
diff --git a/src/backend/utils/cache/plancache.c b/src/backend/utils/cache/plancache.c
index 50d6ad28b4..47dc74a634 100644
--- a/src/backend/utils/cache/plancache.c
+++ b/src/backend/utils/cache/plancache.c
@@ -1217,12 +1217,14 @@ GetCachedPlan(CachedPlanSource *plansource, ParamListInfo boundParams,
plan = BuildCachedPlan(plansource, qlist, boundParams, queryEnv);
/* Accumulate total costs of custom plans */
plansource->total_custom_cost += cached_plan_cost(plan, true);
+ plan->is_generic = false;
plansource->num_custom_plans++;
}
else
{
plansource->num_generic_plans++;
+ plan->is_generic = true;
}
Assert(plan != NULL);
diff --git a/src/include/utils/plancache.h b/src/include/utils/plancache.h
index 4901568553..4edc15602a 100644
--- a/src/include/utils/plancache.h
+++ b/src/include/utils/plancache.h
@@ -158,6 +158,7 @@ typedef struct CachedPlan
int generation; /* parent's generation number for this plan */
int refcount; /* count of live references to this struct */
MemoryContext context; /* context containing this CachedPlan */
+ bool is_generic; /* is this plan generic or not? */
} CachedPlan;
/*
Hi Atsushi,
+1: Your proposal is a good answer for time based performance analysis
(even if parsing durationor blks are not differentiated) .
As it makes pgss number of columns wilder, may be an other solution
would be to create a pg_stat_statements_xxx view with the same key
as pgss (dbid,userid,queryid) and all thoses new counters.
And last solution would be to display only generic counters,
because in most cases (and per default) executions are custom ones
(and generic counters = 0).
if not (when generic counters != 0), customs ones could be deducted from
total_exec_time - total_generic_time, calls - generic_calls.
Good luck for this feature development
Regards
PAscal
--
Sent from: https://www.postgresql-archive.org/PostgreSQL-hackers-f1928748.html
On 2020-09-29 02:39, legrand legrand wrote:
Hi Atsushi,
+1: Your proposal is a good answer for time based performance analysis
(even if parsing durationor blks are not differentiated) .As it makes pgss number of columns wilder, may be an other solution
would be to create a pg_stat_statements_xxx view with the same key
as pgss (dbid,userid,queryid) and all thoses new counters.
Thanks for your ideas and sorry for my late reply.
It seems creating pg_stat_statements_xxx views both for generic and
custom plans is better than my PoC patch.
However, I also began to wonder how effective it would be to just
distinguish between generic and custom plans. Custom plans can
include all sorts of plans. and thinking cache validation, generic
plans can also include various plans.
Considering this, I'm starting to feel that it would be better to
not just keeping whether generic or cutom but the plan itself as
discussed in the below thread.
/messages/by-id/CAKU4AWq5_jx1Vyai0_Sumgn-Ks0R+N80cf+t170+zQs8x6=Hew@mail.gmail.com
Any thoughts?
Regards,
--
Atsushi Torikoshi
čt 12. 11. 2020 v 2:50 odesílatel torikoshia <torikoshia@oss.nttdata.com>
napsal:
On 2020-09-29 02:39, legrand legrand wrote:
Hi Atsushi,
+1: Your proposal is a good answer for time based performance analysis
(even if parsing durationor blks are not differentiated) .As it makes pgss number of columns wilder, may be an other solution
would be to create a pg_stat_statements_xxx view with the same key
as pgss (dbid,userid,queryid) and all thoses new counters.Thanks for your ideas and sorry for my late reply.
It seems creating pg_stat_statements_xxx views both for generic and
custom plans is better than my PoC patch.However, I also began to wonder how effective it would be to just
distinguish between generic and custom plans. Custom plans can
include all sorts of plans. and thinking cache validation, generic
plans can also include various plans.Considering this, I'm starting to feel that it would be better to
not just keeping whether generic or cutom but the plan itself as
discussed in the below thread./messages/by-id/CAKU4AWq5_jx1Vyai0_Sumgn-Ks0R+N80cf+t170+zQs8x6=Hew@mail.gmail.com
Any thoughts?
yes, the plan self is very interesting information - and information if
plan was generic or not is interesting too. It is other dimension of query
- maybe there can be rule - for any query store max 100 most slows plans
with all attributes. The next issue is fact so first first 5 execution of
generic plans are not generic in real. This fact should be visible too.
Regards
Pavel
Show quoted text
Regards,
--
Atsushi Torikoshi
On 2020-11-12 14:23, Pavel Stehule wrote:
yes, the plan self is very interesting information - and information
if plan was generic or not is interesting too. It is other dimension
of query - maybe there can be rule - for any query store max 100 most
slows plans with all attributes. The next issue is fact so first first
5 execution of generic plans are not generic in real. This fact should
be visible too.
Thanks!
However, AFAIU, we can know whether the plan type is generic or custom
from the plan information as described in the manual.
-- https://www.postgresql.org/docs/devel/sql-prepare.html
If a generic plan is in use, it will contain parameter symbols $n,
while a custom plan will have the supplied parameter values substituted
into it.
If we can get the plan information, the case like 'first 5 execution
of generic plans are not generic in real' does not happen, doesn't it?
Regards,
--
Atsushi Torikoshi
út 17. 11. 2020 v 15:21 odesílatel torikoshia <torikoshia@oss.nttdata.com>
napsal:
On 2020-11-12 14:23, Pavel Stehule wrote:
yes, the plan self is very interesting information - and information
if plan was generic or not is interesting too. It is other dimension
of query - maybe there can be rule - for any query store max 100 most
slows plans with all attributes. The next issue is fact so first first
5 execution of generic plans are not generic in real. This fact should
be visible too.Thanks!
However, AFAIU, we can know whether the plan type is generic or custom
from the plan information as described in the manual.-- https://www.postgresql.org/docs/devel/sql-prepare.html
If a generic plan is in use, it will contain parameter symbols $n,
while a custom plan will have the supplied parameter values substituted
into it.If we can get the plan information, the case like 'first 5 execution
of generic plans are not generic in real' does not happen, doesn't it?
yes
postgres=# create table foo(a int);
CREATE TABLE
postgres=# prepare x as select * from foo where a = $1;
PREPARE
postgres=# explain execute x(10);
┌─────────────────────────────────────────────────────┐
│ QUERY PLAN │
╞═════════════════════════════════════════════════════╡
│ Seq Scan on foo (cost=0.00..41.88 rows=13 width=4) │
│ Filter: (a = 10) │
└─────────────────────────────────────────────────────┘
(2 rows)
postgres=# explain execute x(10);
┌─────────────────────────────────────────────────────┐
│ QUERY PLAN │
╞═════════════════════════════════════════════════════╡
│ Seq Scan on foo (cost=0.00..41.88 rows=13 width=4) │
│ Filter: (a = 10) │
└─────────────────────────────────────────────────────┘
(2 rows)
postgres=# explain execute x(10);
┌─────────────────────────────────────────────────────┐
│ QUERY PLAN │
╞═════════════════════════════════════════════════════╡
│ Seq Scan on foo (cost=0.00..41.88 rows=13 width=4) │
│ Filter: (a = 10) │
└─────────────────────────────────────────────────────┘
(2 rows)
postgres=# explain execute x(10);
┌─────────────────────────────────────────────────────┐
│ QUERY PLAN │
╞═════════════════════════════════════════════════════╡
│ Seq Scan on foo (cost=0.00..41.88 rows=13 width=4) │
│ Filter: (a = 10) │
└─────────────────────────────────────────────────────┘
(2 rows)
postgres=# explain execute x(10);
┌─────────────────────────────────────────────────────┐
│ QUERY PLAN │
╞═════════════════════════════════════════════════════╡
│ Seq Scan on foo (cost=0.00..41.88 rows=13 width=4) │
│ Filter: (a = 10) │
└─────────────────────────────────────────────────────┘
(2 rows)
postgres=# explain execute x(10);
┌─────────────────────────────────────────────────────┐
│ QUERY PLAN │
╞═════════════════════════════════════════════════════╡
│ Seq Scan on foo (cost=0.00..41.88 rows=13 width=4) │
│ Filter: (a = $1) │
└─────────────────────────────────────────────────────┘
(2 rows)
postgres=# explain execute x(10);
┌─────────────────────────────────────────────────────┐
│ QUERY PLAN │
╞═════════════════════════════════════════════════════╡
│ Seq Scan on foo (cost=0.00..41.88 rows=13 width=4) │
│ Filter: (a = $1) │
└─────────────────────────────────────────────────────┘
(2 rows)
Show quoted text
Regards,
--
Atsushi Torikoshi
Hi Torikoshi-san,
In this patch, exposing new columns is mandatory, but I think
it's better to make it optional by adding a GUC something
like 'pgss.track_general_custom_plans.I also feel it makes the number of columns too many.
Just adding the total time may be sufficient.
I think this feature is useful for DBA. So I hope that it gets
committed to PG14. IMHO, many columns are Okay because DBA can
select specific columns by their query.
Therefore, it would be better to go with the current design.
I did the regression test using your patch on 7e5e1bba03, and
it failed unfortunately. See below:
=======================================================
122 of 201 tests failed, 1 of these failures ignored.
=======================================================
...
2020-11-30 09:45:18.160 JST [12977] LOG: database system was not
properly shut down; automatic recovery in progress
Regards,
Tatsuro Yamada
On 2020/11/30 15:24, Tatsuro Yamada wrote:
Hi Torikoshi-san,
In this patch, exposing new columns is mandatory, but I think
it's better to make it optional by adding a GUC something
like 'pgss.track_general_custom_plans.I also feel it makes the number of columns too many.
Just adding the total time may be sufficient.I think this feature is useful for DBA. So I hope that it gets
committed to PG14. IMHO, many columns are Okay because DBA can
select specific columns by their query.
Therefore, it would be better to go with the current design.
But that design may waste lots of memory. No? For example, when
plan_cache_mode=force_custom_plan, the memory used for the columns
for generic plans is not used.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
On 2020-12-04 14:29, Fujii Masao wrote:
On 2020/11/30 15:24, Tatsuro Yamada wrote:
Hi Torikoshi-san,
In this patch, exposing new columns is mandatory, but I think
it's better to make it optional by adding a GUC something
like 'pgss.track_general_custom_plans.I also feel it makes the number of columns too many.
Just adding the total time may be sufficient.I think this feature is useful for DBA. So I hope that it gets
committed to PG14. IMHO, many columns are Okay because DBA can
select specific columns by their query.
Therefore, it would be better to go with the current design.But that design may waste lots of memory. No? For example, when
plan_cache_mode=force_custom_plan, the memory used for the columns
for generic plans is not used.
Yeah.
ISTM now that creating pg_stat_statements_xxx views
both for generic andcustom plans is better than my PoC patch.
And I'm also struggling with the following.
| However, I also began to wonder how effective it would be to just
| distinguish between generic and custom plans. Custom plans can
| include all sorts of plans. and thinking cache validation, generic
| plans can also include various plans.
| Considering this, I'm starting to feel that it would be better to
| not just keeping whether generic or cutom but the plan itself as
| discussed in the below thread.
Yamada-san,
Do you think it's effective just distinguish between generic
and custom plans?
Regards,
At Fri, 04 Dec 2020 15:03:25 +0900, torikoshia <torikoshia@oss.nttdata.com> wrote in
On 2020-12-04 14:29, Fujii Masao wrote:
On 2020/11/30 15:24, Tatsuro Yamada wrote:
Hi Torikoshi-san,
In this patch, exposing new columns is mandatory, but I think
it's better to make it optional by adding a GUC something
like 'pgss.track_general_custom_plans.
I also feel it makes the number of columns too many.
Just adding the total time may be sufficient.I think this feature is useful for DBA. So I hope that it gets
committed to PG14. IMHO, many columns are Okay because DBA can
select specific columns by their query.
Therefore, it would be better to go with the current design.But that design may waste lots of memory. No? For example, when
plan_cache_mode=force_custom_plan, the memory used for the columns
for generic plans is not used.Yeah.
ISTM now that creating pg_stat_statements_xxx views
both for generic andcustom plans is better than my PoC patch.And I'm also struggling with the following.
| However, I also began to wonder how effective it would be to just
| distinguish between generic and custom plans. Custom plans can
| include all sorts of plans. and thinking cache validation, generic
| plans can also include various plans.| Considering this, I'm starting to feel that it would be better to
| not just keeping whether generic or cutom but the plan itself as
| discussed in the below thread.
FWIW, that seems to me to be like some existing extension modules,
pg_stat_plans or pg_store_plans.. The former is faster but may lose
plans, the latter doesn't lose plans but slower. I feel that we'd
beter consider simpler feature if we are intendeng it to be a part of
a contrib module,
Yamada-san,
Do you think it's effective just distinguish between generic
and custom plans?Regards,
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
<torikoshia@oss.nttdata.com> wrote in
ISTM now that creating pg_stat_statements_xxx views
both for generic andcustom plans is better than my PoC patch.
On my second thought, it also makes pg_stat_statements too complicated
compared to what it makes possible..
I'm also worrying that whether taking generic and custom plan execution
time or not would be controlled by a GUC variable, and the default
would be not taking them.
Not many people will change the default.
Since the same queryid can contain various queries (different plan,
different parameter $n, etc.), I also started to feel that it is not
appropriate to get the execution time of only generic/custom queries
separately.
I suppose it would be normal practice to store past results of
pg_stat_statements for future comparisons.
If this is the case, I think that if we only add the number of
generic plan execution, it will give us a hint to notice the cause
of performance degradation due to changes in the plan between
generic and custom.
For example, if there is a clear difference in the number of times
the generic plan is executed between before and after performance
degradation as below, it would be natural to check if there is a
problem with the generic plan.
[after performance degradation]
=# SELECT query, calls, generic_calls FROM pg_stat_statements where
query like '%t1%';
query | calls | generic_calls
---------------------------------------------+-------+---------------
PREPARE p1 as select * from t1 where i = $1 | 1100 | 50
[before performance degradation]
=# SELECT query, calls, generic_calls FROM pg_stat_statements where
query like '%t1%';
query | calls | generic_calls
---------------------------------------------+-------+---------------
PREPARE p1 as select * from t1 where i = $1 | 1000 | 0
Attached a patch that just adds a generic call counter to
pg_stat_statements.
Any thoughts?
Regards,
--
Atsushi Torikoshi
Attachments:
v4-0001-POC-add-plan-type-to-pgss.patchtext/x-diff; name=v4-0001-POC-add-plan-type-to-pgss.patchDownload
diff --git a/contrib/pg_stat_statements/pg_stat_statements--1.7--1.8.sql b/contrib/pg_stat_statements/pg_stat_statements--1.7--1.8.sql
index 0f63f08f7e..7fdef315ae 100644
--- a/contrib/pg_stat_statements/pg_stat_statements--1.7--1.8.sql
+++ b/contrib/pg_stat_statements/pg_stat_statements--1.7--1.8.sql
@@ -44,7 +44,8 @@ CREATE FUNCTION pg_stat_statements(IN showtext boolean,
OUT blk_write_time float8,
OUT wal_records int8,
OUT wal_fpi int8,
- OUT wal_bytes numeric
+ OUT wal_bytes numeric,
+ OUT generic_calls int8
)
RETURNS SETOF record
AS 'MODULE_PATHNAME', 'pg_stat_statements_1_8'
diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index 72a117fc19..171c39f857 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -77,10 +77,12 @@
#include "storage/fd.h"
#include "storage/ipc.h"
#include "storage/spin.h"
+#include "tcop/pquery.h"
#include "tcop/utility.h"
#include "utils/acl.h"
#include "utils/builtins.h"
#include "utils/memutils.h"
+#include "utils/plancache.h"
#include "utils/timestamp.h"
PG_MODULE_MAGIC;
@@ -192,6 +194,7 @@ typedef struct Counters
int64 wal_records; /* # of WAL records generated */
int64 wal_fpi; /* # of WAL full page images generated */
uint64 wal_bytes; /* total amount of WAL generated in bytes */
+ int64 generic_calls; /* # of times generic plans executed */
} Counters;
/*
@@ -277,6 +280,9 @@ static int exec_nested_level = 0;
/* Current nesting depth of planner calls */
static int plan_nested_level = 0;
+/* Current plan type */
+static bool is_plan_type_generic = false;
+
/* Saved hook values in case of unload */
static shmem_startup_hook_type prev_shmem_startup_hook = NULL;
static post_parse_analyze_hook_type prev_post_parse_analyze_hook = NULL;
@@ -1034,6 +1040,20 @@ pgss_ExecutorStart(QueryDesc *queryDesc, int eflags)
*/
if (pgss_enabled(exec_nested_level) && queryDesc->plannedstmt->queryId != UINT64CONST(0))
{
+ /*
+ * Since ActivePortal is not available at ExecutorEnd, we preserve
+ * the plan type here.
+ */
+ Assert(ActivePortal);
+
+ if (ActivePortal->cplan)
+ {
+ if (ActivePortal->cplan->is_generic)
+ is_plan_type_generic = true;
+ else
+ is_plan_type_generic = false;
+ }
+
/*
* Set up to track total elapsed time in ExecutorRun. Make sure the
* space is allocated in the per-query context so it will go away at
@@ -1427,6 +1447,8 @@ pgss_store(const char *query, uint64 queryId,
e->counters.max_time[kind] = total_time;
e->counters.mean_time[kind] = total_time;
}
+ else if (kind == PGSS_EXEC && is_plan_type_generic)
+ e->counters.generic_calls += 1;
else
{
/*
@@ -1510,8 +1532,8 @@ pg_stat_statements_reset(PG_FUNCTION_ARGS)
#define PG_STAT_STATEMENTS_COLS_V1_1 18
#define PG_STAT_STATEMENTS_COLS_V1_2 19
#define PG_STAT_STATEMENTS_COLS_V1_3 23
-#define PG_STAT_STATEMENTS_COLS_V1_8 32
-#define PG_STAT_STATEMENTS_COLS 32 /* maximum of above */
+#define PG_STAT_STATEMENTS_COLS_V1_8 33
+#define PG_STAT_STATEMENTS_COLS 33 /* maximum of above */
/*
* Retrieve statement statistics.
@@ -1863,6 +1885,7 @@ pg_stat_statements_internal(FunctionCallInfo fcinfo,
ObjectIdGetDatum(0),
Int32GetDatum(-1));
values[i++] = wal_bytes;
+ values[i++] = Int64GetDatumFast(tmp.generic_calls);
}
Assert(i == (api_version == PGSS_V1_0 ? PG_STAT_STATEMENTS_COLS_V1_0 :
diff --git a/doc/src/sgml/pgstatstatements.sgml b/doc/src/sgml/pgstatstatements.sgml
index 464bf0e5ae..8c5e304882 100644
--- a/doc/src/sgml/pgstatstatements.sgml
+++ b/doc/src/sgml/pgstatstatements.sgml
@@ -363,6 +363,14 @@
Total amount of WAL generated by the statement in bytes
</para></entry>
</row>
+ <row>
+ <entry role="catalog_table_entry"><para role="column_definition">
+ <structfield>generic_calls</structfield> <type>bigint</type>
+ </para>
+ <para>
+ Number of times the statement was executed as generic plan
+ </para></entry>
+ </row>
</tbody>
</tgroup>
</table>
diff --git a/src/backend/utils/cache/plancache.c b/src/backend/utils/cache/plancache.c
index cc04b5b4be..d9f4523a7e 100644
--- a/src/backend/utils/cache/plancache.c
+++ b/src/backend/utils/cache/plancache.c
@@ -1219,10 +1219,12 @@ GetCachedPlan(CachedPlanSource *plansource, ParamListInfo boundParams,
plansource->total_custom_cost += cached_plan_cost(plan, true);
plansource->num_custom_plans++;
+ plan->is_generic = false;
}
else
{
plansource->num_generic_plans++;
+ plan->is_generic = true;
}
Assert(plan != NULL);
diff --git a/src/include/utils/plancache.h b/src/include/utils/plancache.h
index 79d96e5ed0..5123ce252c 100644
--- a/src/include/utils/plancache.h
+++ b/src/include/utils/plancache.h
@@ -158,6 +158,7 @@ typedef struct CachedPlan
int generation; /* parent's generation number for this plan */
int refcount; /* count of live references to this struct */
MemoryContext context; /* context containing this CachedPlan */
+ bool is_generic; /* is this plan generic or not? */
} CachedPlan;
/*
The following review has been posted through the commitfest application:
make installcheck-world: tested, failed
Implements feature: tested, failed
Spec compliant: not tested
Documentation: not tested
Hi Atsushi,
I just run a few test on your latest patch. It works well. I agree with you, I think just tracking generic_calls is enough to get the reason of performance change from pg_stat_statements. I mean if you need more detailed information about the plan and execution of prepared statements, it is better to store this information in a separate view. It seems more reasonable to me.
Regards,
At Tue, 12 Jan 2021 20:36:58 +0900, torikoshia <torikoshia@oss.nttdata.com> wrote in
I suppose it would be normal practice to store past results of
pg_stat_statements for future comparisons.
If this is the case, I think that if we only add the number of
generic plan execution, it will give us a hint to notice the cause
of performance degradation due to changes in the plan between
generic and custom.
Agreed.
Attached a patch that just adds a generic call counter to
pg_stat_statements.Any thoughts?
Note that ActivePortal is the closest nested portal. So it gives the
wrong result for nested portals.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
On 2020/12/04 14:29, Fujii Masao wrote:
On 2020/11/30 15:24, Tatsuro Yamada wrote:
Hi Torikoshi-san,
In this patch, exposing new columns is mandatory, but I think
it's better to make it optional by adding a GUC something
like 'pgss.track_general_custom_plans.I also feel it makes the number of columns too many.
Just adding the total time may be sufficient.I think this feature is useful for DBA. So I hope that it gets
committed to PG14. IMHO, many columns are Okay because DBA can
select specific columns by their query.
Therefore, it would be better to go with the current design.But that design may waste lots of memory. No? For example, when
plan_cache_mode=force_custom_plan, the memory used for the columns
for generic plans is not used.Regards,
Sorry for the super delayed replay.
I don't think that because I suppose that DBA uses plan_cache_mode if
they faced an inefficient execution plan. And the parameter will be used
as a session-level GUC parameter, not a database-level.
Regards,
Tatsuro Yamada
Torikoshi-san,
On 2020/12/04 15:03, torikoshia wrote:
ISTM now that creating pg_stat_statements_xxx views
both for generic andcustom plans is better than my PoC patch.And I'm also struggling with the following.
| However, I also began to wonder how effective it would be to just
| distinguish between generic and custom plans. Custom plans can
| include all sorts of plans. and thinking cache validation, generic
| plans can also include various plans.| Considering this, I'm starting to feel that it would be better to
| not just keeping whether generic or cutom but the plan itself as
| discussed in the below thread.Yamada-san,
Do you think it's effective just distinguish between generic
and custom plans?
Sorry for the super delayed replay.
Ah, it's okay.
It would be better to check both info by using a single view from the
perspective of usability. However, it's okay to divide both information
into two views to use memory effectively.
Regards,
Tatsuro Yamada
Horiguchi-san,
On 2020/12/04 15:37, Kyotaro Horiguchi wrote:
And I'm also struggling with the following.
| However, I also began to wonder how effective it would be to just
| distinguish between generic and custom plans. Custom plans can
| include all sorts of plans. and thinking cache validation, generic
| plans can also include various plans.| Considering this, I'm starting to feel that it would be better to
| not just keeping whether generic or cutom but the plan itself as
| discussed in the below thread.FWIW, that seems to me to be like some existing extension modules,
pg_stat_plans or pg_store_plans.. The former is faster but may lose
plans, the latter doesn't lose plans but slower. I feel that we'd
beter consider simpler feature if we are intendeng it to be a part of
a contrib module,
There is also pg_show_plans.
Ideally, it would be better to able to track all of the plan changes by
checking something view since Plan Stability is important for DBA when
they use PostgreSQL in Mission-critical systems.
I prefer that the feature will be released as a contrib module. :-D
Regards,
Tatsuro Yamada
Hi Toricoshi-san,
On 2021/01/12 20:36, torikoshia wrote:
I suppose it would be normal practice to store past results of
pg_stat_statements for future comparisons.
If this is the case, I think that if we only add the number of
generic plan execution, it will give us a hint to notice the cause
of performance degradation due to changes in the plan between
generic and custom.For example, if there is a clear difference in the number of times
the generic plan is executed between before and after performance
degradation as below, it would be natural to check if there is a
problem with the generic plan.
...
Attached a patch that just adds a generic call counter to
pg_stat_statements.
I think that I'd like to use the view when we faced a performance
problem and find the reason. If we did the fixed-point observation
(should I say time-series analysis?) of generic_calls, it allows us to
realize the counter changes, and we can know whether the suspect is
generic_plan or not. So the patch helps DBA, I believe.
Regards,
Tatsuro Yamada
Chengxi Sun, Yamada-san, Horiguchi-san,
Thanks for all your comments.
Adding only the number of generic plan execution seems acceptable.
On Mon, Jan 25, 2021 at 2:10 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
Note that ActivePortal is the closest nested portal. So it gives the
wrong result for nested portals.
I may be wrong, but I thought it was ok since the closest nested portal
is the portal to be executed.
ActivePortal is used in ExecutorStart hook in the patch.
And as far as I read PortalStart(), ActivePortal is changed to the
portal to be executed before ExecutorStart().
If possible, could you tell me the specific case which causes wrong
results?
Regards,
--
Atsushi Torikoshi
At Thu, 04 Feb 2021 10:16:47 +0900, torikoshia <torikoshia@oss.nttdata.com> wrote in
Chengxi Sun, Yamada-san, Horiguchi-san,
Thanks for all your comments.
Adding only the number of generic plan execution seems acceptable.On Mon, Jan 25, 2021 at 2:10 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:Note that ActivePortal is the closest nested portal. So it gives the
wrong result for nested portals.I may be wrong, but I thought it was ok since the closest nested
portal is the portal to be executed.
After executing the inner-most portal, is_plan_type_generic has a
value for the inner-most portal and it won't be changed ever after. At
the ExecutorEnd of all the upper-portals see the value for the
inner-most portal left behind is_plan_type_generic nevertheless the
portals at every nest level are indenpendent.
ActivePortal is used in ExecutorStart hook in the patch.
And as far as I read PortalStart(), ActivePortal is changed to the
portal to be executed before ExecutorStart().If possible, could you tell me the specific case which causes wrong
results?
Running a plpgsql function that does PREPRE in a query that does
PREPARE?
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
On 2021-02-04 11:19, Kyotaro Horiguchi wrote:
At Thu, 04 Feb 2021 10:16:47 +0900, torikoshia
<torikoshia@oss.nttdata.com> wrote inChengxi Sun, Yamada-san, Horiguchi-san,
Thanks for all your comments.
Adding only the number of generic plan execution seems acceptable.On Mon, Jan 25, 2021 at 2:10 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:Note that ActivePortal is the closest nested portal. So it gives the
wrong result for nested portals.I may be wrong, but I thought it was ok since the closest nested
portal is the portal to be executed.After executing the inner-most portal, is_plan_type_generic has a
value for the inner-most portal and it won't be changed ever after. At
the ExecutorEnd of all the upper-portals see the value for the
inner-most portal left behind is_plan_type_generic nevertheless the
portals at every nest level are independent.ActivePortal is used in ExecutorStart hook in the patch.
And as far as I read PortalStart(), ActivePortal is changed to the
portal to be executed before ExecutorStart().If possible, could you tell me the specific case which causes wrong
results?Running a plpgsql function that does PREPRE in a query that does
PREPARE?
Thanks for your explanation!
I confirmed that it in fact happened.
To avoid it, attached patch preserves the is_plan_type_generic before
changing it and sets it back at the end of pgss_ExecutorEnd().
Any thoughts?
Regards,
--
Atsushi Torikoshi
Attachments:
v5-0001-add-plan-type-to-pgss.patchtext/x-diff; name=v5-0001-add-plan-type-to-pgss.patchDownload
diff --git a/contrib/pg_stat_statements/pg_stat_statements--1.7--1.8.sql b/contrib/pg_stat_statements/pg_stat_statements--1.7--1.8.sql
index 0f63f08f7e..7fdef315ae 100644
--- a/contrib/pg_stat_statements/pg_stat_statements--1.7--1.8.sql
+++ b/contrib/pg_stat_statements/pg_stat_statements--1.7--1.8.sql
@@ -44,7 +44,8 @@ CREATE FUNCTION pg_stat_statements(IN showtext boolean,
OUT blk_write_time float8,
OUT wal_records int8,
OUT wal_fpi int8,
- OUT wal_bytes numeric
+ OUT wal_bytes numeric,
+ OUT generic_calls int8
)
RETURNS SETOF record
AS 'MODULE_PATHNAME', 'pg_stat_statements_1_8'
diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index 62cccbfa44..f5801016d6 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -77,10 +77,12 @@
#include "storage/fd.h"
#include "storage/ipc.h"
#include "storage/spin.h"
+#include "tcop/pquery.h"
#include "tcop/utility.h"
#include "utils/acl.h"
#include "utils/builtins.h"
#include "utils/memutils.h"
+#include "utils/plancache.h"
#include "utils/timestamp.h"
PG_MODULE_MAGIC;
@@ -192,6 +194,7 @@ typedef struct Counters
int64 wal_records; /* # of WAL records generated */
int64 wal_fpi; /* # of WAL full page images generated */
uint64 wal_bytes; /* total amount of WAL generated in bytes */
+ int64 generic_calls; /* # of times generic plans executed */
} Counters;
/*
@@ -277,6 +280,10 @@ static int exec_nested_level = 0;
/* Current nesting depth of planner calls */
static int plan_nested_level = 0;
+/* Current and previous plan type */
+static bool is_plan_type_generic = false;
+static bool is_prev_plan_type_generic = false;
+
/* Saved hook values in case of unload */
static shmem_startup_hook_type prev_shmem_startup_hook = NULL;
static post_parse_analyze_hook_type prev_post_parse_analyze_hook = NULL;
@@ -1034,6 +1041,23 @@ pgss_ExecutorStart(QueryDesc *queryDesc, int eflags)
*/
if (pgss_enabled(exec_nested_level) && queryDesc->plannedstmt->queryId != UINT64CONST(0))
{
+ /*
+ * Since ActivePortal is not available at ExecutorEnd, we preserve
+ * the current and previous plan type here.
+ * Previous one is necessary since portals can be nested.
+ */
+ Assert(ActivePortal);
+
+ is_prev_plan_type_generic = is_plan_type_generic;
+
+ if (ActivePortal->cplan)
+ {
+ if (ActivePortal->cplan->is_generic)
+ is_plan_type_generic = true;
+ else
+ is_plan_type_generic = false;
+ }
+
/*
* Set up to track total elapsed time in ExecutorRun. Make sure the
* space is allocated in the per-query context so it will go away at
@@ -1122,6 +1146,9 @@ pgss_ExecutorEnd(QueryDesc *queryDesc)
NULL);
}
+ /* Storing has done. Set is_plan_type_generic back to the original. */
+ is_plan_type_generic = is_prev_plan_type_generic;
+
if (prev_ExecutorEnd)
prev_ExecutorEnd(queryDesc);
else
@@ -1427,6 +1454,8 @@ pgss_store(const char *query, uint64 queryId,
e->counters.max_time[kind] = total_time;
e->counters.mean_time[kind] = total_time;
}
+ else if (kind == PGSS_EXEC && is_plan_type_generic)
+ e->counters.generic_calls += 1;
else
{
/*
@@ -1510,8 +1539,8 @@ pg_stat_statements_reset(PG_FUNCTION_ARGS)
#define PG_STAT_STATEMENTS_COLS_V1_1 18
#define PG_STAT_STATEMENTS_COLS_V1_2 19
#define PG_STAT_STATEMENTS_COLS_V1_3 23
-#define PG_STAT_STATEMENTS_COLS_V1_8 32
-#define PG_STAT_STATEMENTS_COLS 32 /* maximum of above */
+#define PG_STAT_STATEMENTS_COLS_V1_8 33
+#define PG_STAT_STATEMENTS_COLS 33 /* maximum of above */
/*
* Retrieve statement statistics.
@@ -1863,6 +1892,7 @@ pg_stat_statements_internal(FunctionCallInfo fcinfo,
ObjectIdGetDatum(0),
Int32GetDatum(-1));
values[i++] = wal_bytes;
+ values[i++] = Int64GetDatumFast(tmp.generic_calls);
}
Assert(i == (api_version == PGSS_V1_0 ? PG_STAT_STATEMENTS_COLS_V1_0 :
diff --git a/doc/src/sgml/pgstatstatements.sgml b/doc/src/sgml/pgstatstatements.sgml
index 464bf0e5ae..8c5e304882 100644
--- a/doc/src/sgml/pgstatstatements.sgml
+++ b/doc/src/sgml/pgstatstatements.sgml
@@ -363,6 +363,14 @@
Total amount of WAL generated by the statement in bytes
</para></entry>
</row>
+ <row>
+ <entry role="catalog_table_entry"><para role="column_definition">
+ <structfield>generic_calls</structfield> <type>bigint</type>
+ </para>
+ <para>
+ Number of times the statement was executed as generic plan
+ </para></entry>
+ </row>
</tbody>
</tgroup>
</table>
diff --git a/src/backend/utils/cache/plancache.c b/src/backend/utils/cache/plancache.c
index 1a0950489d..b25c63ddfe 100644
--- a/src/backend/utils/cache/plancache.c
+++ b/src/backend/utils/cache/plancache.c
@@ -1219,10 +1219,12 @@ GetCachedPlan(CachedPlanSource *plansource, ParamListInfo boundParams,
plansource->total_custom_cost += cached_plan_cost(plan, true);
plansource->num_custom_plans++;
+ plan->is_generic = false;
}
else
{
plansource->num_generic_plans++;
+ plan->is_generic = true;
}
Assert(plan != NULL);
diff --git a/src/include/utils/plancache.h b/src/include/utils/plancache.h
index ff09c63a02..df9934d89b 100644
--- a/src/include/utils/plancache.h
+++ b/src/include/utils/plancache.h
@@ -158,6 +158,7 @@ typedef struct CachedPlan
int generation; /* parent's generation number for this plan */
int refcount; /* count of live references to this struct */
MemoryContext context; /* context containing this CachedPlan */
+ bool is_generic; /* is this plan generic or not? */
} CachedPlan;
/*
On 2021/01/28 8:11, Tatsuro Yamada wrote:
Hi Toricoshi-san,
On 2021/01/12 20:36, torikoshia wrote:
I suppose it would be normal practice to store past results of
pg_stat_statements for future comparisons.
If this is the case, I think that if we only add the number of
generic plan execution, it will give us a hint to notice the cause
of performance degradation due to changes in the plan between
generic and custom.For example, if there is a clear difference in the number of times
the generic plan is executed between before and after performance
degradation as below, it would be natural to check if there is a
problem with the generic plan....
Attached a patch that just adds a generic call counter to
pg_stat_statements.I think that I'd like to use the view when we faced a performance
problem and find the reason. If we did the fixed-point observation
(should I say time-series analysis?) of generic_calls, it allows us to
realize the counter changes, and we can know whether the suspect is
generic_plan or not. So the patch helps DBA, I believe.
In that use case maybe what you actually want to see is whether the plan was
changed or not, rather than whether generic plan or custom plan is used?
If so, it's better to expose seq_scan (num of sequential scans processed by
the query) and idx_scan (num of index scans processed by the query) like
pg_stat_all_tables, per query in pg_stat_statements?
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
On 2021/02/08 14:02, torikoshia wrote:
On 2021-02-04 11:19, Kyotaro Horiguchi wrote:
At Thu, 04 Feb 2021 10:16:47 +0900, torikoshia
<torikoshia@oss.nttdata.com> wrote inChengxi Sun, Yamada-san, Horiguchi-san,
Thanks for all your comments.
Adding only the number of generic plan execution seems acceptable.On Mon, Jan 25, 2021 at 2:10 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:Note that ActivePortal is the closest nested portal. So it gives the
wrong result for nested portals.I may be wrong, but I thought it was ok since the closest nested
portal is the portal to be executed.After executing the inner-most portal, is_plan_type_generic has a
value for the inner-most portal and it won't be changed ever after. At
the ExecutorEnd of all the upper-portals see the value for the
inner-most portal left behind is_plan_type_generic nevertheless the
portals at every nest level are independent.ActivePortal is used in ExecutorStart hook in the patch.
And as far as I read PortalStart(), ActivePortal is changed to the
portal to be executed before ExecutorStart().If possible, could you tell me the specific case which causes wrong
results?Running a plpgsql function that does PREPRE in a query that does
PREPARE?Thanks for your explanation!
I confirmed that it in fact happened.
To avoid it, attached patch preserves the is_plan_type_generic before changing it and sets it back at the end of pgss_ExecutorEnd().
Any thoughts?
I just tried this feature. When I set plan_cache_mode to force_generic_plan
and executed the following queries, I found that pg_stat_statements.generic_calls
and pg_prepared_statements.generic_plans were not the same.
Is this behavior expected? I was thinking that they are basically the same.
DEALLOCATE ALL;
SELECT pg_stat_statements_reset();
PREPARE hoge AS SELECT * FROM pgbench_accounts WHERE aid = $1;
EXECUTE hoge(1);
EXECUTE hoge(1);
EXECUTE hoge(1);
SELECT generic_plans, statement FROM pg_prepared_statements WHERE statement LIKE '%hoge%';
generic_plans | statement
---------------+----------------------------------------------------------------
3 | PREPARE hoge AS SELECT * FROM pgbench_accounts WHERE aid = $1;
SELECT calls, generic_calls, query FROM pg_stat_statements WHERE query LIKE '%hoge%';
calls | generic_calls | query
-------+---------------+---------------------------------------------------------------
3 | 2 | PREPARE hoge AS SELECT * FROM pgbench_accounts WHERE aid = $1
When I executed the prepared statements via EXPLAIN ANALYZE, I found
pg_stat_statements.generic_calls was not incremented. Is this behavior expected?
Or we should count generic_calls even when executing the queries via ProcessUtility()?
DEALLOCATE ALL;
SELECT pg_stat_statements_reset();
PREPARE hoge AS SELECT * FROM pgbench_accounts WHERE aid = $1;
EXPLAIN ANALYZE EXECUTE hoge(1);
EXPLAIN ANALYZE EXECUTE hoge(1);
EXPLAIN ANALYZE EXECUTE hoge(1);
SELECT generic_plans, statement FROM pg_prepared_statements WHERE statement LIKE '%hoge%';
generic_plans | statement
---------------+----------------------------------------------------------------
3 | PREPARE hoge AS SELECT * FROM pgbench_accounts WHERE aid = $1;
SELECT calls, generic_calls, query FROM pg_stat_statements WHERE query LIKE '%hoge%';
calls | generic_calls | query
-------+---------------+---------------------------------------------------------------
3 | 0 | PREPARE hoge AS SELECT * FROM pgbench_accounts WHERE aid = $1
3 | 0 | EXPLAIN ANALYZE EXECUTE hoge(1)
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
On 2021-03-05 17:47, Fujii Masao wrote:
Thanks for your comments!
I just tried this feature. When I set plan_cache_mode to
force_generic_plan
and executed the following queries, I found that
pg_stat_statements.generic_calls
and pg_prepared_statements.generic_plans were not the same.
Is this behavior expected? I was thinking that they are basically the
same.
It's not expected behavior, fixed.
DEALLOCATE ALL;
SELECT pg_stat_statements_reset();
PREPARE hoge AS SELECT * FROM pgbench_accounts WHERE aid = $1;
EXECUTE hoge(1);
EXECUTE hoge(1);
EXECUTE hoge(1);SELECT generic_plans, statement FROM pg_prepared_statements WHERE
statement LIKE '%hoge%';
generic_plans | statement
---------------+----------------------------------------------------------------
3 | PREPARE hoge AS SELECT * FROM pgbench_accounts WHERE
aid = $1;SELECT calls, generic_calls, query FROM pg_stat_statements WHERE query
LIKE '%hoge%';
calls | generic_calls | query
-------+---------------+---------------------------------------------------------------
3 | 2 | PREPARE hoge AS SELECT * FROM
pgbench_accounts WHERE aid = $1When I executed the prepared statements via EXPLAIN ANALYZE, I found
pg_stat_statements.generic_calls was not incremented. Is this behavior
expected?
Or we should count generic_calls even when executing the queries via
ProcessUtility()?
I think prepared statements via EXPLAIN ANALYZE also should be counted
for consistency with pg_prepared_statements.
Since ActivePortal did not keep the plan type in the
ProcessUtility_hook,
I moved the global variables 'is_plan_type_generic' and
'is_prev_plan_type_generic' from pg_stat_statements to plancache.c.
DEALLOCATE ALL;
SELECT pg_stat_statements_reset();
PREPARE hoge AS SELECT * FROM pgbench_accounts WHERE aid = $1;
EXPLAIN ANALYZE EXECUTE hoge(1);
EXPLAIN ANALYZE EXECUTE hoge(1);
EXPLAIN ANALYZE EXECUTE hoge(1);SELECT generic_plans, statement FROM pg_prepared_statements WHERE
statement LIKE '%hoge%';
generic_plans | statement
---------------+----------------------------------------------------------------
3 | PREPARE hoge AS SELECT * FROM pgbench_accounts WHERE
aid = $1;SELECT calls, generic_calls, query FROM pg_stat_statements WHERE query
LIKE '%hoge%';
calls | generic_calls | query
-------+---------------+---------------------------------------------------------------
3 | 0 | PREPARE hoge AS SELECT * FROM
pgbench_accounts WHERE aid = $1
3 | 0 | EXPLAIN ANALYZE EXECUTE hoge(1)
Regards,
Attachments:
v6-0001-add-plan-type-to-pgss.patchtext/x-diff; name=v6-0001-add-plan-type-to-pgss.patchDownload
diff --git a/contrib/pg_stat_statements/expected/pg_stat_statements.out b/contrib/pg_stat_statements/expected/pg_stat_statements.out
index 16158525ca..887c4b2be8 100644
--- a/contrib/pg_stat_statements/expected/pg_stat_statements.out
+++ b/contrib/pg_stat_statements/expected/pg_stat_statements.out
@@ -251,6 +251,72 @@ FROM pg_stat_statements ORDER BY query COLLATE "C";
UPDATE pgss_test SET b = $1 WHERE a > $2 | 1 | 3 | t | t | t
(7 rows)
+--
+-- Track the number of generic plan
+--
+CREATE TABLE pgss_test (i int, j int, k int);
+SELECT pg_stat_statements_reset();
+ pg_stat_statements_reset
+--------------------------
+
+(1 row)
+
+SET plan_cache_mode TO force_generic_plan;
+SET pg_stat_statements.track_utility = TRUE;
+PREPARE pgss_p1 AS SELECT i FROM pgss_test WHERE i = $1;
+EXECUTE pgss_p1(1);
+ i
+---
+(0 rows)
+
+-- EXPLAIN ANALZE should be recorded
+PREPARE pgss_p2 AS SELECT j FROM pgss_test WHERE j = $1;
+EXPLAIN (ANALYZE, COSTS OFF, SUMMARY OFF, TIMING OFF) EXECUTE pgss_p2(1);
+ QUERY PLAN
+-----------------------------------------------
+ Seq Scan on pgss_test (actual rows=0 loops=1)
+ Filter: (j = $1)
+(2 rows)
+
+-- Nested Portal
+PREPARE pgss_p3 AS SELECT k FROM pgss_test WHERE k = $1;
+BEGIN;
+DECLARE pgss_c1 CURSOR FOR SELECT name FROM pg_prepared_statements;
+FETCH IN pgss_c1;
+ name
+---------
+ pgss_p2
+(1 row)
+
+EXECUTE pgss_p3(1);
+ k
+---
+(0 rows)
+
+FETCH IN pgss_c1;
+ name
+---------
+ pgss_p1
+(1 row)
+
+COMMIT;
+SELECT calls, generic_calls, query FROM pg_stat_statements;
+ calls | generic_calls | query
+-------+---------------+--------------------------------------------------------------------------
+ 1 | 0 | DECLARE pgss_c1 CURSOR FOR SELECT name FROM pg_prepared_statements
+ 0 | 0 | SELECT calls, generic_calls, query FROM pg_stat_statements
+ 1 | 1 | PREPARE pgss_p1 AS SELECT i FROM pgss_test WHERE i = $1
+ 2 | 0 | FETCH IN pgss_c1
+ 1 | 0 | BEGIN
+ 1 | 0 | SELECT pg_stat_statements_reset()
+ 1 | 1 | EXPLAIN (ANALYZE, COSTS OFF, SUMMARY OFF, TIMING OFF) EXECUTE pgss_p2(1)
+ 1 | 0 | COMMIT
+ 1 | 1 | PREPARE pgss_p3 AS SELECT k FROM pgss_test WHERE k = $1
+(9 rows)
+
+SET pg_stat_statements.track_utility = FALSE;
+DEALLOCATE ALL;
+DROP TABLE pgss_test;
--
-- pg_stat_statements.track = none
--
diff --git a/contrib/pg_stat_statements/pg_stat_statements--1.7--1.8.sql b/contrib/pg_stat_statements/pg_stat_statements--1.7--1.8.sql
index 0f63f08f7e..7fdef315ae 100644
--- a/contrib/pg_stat_statements/pg_stat_statements--1.7--1.8.sql
+++ b/contrib/pg_stat_statements/pg_stat_statements--1.7--1.8.sql
@@ -44,7 +44,8 @@ CREATE FUNCTION pg_stat_statements(IN showtext boolean,
OUT blk_write_time float8,
OUT wal_records int8,
OUT wal_fpi int8,
- OUT wal_bytes numeric
+ OUT wal_bytes numeric,
+ OUT generic_calls int8
)
RETURNS SETOF record
AS 'MODULE_PATHNAME', 'pg_stat_statements_1_8'
diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index 62cccbfa44..b14919c989 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -81,6 +81,7 @@
#include "utils/acl.h"
#include "utils/builtins.h"
#include "utils/memutils.h"
+#include "utils/plancache.h"
#include "utils/timestamp.h"
PG_MODULE_MAGIC;
@@ -192,6 +193,7 @@ typedef struct Counters
int64 wal_records; /* # of WAL records generated */
int64 wal_fpi; /* # of WAL full page images generated */
uint64 wal_bytes; /* total amount of WAL generated in bytes */
+ int64 generic_calls; /* # of times generic plans executed */
} Counters;
/*
@@ -1446,6 +1448,10 @@ pgss_store(const char *query, uint64 queryId,
if (e->counters.max_time[kind] < total_time)
e->counters.max_time[kind] = total_time;
}
+
+ if (kind == PGSS_EXEC && is_plan_type_generic)
+ e->counters.generic_calls += 1;
+
e->counters.rows += rows;
e->counters.shared_blks_hit += bufusage->shared_blks_hit;
e->counters.shared_blks_read += bufusage->shared_blks_read;
@@ -1510,8 +1516,8 @@ pg_stat_statements_reset(PG_FUNCTION_ARGS)
#define PG_STAT_STATEMENTS_COLS_V1_1 18
#define PG_STAT_STATEMENTS_COLS_V1_2 19
#define PG_STAT_STATEMENTS_COLS_V1_3 23
-#define PG_STAT_STATEMENTS_COLS_V1_8 32
-#define PG_STAT_STATEMENTS_COLS 32 /* maximum of above */
+#define PG_STAT_STATEMENTS_COLS_V1_8 33
+#define PG_STAT_STATEMENTS_COLS 33 /* maximum of above */
/*
* Retrieve statement statistics.
@@ -1863,6 +1869,7 @@ pg_stat_statements_internal(FunctionCallInfo fcinfo,
ObjectIdGetDatum(0),
Int32GetDatum(-1));
values[i++] = wal_bytes;
+ values[i++] = Int64GetDatumFast(tmp.generic_calls);
}
Assert(i == (api_version == PGSS_V1_0 ? PG_STAT_STATEMENTS_COLS_V1_0 :
diff --git a/contrib/pg_stat_statements/sql/pg_stat_statements.sql b/contrib/pg_stat_statements/sql/pg_stat_statements.sql
index 6f58d9d0f6..06f34d8450 100644
--- a/contrib/pg_stat_statements/sql/pg_stat_statements.sql
+++ b/contrib/pg_stat_statements/sql/pg_stat_statements.sql
@@ -125,6 +125,39 @@ wal_records > 0 as wal_records_generated,
wal_records = rows as wal_records_as_rows
FROM pg_stat_statements ORDER BY query COLLATE "C";
+--
+-- Track the number of generic plan
+--
+CREATE TABLE pgss_test (i int, j int, k int);
+SELECT pg_stat_statements_reset();
+SET plan_cache_mode TO force_generic_plan;
+SET pg_stat_statements.track_utility = TRUE;
+
+PREPARE pgss_p1 AS SELECT i FROM pgss_test WHERE i = $1;
+EXECUTE pgss_p1(1);
+
+-- EXPLAIN ANALZE should be recorded
+PREPARE pgss_p2 AS SELECT j FROM pgss_test WHERE j = $1;
+EXPLAIN (ANALYZE, COSTS OFF, SUMMARY OFF, TIMING OFF) EXECUTE pgss_p2(1);
+
+-- Nested Portal
+PREPARE pgss_p3 AS SELECT k FROM pgss_test WHERE k = $1;
+
+BEGIN;
+DECLARE pgss_c1 CURSOR FOR SELECT name FROM pg_prepared_statements;
+
+FETCH IN pgss_c1;
+EXECUTE pgss_p3(1);
+FETCH IN pgss_c1;
+
+COMMIT;
+
+SELECT calls, generic_calls, query FROM pg_stat_statements;
+
+SET pg_stat_statements.track_utility = FALSE;
+DEALLOCATE ALL;
+DROP TABLE pgss_test;
+
--
-- pg_stat_statements.track = none
--
diff --git a/doc/src/sgml/pgstatstatements.sgml b/doc/src/sgml/pgstatstatements.sgml
index 464bf0e5ae..8c5e304882 100644
--- a/doc/src/sgml/pgstatstatements.sgml
+++ b/doc/src/sgml/pgstatstatements.sgml
@@ -363,6 +363,14 @@
Total amount of WAL generated by the statement in bytes
</para></entry>
</row>
+ <row>
+ <entry role="catalog_table_entry"><para role="column_definition">
+ <structfield>generic_calls</structfield> <type>bigint</type>
+ </para>
+ <para>
+ Number of times the statement was executed as generic plan
+ </para></entry>
+ </row>
</tbody>
</tgroup>
</table>
diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index 0648dd82ba..159f07d536 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -61,6 +61,7 @@
#include "utils/lsyscache.h"
#include "utils/memutils.h"
#include "utils/partcache.h"
+#include "utils/plancache.h"
#include "utils/rls.h"
#include "utils/ruleutils.h"
#include "utils/snapmgr.h"
diff --git a/src/backend/utils/cache/plancache.c b/src/backend/utils/cache/plancache.c
index c1f4128445..68535a71bc 100644
--- a/src/backend/utils/cache/plancache.c
+++ b/src/backend/utils/cache/plancache.c
@@ -83,6 +83,10 @@
((plansource)->raw_parse_tree && \
IsA((plansource)->raw_parse_tree->stmt, TransactionStmt))
+/* Set whether the current and previous plan is generic or not */
+bool is_plan_type_generic = false;
+bool is_prev_plan_type_generic = false;
+
/*
* This is the head of the backend's list of "saved" CachedPlanSources (i.e.,
* those that are in long-lived storage and are examined for sinval events).
@@ -1219,10 +1223,15 @@ GetCachedPlan(CachedPlanSource *plansource, ParamListInfo boundParams,
plansource->total_custom_cost += cached_plan_cost(plan, true);
plansource->num_custom_plans++;
+ is_prev_plan_type_generic = is_plan_type_generic;
+ is_plan_type_generic = false;
+
}
else
{
plansource->num_generic_plans++;
+ is_prev_plan_type_generic = is_plan_type_generic;
+ is_plan_type_generic = true;
}
Assert(plan != NULL);
diff --git a/src/backend/utils/mmgr/portalmem.c b/src/backend/utils/mmgr/portalmem.c
index 66e3181815..040f1ca5d3 100644
--- a/src/backend/utils/mmgr/portalmem.c
+++ b/src/backend/utils/mmgr/portalmem.c
@@ -592,6 +592,9 @@ PortalDrop(Portal portal, bool isTopCommit)
/* release portal struct (it's in TopPortalContext) */
pfree(portal);
+
+ /* Set is_plan_type_generic back to the original value. */
+ is_plan_type_generic = is_prev_plan_type_generic;
}
/*
diff --git a/src/include/utils/plancache.h b/src/include/utils/plancache.h
index ff09c63a02..1145d250cb 100644
--- a/src/include/utils/plancache.h
+++ b/src/include/utils/plancache.h
@@ -34,6 +34,10 @@ typedef enum
PLAN_CACHE_MODE_FORCE_CUSTOM_PLAN
} PlanCacheMode;
+/* Set whether the current and previous plan is generic or not */
+extern bool is_plan_type_generic;
+extern bool is_prev_plan_type_generic;
+
/* GUC parameter */
extern int plan_cache_mode;
@@ -158,6 +162,7 @@ typedef struct CachedPlan
int generation; /* parent's generation number for this plan */
int refcount; /* count of live references to this struct */
MemoryContext context; /* context containing this CachedPlan */
+ bool is_generic; /* is this plan generic or not? */
} CachedPlan;
/*
On 2021/03/23 16:32, torikoshia wrote:
On 2021-03-05 17:47, Fujii Masao wrote:
Thanks for your comments!
Thanks for updating the patch!
PostgreSQL Patch Tester reported that the patched version failed to be compiled
at Windows. Could you fix this issue?
https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.131238
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
On 2021-03-25 22:14, Fujii Masao wrote:
On 2021/03/23 16:32, torikoshia wrote:
On 2021-03-05 17:47, Fujii Masao wrote:
Thanks for your comments!
Thanks for updating the patch!
PostgreSQL Patch Tester reported that the patched version failed to be
compiled
at Windows. Could you fix this issue?
https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.131238
It seems PGDLLIMPORT was necessary..
Attached a new one.
Regards.
Attachments:
v7-0001-add-plan-type-to-pgss.patchtext/x-diff; name=v7-0001-add-plan-type-to-pgss.patchDownload
diff --git a/contrib/pg_stat_statements/expected/pg_stat_statements.out b/contrib/pg_stat_statements/expected/pg_stat_statements.out
index 16158525ca..887c4b2be8 100644
--- a/contrib/pg_stat_statements/expected/pg_stat_statements.out
+++ b/contrib/pg_stat_statements/expected/pg_stat_statements.out
@@ -251,6 +251,72 @@ FROM pg_stat_statements ORDER BY query COLLATE "C";
UPDATE pgss_test SET b = $1 WHERE a > $2 | 1 | 3 | t | t | t
(7 rows)
+--
+-- Track the number of generic plan
+--
+CREATE TABLE pgss_test (i int, j int, k int);
+SELECT pg_stat_statements_reset();
+ pg_stat_statements_reset
+--------------------------
+
+(1 row)
+
+SET plan_cache_mode TO force_generic_plan;
+SET pg_stat_statements.track_utility = TRUE;
+PREPARE pgss_p1 AS SELECT i FROM pgss_test WHERE i = $1;
+EXECUTE pgss_p1(1);
+ i
+---
+(0 rows)
+
+-- EXPLAIN ANALZE should be recorded
+PREPARE pgss_p2 AS SELECT j FROM pgss_test WHERE j = $1;
+EXPLAIN (ANALYZE, COSTS OFF, SUMMARY OFF, TIMING OFF) EXECUTE pgss_p2(1);
+ QUERY PLAN
+-----------------------------------------------
+ Seq Scan on pgss_test (actual rows=0 loops=1)
+ Filter: (j = $1)
+(2 rows)
+
+-- Nested Portal
+PREPARE pgss_p3 AS SELECT k FROM pgss_test WHERE k = $1;
+BEGIN;
+DECLARE pgss_c1 CURSOR FOR SELECT name FROM pg_prepared_statements;
+FETCH IN pgss_c1;
+ name
+---------
+ pgss_p2
+(1 row)
+
+EXECUTE pgss_p3(1);
+ k
+---
+(0 rows)
+
+FETCH IN pgss_c1;
+ name
+---------
+ pgss_p1
+(1 row)
+
+COMMIT;
+SELECT calls, generic_calls, query FROM pg_stat_statements;
+ calls | generic_calls | query
+-------+---------------+--------------------------------------------------------------------------
+ 1 | 0 | DECLARE pgss_c1 CURSOR FOR SELECT name FROM pg_prepared_statements
+ 0 | 0 | SELECT calls, generic_calls, query FROM pg_stat_statements
+ 1 | 1 | PREPARE pgss_p1 AS SELECT i FROM pgss_test WHERE i = $1
+ 2 | 0 | FETCH IN pgss_c1
+ 1 | 0 | BEGIN
+ 1 | 0 | SELECT pg_stat_statements_reset()
+ 1 | 1 | EXPLAIN (ANALYZE, COSTS OFF, SUMMARY OFF, TIMING OFF) EXECUTE pgss_p2(1)
+ 1 | 0 | COMMIT
+ 1 | 1 | PREPARE pgss_p3 AS SELECT k FROM pgss_test WHERE k = $1
+(9 rows)
+
+SET pg_stat_statements.track_utility = FALSE;
+DEALLOCATE ALL;
+DROP TABLE pgss_test;
--
-- pg_stat_statements.track = none
--
diff --git a/contrib/pg_stat_statements/pg_stat_statements--1.7--1.8.sql b/contrib/pg_stat_statements/pg_stat_statements--1.7--1.8.sql
index 0f63f08f7e..7fdef315ae 100644
--- a/contrib/pg_stat_statements/pg_stat_statements--1.7--1.8.sql
+++ b/contrib/pg_stat_statements/pg_stat_statements--1.7--1.8.sql
@@ -44,7 +44,8 @@ CREATE FUNCTION pg_stat_statements(IN showtext boolean,
OUT blk_write_time float8,
OUT wal_records int8,
OUT wal_fpi int8,
- OUT wal_bytes numeric
+ OUT wal_bytes numeric,
+ OUT generic_calls int8
)
RETURNS SETOF record
AS 'MODULE_PATHNAME', 'pg_stat_statements_1_8'
diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index 62cccbfa44..b14919c989 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -81,6 +81,7 @@
#include "utils/acl.h"
#include "utils/builtins.h"
#include "utils/memutils.h"
+#include "utils/plancache.h"
#include "utils/timestamp.h"
PG_MODULE_MAGIC;
@@ -192,6 +193,7 @@ typedef struct Counters
int64 wal_records; /* # of WAL records generated */
int64 wal_fpi; /* # of WAL full page images generated */
uint64 wal_bytes; /* total amount of WAL generated in bytes */
+ int64 generic_calls; /* # of times generic plans executed */
} Counters;
/*
@@ -1446,6 +1448,10 @@ pgss_store(const char *query, uint64 queryId,
if (e->counters.max_time[kind] < total_time)
e->counters.max_time[kind] = total_time;
}
+
+ if (kind == PGSS_EXEC && is_plan_type_generic)
+ e->counters.generic_calls += 1;
+
e->counters.rows += rows;
e->counters.shared_blks_hit += bufusage->shared_blks_hit;
e->counters.shared_blks_read += bufusage->shared_blks_read;
@@ -1510,8 +1516,8 @@ pg_stat_statements_reset(PG_FUNCTION_ARGS)
#define PG_STAT_STATEMENTS_COLS_V1_1 18
#define PG_STAT_STATEMENTS_COLS_V1_2 19
#define PG_STAT_STATEMENTS_COLS_V1_3 23
-#define PG_STAT_STATEMENTS_COLS_V1_8 32
-#define PG_STAT_STATEMENTS_COLS 32 /* maximum of above */
+#define PG_STAT_STATEMENTS_COLS_V1_8 33
+#define PG_STAT_STATEMENTS_COLS 33 /* maximum of above */
/*
* Retrieve statement statistics.
@@ -1863,6 +1869,7 @@ pg_stat_statements_internal(FunctionCallInfo fcinfo,
ObjectIdGetDatum(0),
Int32GetDatum(-1));
values[i++] = wal_bytes;
+ values[i++] = Int64GetDatumFast(tmp.generic_calls);
}
Assert(i == (api_version == PGSS_V1_0 ? PG_STAT_STATEMENTS_COLS_V1_0 :
diff --git a/contrib/pg_stat_statements/sql/pg_stat_statements.sql b/contrib/pg_stat_statements/sql/pg_stat_statements.sql
index 6f58d9d0f6..06f34d8450 100644
--- a/contrib/pg_stat_statements/sql/pg_stat_statements.sql
+++ b/contrib/pg_stat_statements/sql/pg_stat_statements.sql
@@ -125,6 +125,39 @@ wal_records > 0 as wal_records_generated,
wal_records = rows as wal_records_as_rows
FROM pg_stat_statements ORDER BY query COLLATE "C";
+--
+-- Track the number of generic plan
+--
+CREATE TABLE pgss_test (i int, j int, k int);
+SELECT pg_stat_statements_reset();
+SET plan_cache_mode TO force_generic_plan;
+SET pg_stat_statements.track_utility = TRUE;
+
+PREPARE pgss_p1 AS SELECT i FROM pgss_test WHERE i = $1;
+EXECUTE pgss_p1(1);
+
+-- EXPLAIN ANALZE should be recorded
+PREPARE pgss_p2 AS SELECT j FROM pgss_test WHERE j = $1;
+EXPLAIN (ANALYZE, COSTS OFF, SUMMARY OFF, TIMING OFF) EXECUTE pgss_p2(1);
+
+-- Nested Portal
+PREPARE pgss_p3 AS SELECT k FROM pgss_test WHERE k = $1;
+
+BEGIN;
+DECLARE pgss_c1 CURSOR FOR SELECT name FROM pg_prepared_statements;
+
+FETCH IN pgss_c1;
+EXECUTE pgss_p3(1);
+FETCH IN pgss_c1;
+
+COMMIT;
+
+SELECT calls, generic_calls, query FROM pg_stat_statements;
+
+SET pg_stat_statements.track_utility = FALSE;
+DEALLOCATE ALL;
+DROP TABLE pgss_test;
+
--
-- pg_stat_statements.track = none
--
diff --git a/doc/src/sgml/pgstatstatements.sgml b/doc/src/sgml/pgstatstatements.sgml
index 464bf0e5ae..8c5e304882 100644
--- a/doc/src/sgml/pgstatstatements.sgml
+++ b/doc/src/sgml/pgstatstatements.sgml
@@ -363,6 +363,14 @@
Total amount of WAL generated by the statement in bytes
</para></entry>
</row>
+ <row>
+ <entry role="catalog_table_entry"><para role="column_definition">
+ <structfield>generic_calls</structfield> <type>bigint</type>
+ </para>
+ <para>
+ Number of times the statement was executed as generic plan
+ </para></entry>
+ </row>
</tbody>
</tgroup>
</table>
diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index 8de78ada63..788e9509d7 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -61,6 +61,7 @@
#include "utils/lsyscache.h"
#include "utils/memutils.h"
#include "utils/partcache.h"
+#include "utils/plancache.h"
#include "utils/rls.h"
#include "utils/ruleutils.h"
#include "utils/snapmgr.h"
diff --git a/src/backend/utils/cache/plancache.c b/src/backend/utils/cache/plancache.c
index 1a0950489d..7efb3a2732 100644
--- a/src/backend/utils/cache/plancache.c
+++ b/src/backend/utils/cache/plancache.c
@@ -83,6 +83,10 @@
((plansource)->raw_parse_tree && \
IsA((plansource)->raw_parse_tree->stmt, TransactionStmt))
+/* Set whether the current and previous plan is generic or not */
+bool is_plan_type_generic = false;
+bool is_prev_plan_type_generic = false;
+
/*
* This is the head of the backend's list of "saved" CachedPlanSources (i.e.,
* those that are in long-lived storage and are examined for sinval events).
@@ -1219,10 +1223,15 @@ GetCachedPlan(CachedPlanSource *plansource, ParamListInfo boundParams,
plansource->total_custom_cost += cached_plan_cost(plan, true);
plansource->num_custom_plans++;
+ is_prev_plan_type_generic = is_plan_type_generic;
+ is_plan_type_generic = false;
+
}
else
{
plansource->num_generic_plans++;
+ is_prev_plan_type_generic = is_plan_type_generic;
+ is_plan_type_generic = true;
}
Assert(plan != NULL);
diff --git a/src/backend/utils/mmgr/portalmem.c b/src/backend/utils/mmgr/portalmem.c
index 66e3181815..040f1ca5d3 100644
--- a/src/backend/utils/mmgr/portalmem.c
+++ b/src/backend/utils/mmgr/portalmem.c
@@ -592,6 +592,9 @@ PortalDrop(Portal portal, bool isTopCommit)
/* release portal struct (it's in TopPortalContext) */
pfree(portal);
+
+ /* Set is_plan_type_generic back to the original value. */
+ is_plan_type_generic = is_prev_plan_type_generic;
}
/*
diff --git a/src/include/utils/plancache.h b/src/include/utils/plancache.h
index ff09c63a02..70421e8d5f 100644
--- a/src/include/utils/plancache.h
+++ b/src/include/utils/plancache.h
@@ -34,6 +34,10 @@ typedef enum
PLAN_CACHE_MODE_FORCE_CUSTOM_PLAN
} PlanCacheMode;
+/* Set whether the current and previous plan is generic or not */
+extern PGDLLIMPORT bool is_plan_type_generic;
+extern PGDLLIMPORT bool is_prev_plan_type_generic;
+
/* GUC parameter */
extern int plan_cache_mode;
@@ -158,6 +162,7 @@ typedef struct CachedPlan
int generation; /* parent's generation number for this plan */
int refcount; /* count of live references to this struct */
MemoryContext context; /* context containing this CachedPlan */
+ bool is_generic; /* is this plan generic or not? */
} CachedPlan;
/*
On 2021/03/26 0:33, torikoshia wrote:
On 2021-03-25 22:14, Fujii Masao wrote:
On 2021/03/23 16:32, torikoshia wrote:
On 2021-03-05 17:47, Fujii Masao wrote:
Thanks for your comments!
Thanks for updating the patch!
PostgreSQL Patch Tester reported that the patched version failed to be compiled
at Windows. Could you fix this issue?
https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.131238It seems PGDLLIMPORT was necessary..
Attached a new one.
Thanks for updating the patch!
In my test, generic_calls for a utility command was not incremented
before PL/pgSQL function was executed. Maybe this is expected behavior.
But it was incremented after the function was executed. Is this a bug?
Please see the following example.
-------------------------------------------
SELECT pg_stat_statements_reset();
SET enable_seqscan TO on;
SELECT calls, generic_calls, query FROM pg_stat_statements WHERE query LIKE '%seqscan%';
CREATE OR REPLACE FUNCTION do_ckpt() RETURNS VOID AS $$
BEGIN
EXECUTE 'CHECKPOINT';
END $$ LANGUAGE plpgsql;
SET enable_seqscan TO on;
SET enable_seqscan TO on;
-- SET commands were executed three times before do_ckpt() was called.
-- generic_calls for SET command is zero in this case.
SELECT calls, generic_calls, query FROM pg_stat_statements WHERE query LIKE '%seqscan%';
SELECT do_ckpt();
SET enable_seqscan TO on;
SET enable_seqscan TO on;
SET enable_seqscan TO on;
-- SET commands were executed additionally three times after do_ckpt() was called.
-- generic_calls for SET command is three in this case.
SELECT calls, generic_calls, query FROM pg_stat_statements WHERE query LIKE '%seqscan%';
-------------------------------------------
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
On 2021-03-26 17:46, Fujii Masao wrote:
On 2021/03/26 0:33, torikoshia wrote:
On 2021-03-25 22:14, Fujii Masao wrote:
On 2021/03/23 16:32, torikoshia wrote:
On 2021-03-05 17:47, Fujii Masao wrote:
Thanks for your comments!
Thanks for updating the patch!
PostgreSQL Patch Tester reported that the patched version failed to
be compiled
at Windows. Could you fix this issue?
https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.131238It seems PGDLLIMPORT was necessary..
Attached a new one.Thanks for updating the patch!
In my test, generic_calls for a utility command was not incremented
before PL/pgSQL function was executed. Maybe this is expected behavior.
But it was incremented after the function was executed. Is this a bug?
Please see the following example.
Thanks for reviewing!
It's a bug and regrettably it seems difficult to fix it during this
commitfest.
Marked the patch as "Withdrawn".
Regards,