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

On Thu, May 14, 2020 at 2:28 AM legrand legrand <legrand_legrand@hotmail.com>
wrote:

Hello,

yes this can be usefull, under the condition of differentiating all the
counters
for a queryid using a generic plan and the one using a custom one.

For me one way to do that is adding a generic_plan column to
pg_stat_statements key, someting like:
- userid,
- dbid,
- queryid,
- generic_plan

Thanks for your kind advice!

I don't know if generic/custom plan types are available during planning
and/or
execution hooks ...

Yeah, that's what I'm concerned about.

As far as I can see, there are no variables tracking executed
plan types so we may need to add variables in
CachedPlanSource or somewhere.

CachedPlanSource.num_custom_plans looked like what is needed,
but it is the number of PLANNING so it also increments when
the planner calculates both plans and decides to take generic
plan.

To track executed plan types, I think execution layer hooks
are appropriate.
These hooks, however, take QueryDesc as a param and it does
not include cached plan information.
Portal includes CachedPlanSource but there seem no hooks to
take Portal.

So I'm wondering it's necessary to add a hook to get Portal
or CachedPlanSource.
Are these too much change for getting plan types?

Regards,

--
Atsushi Torikoshi

To track executed plan types, I think execution layer hooks
are appropriate.
These hooks, however, take QueryDesc as a param and it does
not include cached plan information.

It seems that the same QueryDesc entry is reused when executing
a generic plan.
For exemple marking queryDesc->plannedstmt->queryId (outside
pg_stat_statements) with a pseudo tag during ExecutorStart
reappears in later executions with generic plans ...

Is this QueryDesc reusable by a custom plan ? If not maybe a solution
could be to add a flag in queryDesc->plannedstmt ?

(sorry, I haven't understood yet how and when this generic plan is
managed during planning)

Regards
PAscal

--
Sent from: https://www.postgresql-archive.org/PostgreSQL-hackers-f1928748.html

On Sat, May 16, 2020 at 6:01 PM legrand legrand <legrand_legrand@hotmail.com>
wrote:

To track executed plan types, I think execution layer hooks
are appropriate.
These hooks, however, take QueryDesc as a param and it does
not include cached plan information.

It seems that the same QueryDesc entry is reused when executing
a generic plan.
For exemple marking queryDesc->plannedstmt->queryId (outside
pg_stat_statements) with a pseudo tag during ExecutorStart
reappears in later executions with generic plans ...

Is this QueryDesc reusable by a custom plan ? If not maybe a solution
could be to add a flag in queryDesc->plannedstmt ?

Thanks for your proposal!

I first thought it was a good idea and tried to add a flag to QueryDesc,
but the comments on QueryDesc say it encapsulates everything that
the executor needs to execute a query.

Whether a plan is generic or custom is not what executor needs to
know for running queries, so now I hesitate to do so.

Instead, I'm now considering using a static hash for prepared queries
(static HTAB *prepared_queries).

BTW, I'd also appreciate other opinions about recording the number
of generic and custom plans on pg_stat_statemtents.

Regards,

--
Atsushi Torikoshi

At Tue, 19 May 2020 22:56:17 +0900, Atsushi Torikoshi <atorik@gmail.com> wrote in

On Sat, May 16, 2020 at 6:01 PM legrand legrand <legrand_legrand@hotmail.com>
wrote:

To track executed plan types, I think execution layer hooks
are appropriate.
These hooks, however, take QueryDesc as a param and it does
not include cached plan information.

It seems that the same QueryDesc entry is reused when executing
a generic plan.
For exemple marking queryDesc->plannedstmt->queryId (outside
pg_stat_statements) with a pseudo tag during ExecutorStart
reappears in later executions with generic plans ...

Is this QueryDesc reusable by a custom plan ? If not maybe a solution
could be to add a flag in queryDesc->plannedstmt ?

Thanks for your proposal!

I first thought it was a good idea and tried to add a flag to QueryDesc,
but the comments on QueryDesc say it encapsulates everything that
the executor needs to execute a query.

Whether a plan is generic or custom is not what executor needs to
know for running queries, so now I hesitate to do so.

Instead, I'm now considering using a static hash for prepared queries
(static HTAB *prepared_queries).

BTW, I'd also appreciate other opinions about recording the number
of generic and custom plans on pg_stat_statemtents.

If you/we just want to know how a prepared statement is executed,
couldn't we show that information in pg_prepared_statements view?

=# select * from pg_prepared_statements;
-[ RECORD 1 ]---+----------------------------------------------------
name | stmt1
statement | prepare stmt1 as select * from t where b = $1;
prepare_time | 2020-05-20 12:01:55.733469+09
parameter_types | {text}
from_sql | t
exec_custom | 5 <- existing num_custom_plans
exec_total | 40 <- new member of CachedPlanSource

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On Wed, May 20, 2020 at 1:32 PM Kyotaro Horiguchi <horikyota.ntt@gmail.com>
wrote:

At Tue, 19 May 2020 22:56:17 +0900, Atsushi Torikoshi <atorik@gmail.com>
wrote in

On Sat, May 16, 2020 at 6:01 PM legrand legrand <

legrand_legrand@hotmail.com>

wrote:

BTW, I'd also appreciate other opinions about recording the number
of generic and custom plans on pg_stat_statemtents.

If you/we just want to know how a prepared statement is executed,
couldn't we show that information in pg_prepared_statements view?

