Thread
Commits
-
Add generic_plans and custom_plans fields into pg_prepared_statements.
- d05b172a760e 14.0 landed
-
Is it useful to record whether plans are generic or custom?
Atsushi Torikoshi <atorik@gmail.com> — 2020-05-12T04:53:15Z
Hi, When we run prepared statements, as far as I know, running EXPLAIN is the only way to know whether executed plans are generic or custom. There seems no way to know how many times a prepared statement was executed as generic and custom. I think it may be useful to record the number of generic and custom plans mainly to ensure the prepared statements are executed as expected plan type. If people also feel it's useful, I'm going to think about adding columns such as 'generic plans' and 'custom plans' to pg_stat_statements. As you know, pg_stat_statements can now track not only 'calls' but 'plans', so we can presume which plan type was executed from them. When both 'calls' and 'plans' were incremented, plan type would be custom. When only 'calls' was incremented, it would be generic. But considering the case such as only the plan phase has succeeded and the execution phase has failed, this presumption can be wrong. Thoughts? Regards, -- Atsushi Torikoshi
-
Re: Is it useful to record whether plans are generic or custom?
legrand legrand <legrand_legrand@hotmail.com> — 2020-05-13T17:28:12Z
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
-
Re: Is it useful to record whether plans are generic or custom?
Atsushi Torikoshi <atorik@gmail.com> — 2020-05-15T08:47:41Z
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
-
Re: Is it useful to record whether plans are generic or custom?
legrand legrand <legrand_legrand@hotmail.com> — 2020-05-16T09:01:35Z
> 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
-
Re: Is it useful to record whether plans are generic or custom?
Atsushi Torikoshi <atorik@gmail.com> — 2020-05-19T13:56:17Z
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
-
Re: Is it useful to record whether plans are generic or custom?
Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2020-05-20T04:32:04Z
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 -
Re: Is it useful to record whether plans are generic or custom?
Atsushi Torikoshi <atorik@gmail.com> — 2020-05-20T12:56:04Z
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 -
Re: Is it useful to record whether plans are generic or custom?
Fujii Masao <masao.fujii@oss.nttdata.com> — 2020-05-21T03:18:16Z
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 -
Re: Is it useful to record whether plans are generic or custom?
Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2020-05-21T08:10:05Z
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 -
Re: Is it useful to record whether plans are generic or custom?
Tatsuro Yamada <tatsuro.yamada.tf@nttcom.co.jp> — 2020-05-21T08:43:01Z
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 -
Re: Is it useful to record whether plans are generic or custom?
Atsushi Torikoshi <atorik@gmail.com> — 2020-05-25T01:54:05Z
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 -
Re: Is it useful to record whether plans are generic or custom?
Atsushi Torikoshi <atorik@gmail.com> — 2020-06-04T08:04:36Z
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 > -
Re: Is it useful to record whether plans are generic or custom?
Masahiro Ikeda <ikedamsh@oss.nttdata.com> — 2020-06-08T11:45:15Z
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 -
Re: Is it useful to record whether plans are generic or custom?
torikoshia <torikoshia@oss.nttdata.com> — 2020-06-10T01:50:58Z
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
-
Re: Is it useful to record whether plans are generic or custom?
Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2020-06-10T09:00:43Z
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 -
Re: Is it useful to record whether plans are generic or custom?
torikoshia <torikoshia@oss.nttdata.com> — 2020-06-11T05:59:22Z
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 -
Re: Is it useful to record whether plans are generic or custom?
Fujii Masao <masao.fujii@oss.nttdata.com> — 2020-07-06T13:16:18Z
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 -
Re: Is it useful to record whether plans are generic or custom?
torikoshia <torikoshia@oss.nttdata.com> — 2020-07-08T01:14:42Z
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 -
Re: Is it useful to record whether plans are generic or custom?
Fujii Masao <masao.fujii@oss.nttdata.com> — 2020-07-08T07:41:31Z
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 -
Re: Is it useful to record whether plans are generic or custom?
torikoshia <torikoshia@oss.nttdata.com> — 2020-07-10T01:49:11Z
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 -
Re: Is it useful to record whether plans are generic or custom?
torikoshia <torikoshia@oss.nttdata.com> — 2020-07-14T12:24:25Z
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 -
Re: Is it useful to record whether plans are generic or custom?
Fujii Masao <masao.fujii@oss.nttdata.com> — 2020-07-15T02:44:22Z
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 -
Re: Is it useful to record whether plans are generic or custom?
torikoshia <torikoshia@oss.nttdata.com> — 2020-07-16T02:50:20Z
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 -
Re: Is it useful to record whether plans are generic or custom?
Fujii Masao <masao.fujii@oss.nttdata.com> — 2020-07-17T07:25:58Z
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 -
Re: Is it useful to record whether plans are generic or custom?
Fujii Masao <masao.fujii@oss.nttdata.com> — 2020-07-20T02:57:39Z
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 -
Re: Is it useful to record whether plans are generic or custom?
torikoshia <torikoshia@oss.nttdata.com> — 2020-07-20T04:57:42Z
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 -
Re: Is it useful to record whether plans are generic or custom?
torikoshia <torikoshia@oss.nttdata.com> — 2020-07-22T07:49:53Z
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
-
Re: Is it useful to record whether plans are generic or custom?
Fujii Masao <masao.fujii@oss.nttdata.com> — 2020-07-30T05:31:47Z
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
-
RE: Is it useful to record whether plans are generic or custom?
legrand legrand <legrand_legrand@hotmail.com> — 2020-07-30T07:34:35Z
>> Main purpose is to decide (1) the user interface and (2) the >> way to get the plan type from pg_stat_statements. >> >> (1) the user interface >> I added a new boolean column 'generic_plan' to both >> pg_stat_statements view and the member of the hash key of >> pg_stat_statements. >> >> This is because as Legrand pointed out the feature seems >> useful under the condition of differentiating all the >> counters for a queryid using a generic plan and the one >> using a custom one. > I don't like this because this may double the number of entries in pgss. > Which means that the number of entries can more easily reach > pg_stat_statements.max and some entries will be discarded. Not all the entries will be doubled, only the ones that are prepared. And even if auto prepare was implemented, having 5000, 10000 or 20000 max entries seems not a problem to me. >> I thought it might be preferable to make a GUC to enable >> or disable this feature, but changing the hash key makes >> it harder. > What happens if the server was running with this option enabled and then > restarted with the option disabled? Firstly two entries for the same query > were stored in pgss because the option was enabled. But when it's disabled > and the server is restarted, those two entries should be merged into one > at the startup of server? If so, that's problematic because it may take > a long time. What would you think about a third value for this flag to handle disabled case (no need to merge entries in this situation) ? > Therefore I think that it's better and simple to just expose the number of > times generic/custom plan was chosen for each query. I thought this feature was mainly needed to identifiy "under optimal generic plans". Without differentiating execution counters, this will be simpler but useless in this case ... isn't it ? > Regards, > -- > Fujii Masao > Advanced Computing Technology Center > Research and Development Headquarters > NTT DATA CORPORATION Regards PAscal
-
Re: Is it useful to record whether plans are generic or custom?
torikoshia <torikoshia@oss.nttdata.com> — 2020-07-31T09:47:48Z
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
-
Re: Is it useful to record whether plans are generic or custom?
Fujii Masao <masao.fujii@oss.nttdata.com> — 2020-08-17T14:00:54Z
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
-
RE: Is it useful to record whether plans are generic or custom?
legrand legrand <legrand_legrand@hotmail.com> — 2020-08-17T16:35:39Z
>>>> I thought it might be preferable to make a GUC to enable >>>> or disable this feature, but changing the hash key makes >>>> it harder. >> >>> What happens if the server was running with this option enabled and then >>> restarted with the option disabled? Firstly two entries for the same query >>> were stored in pgss because the option was enabled. But when it's disabled >>> and the server is restarted, those two entries should be merged into one >>> at the startup of server? If so, that's problematic because it may take >>> a long time. >> >> What would you think about a third value for this flag to handle disabled >> case (no need to merge entries in this situation) ? > > Sorry I failed to understand your point. You mean that we can have another flag > to specify whether to merge the entries for the same query at that case or not? > > If those entries are not merged, what does pg_stat_statements return? > It returns the sum of those entries? Or either generic or custom entry is > returned? The idea is to use a plan_type enum with 'generic','custom','unknown' or 'unset'. if tracking plan_type is disabled, then plan_type='unknown', else plan_type can take 'generic' or 'custom' value. I'm not proposing to merge results for plan_type when disabling or enabling its tracking. >>> Therefore I think that it's better and simple to just expose the number of >>> times generic/custom plan was chosen for each query. >> >> I thought this feature was mainly needed to identifiy "under optimal generic >> plans". Without differentiating execution counters, this will be simpler but >> useless in this case ... isn't it ? > Could you elaborate "under optimal generic plans"? Sorry, I failed to > understand your point.. But maybe you're thinking to use this feature to > check which generic or custom plan is better for each query? > I was just thinking that this feature was useful to detect the case where > the query was executed with unpected plan mode. That is, we can detect > the unexpected case where the query that should be executed with generic > plan is actually executed with custom plan, and vice versa. There are many exemples in pg lists, where users comes saying that sometimes their query takes a (very) longer time than before, without understand why. I some of some exemples, it is that there is a switch from custom to generic after n executions, and it takes a longer time because generic plan is not as good as custom one (I call them under optimal generic plans). If pgss keeps counters aggregated for both plan_types, I don't see how this (under optimal) can be shown. If there is a line in pgss for custom and an other for generic, then it would be easier to compare. Does this makes sence ? Regards PAscal > Regards, > -- > Fujii Masao > Advanced Computing Technology Center > Research and Development Headquarters > NTT DATA CORPORATION
-
Re: Is it useful to record whether plans are generic or custom?
Michael Paquier <michael@paquier.xyz> — 2020-09-17T04:46:53Z
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
-
Re: Is it useful to record whether plans are generic or custom?
torikoshia <torikoshia@oss.nttdata.com> — 2020-09-28T13:14:01Z
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
-
Re: Is it useful to record whether plans are generic or custom?
legrand legrand <legrand_legrand@hotmail.com> — 2020-09-28T17:39:39Z
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
-
Re: Is it useful to record whether plans are generic or custom?
torikoshia <torikoshia@oss.nttdata.com> — 2020-11-12T01:49:53Z
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
-
Re: Is it useful to record whether plans are generic or custom?
Pavel Stehule <pavel.stehule@gmail.com> — 2020-11-12T05:23:06Z
čt 12. 11. 2020 v 2:50 odesílatel torikoshia <torikoshia@oss.nttdata.com> napsal: > On 2020-09-29 02:39, legrand legrand wrote: > > Hi Atsushi, > > > > +1: Your proposal is a good answer for time based performance analysis > > (even if parsing durationor blks are not differentiated) . > > > > As it makes pgss number of columns wilder, may be an other solution > > would be to create a pg_stat_statements_xxx view with the same key > > as pgss (dbid,userid,queryid) and all thoses new counters. > > Thanks for your ideas and sorry for my late reply. > > It seems creating pg_stat_statements_xxx views both for generic and > custom plans is better than my PoC patch. > > However, I also began to wonder how effective it would be to just > distinguish between generic and custom plans. Custom plans can > include all sorts of plans. and thinking cache validation, generic > plans can also include various plans. > > Considering this, I'm starting to feel that it would be better to > not just keeping whether generic or cutom but the plan itself as > discussed in the below thread. > > > https://www.postgresql.org/message-id/flat/CAKU4AWq5_jx1Vyai0_Sumgn-Ks0R%2BN80cf%2Bt170%2BzQs8x6%3DHew%40mail.gmail.com#f57e64b8d37697c808e4385009340871 > > > Any thoughts? > yes, the plan self is very interesting information - and information if plan was generic or not is interesting too. It is other dimension of query - maybe there can be rule - for any query store max 100 most slows plans with all attributes. The next issue is fact so first first 5 execution of generic plans are not generic in real. This fact should be visible too. Regards Pavel > > Regards, > > -- > Atsushi Torikoshi > > >
-
Re: Is it useful to record whether plans are generic or custom?
torikoshia <torikoshia@oss.nttdata.com> — 2020-11-17T14:21:53Z
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
-
Re: Is it useful to record whether plans are generic or custom?
Pavel Stehule <pavel.stehule@gmail.com> — 2020-11-17T14:31:31Z
út 17. 11. 2020 v 15:21 odesílatel torikoshia <torikoshia@oss.nttdata.com> napsal: > On 2020-11-12 14:23, Pavel Stehule wrote: > > > yes, the plan self is very interesting information - and information > > if plan was generic or not is interesting too. It is other dimension > > of query - maybe there can be rule - for any query store max 100 most > > slows plans with all attributes. The next issue is fact so first first > > 5 execution of generic plans are not generic in real. This fact should > > be visible too. > > Thanks! > However, AFAIU, we can know whether the plan type is generic or custom > from the plan information as described in the manual. > > -- https://www.postgresql.org/docs/devel/sql-prepare.html > > If a generic plan is in use, it will contain parameter symbols $n, > > while a custom plan will have the supplied parameter values substituted > > into it. > > If we can get the plan information, the case like 'first 5 execution > of generic plans are not generic in real' does not happen, doesn't it? > yes postgres=# create table foo(a int); CREATE TABLE postgres=# prepare x as select * from foo where a = $1; PREPARE postgres=# explain execute x(10); ┌─────────────────────────────────────────────────────┐ │ QUERY PLAN │ ╞═════════════════════════════════════════════════════╡ │ Seq Scan on foo (cost=0.00..41.88 rows=13 width=4) │ │ Filter: (a = 10) │ └─────────────────────────────────────────────────────┘ (2 rows) postgres=# explain execute x(10); ┌─────────────────────────────────────────────────────┐ │ QUERY PLAN │ ╞═════════════════════════════════════════════════════╡ │ Seq Scan on foo (cost=0.00..41.88 rows=13 width=4) │ │ Filter: (a = 10) │ └─────────────────────────────────────────────────────┘ (2 rows) postgres=# explain execute x(10); ┌─────────────────────────────────────────────────────┐ │ QUERY PLAN │ ╞═════════════════════════════════════════════════════╡ │ Seq Scan on foo (cost=0.00..41.88 rows=13 width=4) │ │ Filter: (a = 10) │ └─────────────────────────────────────────────────────┘ (2 rows) postgres=# explain execute x(10); ┌─────────────────────────────────────────────────────┐ │ QUERY PLAN │ ╞═════════════════════════════════════════════════════╡ │ Seq Scan on foo (cost=0.00..41.88 rows=13 width=4) │ │ Filter: (a = 10) │ └─────────────────────────────────────────────────────┘ (2 rows) postgres=# explain execute x(10); ┌─────────────────────────────────────────────────────┐ │ QUERY PLAN │ ╞═════════════════════════════════════════════════════╡ │ Seq Scan on foo (cost=0.00..41.88 rows=13 width=4) │ │ Filter: (a = 10) │ └─────────────────────────────────────────────────────┘ (2 rows) postgres=# explain execute x(10); ┌─────────────────────────────────────────────────────┐ │ QUERY PLAN │ ╞═════════════════════════════════════════════════════╡ │ Seq Scan on foo (cost=0.00..41.88 rows=13 width=4) │ │ Filter: (a = $1) │ └─────────────────────────────────────────────────────┘ (2 rows) postgres=# explain execute x(10); ┌─────────────────────────────────────────────────────┐ │ QUERY PLAN │ ╞═════════════════════════════════════════════════════╡ │ Seq Scan on foo (cost=0.00..41.88 rows=13 width=4) │ │ Filter: (a = $1) │ └─────────────────────────────────────────────────────┘ (2 rows) > > Regards, > > -- > Atsushi Torikoshi >
-
Re: Is it useful to record whether plans are generic or custom?
Tatsuro Yamada <tatsuro.yamada.tf@nttcom.co.jp> — 2020-11-30T06:24:23Z
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
-
Re: Is it useful to record whether plans are generic or custom?
Fujii Masao <masao.fujii@oss.nttdata.com> — 2020-12-04T05:29:35Z
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
-
Re: Is it useful to record whether plans are generic or custom?
torikoshia <torikoshia@oss.nttdata.com> — 2020-12-04T06:03:25Z
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,
-
Re: Is it useful to record whether plans are generic or custom?
Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2020-12-04T06:37:33Z
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
-
Re: Is it useful to record whether plans are generic or custom?
torikoshia <torikoshia@oss.nttdata.com> — 2021-01-12T11:36:58Z
> <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 -
Re: Is it useful to record whether plans are generic or custom?
Chengxi Sun <sunchengxi@highgo.com> — 2021-01-22T11:25:40Z
The following review has been posted through the commitfest application: make installcheck-world: tested, failed Implements feature: tested, failed Spec compliant: not tested Documentation: not tested Hi Atsushi, I just run a few test on your latest patch. It works well. I agree with you, I think just tracking generic_calls is enough to get the reason of performance change from pg_stat_statements. I mean if you need more detailed information about the plan and execution of prepared statements, it is better to store this information in a separate view. It seems more reasonable to me. Regards,
-
Re: Is it useful to record whether plans are generic or custom?
Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2021-01-25T05:10:20Z
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
-
Re: Is it useful to record whether plans are generic or custom?
Tatsuro Yamada <tatsuro.yamada.tf@nttcom.co.jp> — 2021-01-27T22:53:18Z
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
-
Re: Is it useful to record whether plans are generic or custom?
Tatsuro Yamada <tatsuro.yamada.tf@nttcom.co.jp> — 2021-01-27T22:56:22Z
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
-
Re: Is it useful to record whether plans are generic or custom?
Tatsuro Yamada <tatsuro.yamada.tf@nttcom.co.jp> — 2021-01-27T23:02:02Z
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
-
Re: Is it useful to record whether plans are generic or custom?
Tatsuro Yamada <tatsuro.yamada.tf@nttcom.co.jp> — 2021-01-27T23:11:08Z
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
-
Re: Is it useful to record whether plans are generic or custom?
torikoshia <torikoshia@oss.nttdata.com> — 2021-02-04T01:16:47Z
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
-
Re: Is it useful to record whether plans are generic or custom?
Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2021-02-04T02:19:53Z
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
-
Re: Is it useful to record whether plans are generic or custom?
torikoshia <torikoshia@oss.nttdata.com> — 2021-02-08T05:02:23Z
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
-
Re: Is it useful to record whether plans are generic or custom?
Fujii Masao <masao.fujii@oss.nttdata.com> — 2021-03-05T08:46:04Z
On 2021/01/28 8:11, Tatsuro Yamada wrote: > Hi Toricoshi-san, > > On 2021/01/12 20:36, torikoshia wrote: >> I suppose it would be normal practice to store past results of >> pg_stat_statements for future comparisons. >> If this is the case, I think that if we only add the number of >> generic plan execution, it will give us a hint to notice the cause >> of performance degradation due to changes in the plan between >> generic and custom. >> >> For example, if there is a clear difference in the number of times >> the generic plan is executed between before and after performance >> degradation as below, it would be natural to check if there is a >> problem with the generic plan. > ... >> Attached a patch that just adds a generic call counter to >> pg_stat_statements. > > > I think that I'd like to use the view when we faced a performance > problem and find the reason. If we did the fixed-point observation > (should I say time-series analysis?) of generic_calls, it allows us to > realize the counter changes, and we can know whether the suspect is > generic_plan or not. So the patch helps DBA, I believe. In that use case maybe what you actually want to see is whether the plan was changed or not, rather than whether generic plan or custom plan is used? If so, it's better to expose seq_scan (num of sequential scans processed by the query) and idx_scan (num of index scans processed by the query) like pg_stat_all_tables, per query in pg_stat_statements? Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
-
Re: Is it useful to record whether plans are generic or custom?
Fujii Masao <masao.fujii@oss.nttdata.com> — 2021-03-05T08:47:54Z
On 2021/02/08 14:02, torikoshia wrote: > On 2021-02-04 11:19, Kyotaro Horiguchi wrote: >> At Thu, 04 Feb 2021 10:16:47 +0900, torikoshia >> <torikoshia@oss.nttdata.com> wrote 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? I just tried this feature. When I set plan_cache_mode to force_generic_plan and executed the following queries, I found that pg_stat_statements.generic_calls and pg_prepared_statements.generic_plans were not the same. Is this behavior expected? I was thinking that they are basically the same. DEALLOCATE ALL; SELECT pg_stat_statements_reset(); PREPARE hoge AS SELECT * FROM pgbench_accounts WHERE aid = $1; EXECUTE hoge(1); EXECUTE hoge(1); EXECUTE hoge(1); SELECT generic_plans, statement FROM pg_prepared_statements WHERE statement LIKE '%hoge%'; generic_plans | statement ---------------+---------------------------------------------------------------- 3 | PREPARE hoge AS SELECT * FROM pgbench_accounts WHERE aid = $1; SELECT calls, generic_calls, query FROM pg_stat_statements WHERE query LIKE '%hoge%'; calls | generic_calls | query -------+---------------+--------------------------------------------------------------- 3 | 2 | PREPARE hoge AS SELECT * FROM pgbench_accounts WHERE aid = $1 When I executed the prepared statements via EXPLAIN ANALYZE, I found pg_stat_statements.generic_calls was not incremented. Is this behavior expected? Or we should count generic_calls even when executing the queries via ProcessUtility()? DEALLOCATE ALL; SELECT pg_stat_statements_reset(); PREPARE hoge AS SELECT * FROM pgbench_accounts WHERE aid = $1; EXPLAIN ANALYZE EXECUTE hoge(1); EXPLAIN ANALYZE EXECUTE hoge(1); EXPLAIN ANALYZE EXECUTE hoge(1); SELECT generic_plans, statement FROM pg_prepared_statements WHERE statement LIKE '%hoge%'; generic_plans | statement ---------------+---------------------------------------------------------------- 3 | PREPARE hoge AS SELECT * FROM pgbench_accounts WHERE aid = $1; SELECT calls, generic_calls, query FROM pg_stat_statements WHERE query LIKE '%hoge%'; calls | generic_calls | query -------+---------------+--------------------------------------------------------------- 3 | 0 | PREPARE hoge AS SELECT * FROM pgbench_accounts WHERE aid = $1 3 | 0 | EXPLAIN ANALYZE EXECUTE hoge(1) Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION -
Re: Is it useful to record whether plans are generic or custom?
torikoshia <torikoshia@oss.nttdata.com> — 2021-03-23T07:32:40Z
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,
-
Re: Is it useful to record whether plans are generic or custom?
Fujii Masao <masao.fujii@oss.nttdata.com> — 2021-03-25T13:14:51Z
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
-
Re: Is it useful to record whether plans are generic or custom?
torikoshia <torikoshia@oss.nttdata.com> — 2021-03-25T15:33:08Z
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.
-
Re: Is it useful to record whether plans are generic or custom?
Fujii Masao <masao.fujii@oss.nttdata.com> — 2021-03-26T08:46:03Z
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
-
Re: Is it useful to record whether plans are generic or custom?
torikoshia <torikoshia@oss.nttdata.com> — 2021-04-05T09:01:48Z
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,