Обсуждение: Is it useful to record whether plans are generic or custom?

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

> 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

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 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 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.

> 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


-- 
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 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 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
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
includingplanning 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

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 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 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.

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

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

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

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 choosen
> 
> Typo: "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

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 choosen
>>
>> Typo: "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 choosen
>>> 
>>> Typo: "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 choosen
>>>> 
>>>> Typo: "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

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 choosen
>>>>>
>>>>> Typo: "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 choosen
>>>>>> 
>>>>>> Typo: "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

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 choosen
>>>>>>>
>>>>>>> Typo: "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

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 choosen
>>>>>>>>
>>>>>>>> Typo: "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.

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 choosen
>>>>>>>>> 
>>>>>>>>> Typo: "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.
> 
> 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

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

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

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

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

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.


https://www.postgresql.org/message-id/flat/CAKU4AWq5_jx1Vyai0_Sumgn-Ks0R%2BN80cf%2Bt170%2BzQs8x6%3DHew%40mail.gmail.com#f57e64b8d37697c808e4385009340871


Any thoughts?


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

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

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
enoughto get the reason of performance change from pg_stat_statements. I mean if you need more detailed information
aboutthe plan and execution of prepared statements, it is better to store this information in a separate view. It seems
morereasonable 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 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 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

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 = $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()?

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,

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.

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.131238
>>
> 
> It 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.131238
>>> 
>> 
>> It 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,