=# select * from pg_prepared_statements;
-[ RECORD 1 ]---+----------------------------------------------------
name | stmt1
statement | prepare stmt1 as select * from t where b = $1;
prepare_time | 2020-05-20 12:01:55.733469+09
parameter_types | {text}
from_sql | t
exec_custom | 5 <- existing num_custom_plans
exec_total | 40 <- new member of CachedPlanSource

Thanks, Horiguchi-san!

Adding counters to pg_prepared_statements seems useful when we want
to know the way prepared statements executed in the current session.

And I also feel adding counters to pg_stat_statements will be convenient
especially in production environments because it enables us to get
information about not only the current session but all sessions of a
PostgreSQL instance.

If both changes are worthwhile, considering implementation complexity,
it may be reasonable to firstly add columns to pg_prepared_statements
and then work on pg_stat_statements.

Regards,

--
Atsushi Torikoshi

On 2020/05/20 21:56, Atsushi Torikoshi wrote:

On Wed, May 20, 2020 at 1:32 PM Kyotaro Horiguchi <horikyota.ntt@gmail.com <mailto:horikyota.ntt@gmail.com>> wrote:

At Tue, 19 May 2020 22:56:17 +0900, Atsushi Torikoshi <atorik@gmail.com <mailto:atorik@gmail.com>> wrote in

On Sat, May 16, 2020 at 6:01 PM legrand legrand <legrand_legrand@hotmail.com <mailto:legrand_legrand@hotmail.com>>
wrote:

BTW, I'd also appreciate other opinions about recording the number
of generic and custom plans on pg_stat_statemtents.

If you/we just want to know how a prepared statement is executed,
couldn't we show that information in pg_prepared_statements view?

=# select * from pg_prepared_statements;
-[ RECORD 1 ]---+----------------------------------------------------
name            | stmt1
statement       | prepare stmt1 as select * from t where b = $1;
prepare_time    | 2020-05-20 12:01:55.733469+09
parameter_types | {text}
from_sql        | t
exec_custom     | 5    <- existing num_custom_plans
exec_total          | 40   <- new member of 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]https://github.com/MasaoFujii/pg_cheat_funcs#record-pg_cached_plan_sourcestmt-text for details.

And I also feel adding counters to pg_stat_statements will be convenient
especially in production environments because it enables us to get
information about not only the current session but all sessions of a
PostgreSQL instance.

+1

Regards,

[1]: https://github.com/MasaoFujii/pg_cheat_funcs#record-pg_cached_plan_sourcestmt-text
https://github.com/MasaoFujii/pg_cheat_funcs#record-pg_cached_plan_sourcestmt-text

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

At Thu, 21 May 2020 12:18:16 +0900, Fujii Masao <masao.fujii@oss.nttdata.com> wrote in

On 2020/05/20 21:56, Atsushi Torikoshi wrote:

On Wed, May 20, 2020 at 1:32 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com <mailto:horikyota.ntt@gmail.com>> wrote:
At Tue, 19 May 2020 22:56:17 +0900, Atsushi Torikoshi
<atorik@gmail.com <mailto:atorik@gmail.com>> wrote 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

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

Thanks for writing a patch!

On Thu, May 21, 2020 at 5:10 PM Kyotaro Horiguchi <horikyota.ntt@gmail.com>
wrote:

At Thu, 21 May 2020 12:18:16 +0900, Fujii Masao <
masao.fujii@oss.nttdata.com> wrote in

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?

I didn't come up with ideas about how to use them.
IMHO they might not so necessary..

=# select * from pg_prepared_statements;
-[ RECORD 1 ]---+--------------------------------------------
name | p1
statement | prepare p1 as select a from t where a = $1;
prepare_time | 2020-05-21 15:41:50.419578+09
parameter_types | {integer}
from_sql | t
calls | 7
custom_calls | 5
plan_generation | 6
generic_cost | 4.3100000000000005
custom_cost | 9.31

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'?

At Tue, 19 May 2020 22:56:17 +0900, Atsushi Torikoshi <atorik@gmail.com>

wrote in

Instead, I'm now considering using a static hash for prepared queries
(static HTAB *prepared_queries).

That might be complex and fragile considering nested query and SPI
calls. I'm not sure, but could we use ActivePortal?
ActivePortal->cplan is a CachedPlan, which can hold the generic/custom
information.

Yes, I once looked for hooks which can get Portal, I couldn't find them.
And it also seems difficult getting keys for HTAB *prepared_queries
in existing executor hooks.
There may be oversights, but I'm now feeling returning to the idea
hook additions.

| Portal includes CachedPlanSource but there seem no hooks to
| take Portal.
| So I'm wondering it's necessary to add a hook to get Portal
| or CachedPlanSource.
| Are these too much change for getting plan types?

On Thu, May 21, 2020 at 5:43 PM Tatsuro Yamada <
tatsuro.yamada.tf@nttcom.co.jp> wrote:

I tried to creating PoC patch too, so I share it.
Please find attached file.

Thanks!

I agree with your idea showing the latest plan is generic or custom.

This patch judges whether the lastest plan was generic based on
plansource->gplan existence, but plansource->gplan can exist even
when the planner chooses custom.
For example, a prepared statement was executed first 6 times and
a generic plan was generated for comparison but the custom plan
won.

Attached another patch showing latest plan based on
'0001-Expose-counters-of-plancache-to-pg_prepared_statemen.patch'.

As I wrote above, I suppose some of the columns might not necessary
and it'd better change some column and variable names, but I left them
for other opinions.

Regards,

--
Atsushi Torikoshi

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

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