Hi,
When we run prepared statements, as far as I know, running EXPLAIN is the only way to know whether executed plans are generic or custom. There seems no way to know how many times a prepared statement was executed as generic and custom. I think it may be useful to record the number of generic and custom plans mainly to ensure the prepared statements are executed as expected plan type. If people also feel it's useful, I'm going to think about adding columns such as 'generic plans' and 'custom plans' to pg_stat_statements. As you know, pg_stat_statements can now track not only 'calls' but 'plans', so we can presume which plan type was executed from them. When both 'calls' and 'plans' were incremented, plan type would be custom. When only 'calls' was incremented, it would be generic. But considering the case such as only the plan phase has succeeded and the execution phase has failed, this presumption can be wrong. Thoughts? Regards, -- Atsushi Torikoshi |
Hello,
yes this can be usefull, under the condition of differentiating all the counters for a queryid using a generic plan and the one using a custom one. For me one way to do that is adding a generic_plan column to pg_stat_statements key, someting like: - userid, - dbid, - queryid, - generic_plan I don't know if generic/custom plan types are available during planning and/or execution hooks ... Regards PAscal -- Sent from: https://www.postgresql-archive.org/PostgreSQL-hackers-f1928748.html |
On Thu, May 14, 2020 at 2:28 AM legrand legrand <[hidden email]> wrote: Hello, Thanks for your kind advice! I don't know if generic/custom plan types are available during planning Yeah, that's what I'm concerned about. As far as I can see, there are no variables tracking executed plan types so we may need to add variables in CachedPlanSource or somewhere. CachedPlanSource.num_custom_plans looked like what is needed, but it is the number of PLANNING so it also increments when the planner calculates both plans and decides to take generic plan. To track executed plan types, I think execution layer hooks are appropriate. These hooks, however, take QueryDesc as a param and it does not include cached plan information. Portal includes CachedPlanSource but there seem no hooks to take Portal. So I'm wondering it's necessary to add a hook to get Portal or CachedPlanSource. Are these too much change for getting plan types? Regards, -- Atsushi Torikoshi |
> To track executed plan types, I think execution layer hooks
> are appropriate. > These hooks, however, take QueryDesc as a param and it does > not include cached plan information. It seems that the same QueryDesc entry is reused when executing a generic plan. For exemple marking queryDesc->plannedstmt->queryId (outside pg_stat_statements) with a pseudo tag during ExecutorStart reappears in later executions with generic plans ... Is this QueryDesc reusable by a custom plan ? If not maybe a solution could be to add a flag in queryDesc->plannedstmt ? (sorry, I haven't understood yet how and when this generic plan is managed during planning) Regards PAscal -- Sent from: https://www.postgresql-archive.org/PostgreSQL-hackers-f1928748.html |
On Sat, May 16, 2020 at 6:01 PM legrand legrand <[hidden email]> wrote: > To track executed plan types, I think execution layer hooks Thanks for your proposal! I first thought it was a good idea and tried to add a flag to QueryDesc, but the comments on QueryDesc say it encapsulates everything that the executor needs to execute a query. Whether a plan is generic or custom is not what executor needs to know for running queries, so now I hesitate to do so. Instead, I'm now considering using a static hash for prepared queries (static HTAB *prepared_queries). BTW, I'd also appreciate other opinions about recording the number of generic and custom plans on pg_stat_statemtents. Regards, -- Atsushi Torikoshi |
At Tue, 19 May 2020 22:56:17 +0900, Atsushi Torikoshi <[hidden email]> wrote in
> On Sat, May 16, 2020 at 6:01 PM legrand legrand <[hidden email]> > wrote: > > > > To track executed plan types, I think execution layer hooks > > > are appropriate. > > > These hooks, however, take QueryDesc as a param and it does > > > not include cached plan information. > > > > It seems that the same QueryDesc entry is reused when executing > > a generic plan. > > For exemple marking queryDesc->plannedstmt->queryId (outside > > pg_stat_statements) with a pseudo tag during ExecutorStart > > reappears in later executions with generic plans ... > > > > Is this QueryDesc reusable by a custom plan ? If not maybe a solution > > could be to add a flag in queryDesc->plannedstmt ? > > > > Thanks for your proposal! > > I first thought it was a good idea and tried to add a flag to QueryDesc, > but the comments on QueryDesc say it encapsulates everything that > the executor needs to execute a query. > > Whether a plan is generic or custom is not what executor needs to > know for running queries, so now I hesitate to do so. > > Instead, I'm now considering using a static hash for prepared queries > (static HTAB *prepared_queries). > > > BTW, I'd also appreciate other opinions about recording the number > of generic and custom plans on pg_stat_statemtents. If you/we just want to know how a prepared statement is executed, couldn't we show that information in pg_prepared_statements view? =# select * from pg_prepared_statements; -[ RECORD 1 ]---+---------------------------------------------------- name | stmt1 statement | prepare stmt1 as select * from t where b = $1; prepare_time | 2020-05-20 12:01:55.733469+09 parameter_types | {text} from_sql | t exec_custom | 5 <- existing num_custom_plans exec_total | 40 <- new member of CachedPlanSource regards. -- Kyotaro Horiguchi NTT Open Source Software Center |
On Wed, May 20, 2020 at 1:32 PM Kyotaro Horiguchi <[hidden email]> wrote: At Tue, 19 May 2020 22:56:17 +0900, Atsushi Torikoshi <[hidden email]> wrote in Thanks, Horiguchi-san! Adding counters to pg_prepared_statements seems useful when we want to know the way prepared statements executed in the current session. And I also feel adding counters to pg_stat_statements will be convenient especially in production environments because it enables us to get information about not only the current session but all sessions of a PostgreSQL instance. If both changes are worthwhile, considering implementation complexity, it may be reasonable to firstly add columns to pg_prepared_statements and then work on pg_stat_statements. Regards, -- Atsushi Torikoshi |
On 2020/05/20 21:56, Atsushi Torikoshi wrote: > > On Wed, May 20, 2020 at 1:32 PM Kyotaro Horiguchi <[hidden email] <mailto:[hidden email]>> wrote: > > At Tue, 19 May 2020 22:56:17 +0900, Atsushi Torikoshi <[hidden email] <mailto:[hidden email]>> wrote in > > On Sat, May 16, 2020 at 6:01 PM legrand legrand <[hidden email] <mailto:[hidden email]>> > > wrote: > > > > BTW, I'd also appreciate other opinions about recording the number > > of generic and custom plans on pg_stat_statemtents. > > If you/we just want to know how a prepared statement is executed, > couldn't we show that information in pg_prepared_statements view? > > =# select * from pg_prepared_statements; > -[ RECORD 1 ]---+---------------------------------------------------- > name | stmt1 > statement | prepare stmt1 as select * from t where b = $1; > prepare_time | 2020-05-20 12:01:55.733469+09 > parameter_types | {text} > from_sql | t > exec_custom | 5 <- existing num_custom_plans > exec_total | 40 <- new member of CachedPlanSource > > > Thanks, Horiguchi-san! > > Adding counters to pg_prepared_statements seems useful when we want > to know the way prepared statements executed in the current session. I like the idea exposing more CachedPlanSource fields in pg_prepared_statements. I agree it's useful, e.g., for the debug purpose. This is why I implemented the similar feature in my extension. Please see [1] for details. > And I also feel adding counters to pg_stat_statements will be convenient > especially in production environments because it enables us to get > information about not only the current session but all sessions of a > PostgreSQL instance. +1 Regards, [1] https://github.com/MasaoFujii/pg_cheat_funcs#record-pg_cached_plan_sourcestmt-text -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION |
At Thu, 21 May 2020 12:18:16 +0900, Fujii Masao <[hidden email]> wrote in
> > > On 2020/05/20 21:56, Atsushi Torikoshi wrote: > > On Wed, May 20, 2020 at 1:32 PM Kyotaro Horiguchi > > <[hidden email] <mailto:[hidden email]>> wrote: > > At Tue, 19 May 2020 22:56:17 +0900, Atsushi Torikoshi > > <[hidden email] <mailto:[hidden email]>> wrote in > > > On Sat, May 16, 2020 at 6:01 PM legrand legrand > > > <[hidden email] <mailto:[hidden email]>> > > > 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. 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 <[hidden email]> wrote in > Instead, I'm now considering using a static hash for prepared queries > (static HTAB *prepared_queries). That might be complex and fragile considering nested query and SPI calls. I'm not sure, but could we use ActivePortal? ActivePortal->cplan is a CachedPlan, which can hold the generic/custom information. Instead, > [1] > https://github.com/MasaoFujii/pg_cheat_funcs#record-pg_cached_plan_sourcestmt-text regards. -- Kyotaro Horiguchi NTT Open Source Software Center From 1c6a3dd41a59504244134ee44ddd4516191656da Mon Sep 17 00:00:00 2001 From: Kyotaro Horiguchi <[hidden email]> Date: Thu, 21 May 2020 15:32:38 +0900 Subject: [PATCH] Expose counters of plancache to pg_prepared_statements We didn't have an easy way to find how many times generic or custom plans of a prepared statement have been executed. This patch exposes such numbers and costs of both plans in pg_prepared_statements. --- doc/src/sgml/catalogs.sgml | 45 +++++++++++++++++++++++++++ src/backend/commands/prepare.c | 30 ++++++++++++++++-- src/backend/utils/cache/plancache.c | 13 ++++---- src/include/catalog/pg_proc.dat | 6 ++-- src/include/utils/plancache.h | 5 +-- src/test/regress/expected/prepare.out | 43 +++++++++++++++++++++++++ src/test/regress/expected/rules.out | 9 ++++-- src/test/regress/sql/prepare.sql | 9 ++++++ 8 files changed, 144 insertions(+), 16 deletions(-) diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml index b1b077c97f..950ed30694 100644 --- a/doc/src/sgml/catalogs.sgml +++ b/doc/src/sgml/catalogs.sgml @@ -10826,6 +10826,51 @@ SELECT * FROM pg_locks pl LEFT JOIN pg_prepared_xacts ppx frontend/backend protocol </para></entry> </row> + + <row> + <entry role="catalog_table_entry"><para role="column_definition"> + <structfield>calls</structfield> <type>bigint</type> + </para> + <para> + Number of times the prepared statement was executed + </para></entry> + </row> + + <row> + <entry role="catalog_table_entry"><para role="column_definition"> + <structfield>custom_calls</structfield> <type>bigint</type> + </para> + <para> + Number of times generic plan is executed for the prepared statement + </para></entry> + </row> + + <row> + <entry role="catalog_table_entry"><para role="column_definition"> + <structfield>plan_geneation</structfield> <type>bigint</type> + </para> + <para> + Number of times execution plan was generated for the prepared statement + </para></entry> + </row> + + <row> + <entry role="catalog_table_entry"><para role="column_definition"> + <structfield>generic_cost</structfield> <type>double precision</type> + </para> + <para> + Estimated cost of generic plan. NULL if not yet planned. + </para></entry> + </row> + + <row> + <entry role="catalog_table_entry"><para role="column_definition"> + <structfield>custom_cost</structfield> <type>double precision</type> + </para> + <para> + Estimated cost of custom plans. NULL if not yet planned. This is mean of the estimated costs for the past calls including planning cost. + </para></entry> + </row> </tbody> </tgroup> </table> diff --git a/src/backend/commands/prepare.c b/src/backend/commands/prepare.c index 80d6df8ac1..e6755df543 100644 --- a/src/backend/commands/prepare.c +++ b/src/backend/commands/prepare.c @@ -723,7 +723,7 @@ pg_prepared_statement(PG_FUNCTION_ARGS) * build tupdesc for result tuples. This must match the definition of the * pg_prepared_statements view in system_views.sql */ - tupdesc = CreateTemplateTupleDesc(5); + tupdesc = CreateTemplateTupleDesc(10); TupleDescInitEntry(tupdesc, (AttrNumber) 1, "name", TEXTOID, -1, 0); TupleDescInitEntry(tupdesc, (AttrNumber) 2, "statement", @@ -734,6 +734,16 @@ pg_prepared_statement(PG_FUNCTION_ARGS) REGTYPEARRAYOID, -1, 0); TupleDescInitEntry(tupdesc, (AttrNumber) 5, "from_sql", BOOLOID, -1, 0); + TupleDescInitEntry(tupdesc, (AttrNumber) 6, "calls", + INT8OID, -1, 0); + TupleDescInitEntry(tupdesc, (AttrNumber) 7, "custom_calls", + INT8OID, -1, 0); + TupleDescInitEntry(tupdesc, (AttrNumber) 8, "plan_generation", + INT8OID, -1, 0); + TupleDescInitEntry(tupdesc, (AttrNumber) 9, "generic_cost", + FLOAT8OID, -1, 0); + TupleDescInitEntry(tupdesc, (AttrNumber) 10, "custom_cost", + FLOAT8OID, -1, 0); /* * We put all the tuples into a tuplestore in one scan of the hashtable. @@ -755,8 +765,8 @@ pg_prepared_statement(PG_FUNCTION_ARGS) hash_seq_init(&hash_seq, prepared_queries); while ((prep_stmt = hash_seq_search(&hash_seq)) != NULL) { - Datum values[5]; - bool nulls[5]; + Datum values[10]; + bool nulls[10]; MemSet(nulls, 0, sizeof(nulls)); @@ -766,6 +776,20 @@ pg_prepared_statement(PG_FUNCTION_ARGS) values[3] = build_regtype_array(prep_stmt->plansource->param_types, prep_stmt->plansource->num_params); values[4] = BoolGetDatum(prep_stmt->from_sql); + values[5] = Int64GetDatumFast(prep_stmt->plansource->num_total_calls); + values[6] = Int64GetDatumFast(prep_stmt->plansource->num_custom_plans); + values[7] = Int64GetDatumFast(prep_stmt->plansource->generation); + if (prep_stmt->plansource->generic_cost >= 0.0) + values[8] = Float8GetDatum(prep_stmt->plansource->generic_cost); + else + nulls[8] = true; + + if (prep_stmt->plansource->num_custom_plans > 0) + values[9] = + Float8GetDatum(prep_stmt->plansource->total_custom_cost / + prep_stmt->plansource->num_custom_plans); + else + nulls[9] = true; tuplestore_putvalues(tupstore, tupdesc, values, nulls); } diff --git a/src/backend/utils/cache/plancache.c b/src/backend/utils/cache/plancache.c index 75b475c179..d91444f60f 100644 --- a/src/backend/utils/cache/plancache.c +++ b/src/backend/utils/cache/plancache.c @@ -286,6 +286,7 @@ CreateOneShotCachedPlan(RawStmt *raw_parse_tree, plansource->generic_cost = -1; plansource->total_custom_cost = 0; plansource->num_custom_plans = 0; + plansource->num_total_calls = 0; return plansource; } @@ -1213,14 +1214,13 @@ GetCachedPlan(CachedPlanSource *plansource, ParamListInfo boundParams, { /* Build a custom plan */ plan = BuildCachedPlan(plansource, qlist, boundParams, queryEnv); - /* Accumulate total costs of custom plans, but 'ware overflow */ - if (plansource->num_custom_plans < INT_MAX) - { - plansource->total_custom_cost += cached_plan_cost(plan, true); - plansource->num_custom_plans++; - } + /* Accumulate total costs of custom plans */ + plansource->total_custom_cost += cached_plan_cost(plan, true); + plansource->num_custom_plans++; } + plansource->num_total_calls++; + Assert(plan != NULL); /* Flag the plan as in use by caller */ @@ -1575,6 +1575,7 @@ CopyCachedPlan(CachedPlanSource *plansource) newsource->generic_cost = plansource->generic_cost; newsource->total_custom_cost = plansource->total_custom_cost; newsource->num_custom_plans = plansource->num_custom_plans; + newsource->num_total_calls = plansource->num_total_calls; MemoryContextSwitchTo(oldcxt); diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat index 61f2c2f5b4..48d0d88d97 100644 --- a/src/include/catalog/pg_proc.dat +++ b/src/include/catalog/pg_proc.dat @@ -7743,9 +7743,9 @@ { oid => '2510', descr => 'get the prepared statements for this session', proname => 'pg_prepared_statement', prorows => '1000', proretset => 't', provolatile => 's', proparallel => 'r', prorettype => 'record', - proargtypes => '', proallargtypes => '{text,text,timestamptz,_regtype,bool}', - proargmodes => '{o,o,o,o,o}', - proargnames => '{name,statement,prepare_time,parameter_types,from_sql}', + proargtypes => '', proallargtypes => '{text,text,timestamptz,_regtype,bool,int8,int8,int8,float8,float8}', + proargmodes => '{o,o,o,o,o,o,o,o,o,o}', + proargnames => '{name,statement,prepare_time,parameter_types,from_sql,calls,custom_calls,plan_generation,generic_cost,custom_cost}', prosrc => 'pg_prepared_statement' }, { oid => '2511', descr => 'get the open cursors for this session', proname => 'pg_cursor', prorows => '1000', proretset => 't', diff --git a/src/include/utils/plancache.h b/src/include/utils/plancache.h index 522020d763..146eb15d34 100644 --- a/src/include/utils/plancache.h +++ b/src/include/utils/plancache.h @@ -124,13 +124,14 @@ typedef struct CachedPlanSource bool is_complete; /* has CompleteCachedPlan been done? */ bool is_saved; /* has CachedPlanSource been "saved"? */ bool is_valid; /* is the query_list currently valid? */ - int generation; /* increments each time we create a plan */ + int64 generation; /* increments each time we create a plan */ /* If CachedPlanSource has been saved, it is a member of a global list */ dlist_node node; /* list link, if is_saved */ /* State kept to help decide whether to use custom or generic plans: */ double generic_cost; /* cost of generic plan, or -1 if not known */ double total_custom_cost; /* total cost of custom plans so far */ - int num_custom_plans; /* number of plans included in total */ + int64 num_custom_plans; /* # of custom plans included in total */ + int64 num_total_calls;/* total number of execution */ } CachedPlanSource; /* diff --git a/src/test/regress/expected/prepare.out b/src/test/regress/expected/prepare.out index 3306c696b1..b41e75c275 100644 --- a/src/test/regress/expected/prepare.out +++ b/src/test/regress/expected/prepare.out @@ -64,6 +64,49 @@ EXECUTE q2('postgres'); postgres | f | t (1 row) +EXECUTE q2('postgres'); + datname | datistemplate | datallowconn +----------+---------------+-------------- + postgres | f | t +(1 row) + +EXECUTE q2('postgres'); + datname | datistemplate | datallowconn +----------+---------------+-------------- + postgres | f | t +(1 row) + +EXECUTE q2('postgres'); + datname | datistemplate | datallowconn +----------+---------------+-------------- + postgres | f | t +(1 row) + +EXECUTE q2('postgres'); + datname | datistemplate | datallowconn +----------+---------------+-------------- + postgres | f | t +(1 row) + +EXECUTE q2('postgres'); + datname | datistemplate | datallowconn +----------+---------------+-------------- + postgres | f | t +(1 row) + +EXECUTE q2('postgres'); + datname | datistemplate | datallowconn +----------+---------------+-------------- + postgres | f | t +(1 row) + +-- the view should return the correct counts +SELECT name, calls, custom_calls, plan_generation FROM pg_prepared_statements; + name | calls | custom_calls | plan_generation +------+-------+--------------+----------------- + q2 | 7 | 5 | 6 +(1 row) + PREPARE q3(text, int, float, boolean, smallint) AS SELECT * FROM tenk1 WHERE string4 = $1 AND (four = $2 OR ten = $3::bigint OR true = $4 OR odd = $5::int) diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out index b813e32215..ff9ce88ce1 100644 --- a/src/test/regress/expected/rules.out +++ b/src/test/regress/expected/rules.out @@ -1428,8 +1428,13 @@ pg_prepared_statements| SELECT p.name, p.statement, p.prepare_time, p.parameter_types, - p.from_sql - FROM pg_prepared_statement() p(name, statement, prepare_time, parameter_types, from_sql); + p.from_sql, + p.calls, + p.custom_calls, + p.plan_generation, + p.generic_cost, + p.custom_cost + FROM pg_prepared_statement() p(name, statement, prepare_time, parameter_types, from_sql, calls, custom_calls, plan_generation, generic_cost, custom_cost); pg_prepared_xacts| SELECT p.transaction, p.gid, p.prepared, diff --git a/src/test/regress/sql/prepare.sql b/src/test/regress/sql/prepare.sql index 985d0f05c9..9d73c2fea3 100644 --- a/src/test/regress/sql/prepare.sql +++ b/src/test/regress/sql/prepare.sql @@ -35,6 +35,15 @@ PREPARE q2(text) AS FROM pg_database WHERE datname = $1; EXECUTE q2('postgres'); +EXECUTE q2('postgres'); +EXECUTE q2('postgres'); +EXECUTE q2('postgres'); +EXECUTE q2('postgres'); +EXECUTE q2('postgres'); +EXECUTE q2('postgres'); + +-- the view should return the correct counts +SELECT name, calls, custom_calls, plan_generation FROM pg_prepared_statements; PREPARE q3(text, int, float, boolean, smallint) AS SELECT * FROM tenk1 WHERE string4 = $1 AND (four = $2 OR -- 2.18.2 |
Hi Torikoshi-san!
On 2020/05/21 17:10, Kyotaro Horiguchi wrote: > At Thu, 21 May 2020 12:18:16 +0900, Fujii Masao <[hidden email]> wrote in >> >> >> On 2020/05/20 21:56, Atsushi Torikoshi wrote: >>> On Wed, May 20, 2020 at 1:32 PM Kyotaro Horiguchi >>> <[hidden email] <mailto:[hidden email]>> wrote: >>> At Tue, 19 May 2020 22:56:17 +0900, Atsushi Torikoshi >>> <[hidden email] <mailto:[hidden email]>> wrote in >>> > On Sat, May 16, 2020 at 6:01 PM legrand legrand >>> > <[hidden email] <mailto:[hidden email]>> >>> > 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. Please find attached file. # Test case prepare count as select count(*) from pg_class where oid >$1; execute count(1); select * from pg_prepared_statements; -[ RECORD 1 ]---+-------------------------------------------------------------- name | count statement | prepare count as select count(*) from pg_class where oid >$1; prepare_time | 2020-05-21 17:41:16.134362+09 parameter_types | {oid} from_sql | t is_generic_plan | f <= False You can see the following result, when you execute it 6 times. -[ RECORD 1 ]---+-------------------------------------------------------------- name | count statement | prepare count as select count(*) from pg_class where oid >$1; prepare_time | 2020-05-21 17:41:16.134362+09 parameter_types | {oid} from_sql | t is_generic_plan | t <= True Thanks, Tatsuro Yamada |
Thanks for writing a patch! On Thu, May 21, 2020 at 5:10 PM Kyotaro Horiguchi <[hidden email]> wrote: At Thu, 21 May 2020 12:18:16 +0900, Fujii Masao <[hidden email]> wrote in I didn't come up with ideas about how to use them. IMHO they might not so necessary.. =# select * from pg_prepared_statements; +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 <[hidden email]> wrote in 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 <[hidden email]> wrote: I tried to creating PoC patch too, so I share it. Thanks! I agree with your idea showing the latest plan is generic or custom. This patch judges whether the lastest plan was generic based on plansource->gplan existence, but plansource->gplan can exist even when the planner chooses custom. For example, a prepared statement was executed first 6 times and a generic plan was generated for comparison but the custom plan won. Attached another patch showing latest plan based on '0001-Expose-counters-of-plancache-to-pg_prepared_statemen.patch'. As I wrote above, I suppose some of the columns might not necessary and it'd better change some column and variable names, but I left them for other opinions. Regards, -- Atsushi Torikoshi |
On Mon, May 25, 2020 at 10:54 AM Atsushi Torikoshi <[hidden email]> wrote:
I've modified the above points and also exposed the numbers of each generic plans and custom plans. I'm now a little bit worried about the below change which removed the overflow checking for num_custom_plans, which was introduced in 0001-Expose-counters-of-plancache-to-pg_prepared_statement.patch, but I've left it because the maximum of int64 seems enough large for counters. Also referencing other counters in pg_stat_user_tables, they don't seem to care about it. ``` - /* Accumulate total costs of custom plans, but 'ware overflow */ - if (plansource->num_custom_plans < INT_MAX) - { - plansource->total_custom_cost += cached_plan_cost(plan, true); - plansource->num_custom_plans++; - } ``` Regards, Atsushi Torikoshi |
On 2020-06-04 17:04, Atsushi Torikoshi wrote:
> On Mon, May 25, 2020 at 10:54 AM Atsushi Torikoshi <[hidden email]> > wrote: > >> On Thu, May 21, 2020 at 5:10 PM Kyotaro Horiguchi >> <[hidden email]> wrote: >> >>> Cost numbers would look better if it is cooked a bit. Is it worth >>> being in core? >> >> I didn't come up with ideas about how to use them. >> IMHO they might not so necessary.. > >>> Perhaps plan_generation is not needed there. >> >> +1. >> Now 'calls' is sufficient and 'plan_generation' may confuse users. >> >> BTW, considering 'calls' in pg_stat_statements is the number of >> times >> statements were EXECUTED and 'plans' is the number of times >> statements were PLANNED, how about substituting 'calls' for >> 'plans'? > > I've modified the above points and also exposed the numbers of each > generic plans and custom plans. > > I'm now a little bit worried about the below change which removed > the overflow checking for num_custom_plans, which was introduced > in 0001-Expose-counters-of-plancache-to-pg_prepared_statement.patch, > but I've left it because the maximum of int64 seems enough large > for counters. > Also referencing other counters in pg_stat_user_tables, they don't > seem to care about it. > > ``` > - /* Accumulate total costs of custom plans, but 'ware > overflow */ > - if (plansource->num_custom_plans < INT_MAX) > - { > - plansource->total_custom_cost += > cached_plan_cost(plan, true); > - plansource->num_custom_plans++; > - } > > ``` > > Regards, > > Atsushi Torikoshi > >> As a user, I think this feature is useful for performance analysis. I hope that this feature is merged. BTW, I found that the dependency between function's comments and the modified code is broken at latest patch. Before this is committed, please fix it. ``` diff --git a/src/backend/commands/prepare.c b/src/backend/commands/prepare.c index 990782e77f..b63d3214df 100644 --- a/src/backend/commands/prepare.c +++ b/src/backend/commands/prepare.c @@ -694,7 +694,8 @@ ExplainExecuteQuery(ExecuteStmt *execstmt, IntoClause *into, ExplainState *es, /* * This set returning function reads all the prepared statements and - * returns a set of (name, statement, prepare_time, param_types, from_sql). + * returns a set of (name, statement, prepare_time, param_types, from_sql, + * generic_plans, custom_plans, last_plan). */ Datum pg_prepared_statement(PG_FUNCTION_ARGS) ``` Regards, -- Masahiro Ikeda |
On 2020-06-08 20:45, Masahiro Ikeda wrote:
> BTW, I found that the dependency between function's comments and > the modified code is broken at latest patch. Before this is > committed, please fix it. > > ``` > diff --git a/src/backend/commands/prepare.c > b/src/backend/commands/prepare.c > index 990782e77f..b63d3214df 100644 > --- a/src/backend/commands/prepare.c > +++ b/src/backend/commands/prepare.c > @@ -694,7 +694,8 @@ ExplainExecuteQuery(ExecuteStmt *execstmt, > IntoClause *into, ExplainState *es, > > /* > * This set returning function reads all the prepared statements and > - * returns a set of (name, statement, prepare_time, param_types, > from_sql). > + * returns a set of (name, statement, prepare_time, param_types, > from_sql, > + * generic_plans, custom_plans, last_plan). > */ > Datum > pg_prepared_statement(PG_FUNCTION_ARGS) > ``` I've fixed it. Regards, -- Atsushi Torikoshi |
At Wed, 10 Jun 2020 10:50:58 +0900, torikoshia <[hidden email]> wrote in
> On 2020-06-08 20:45, Masahiro Ikeda wrote: > > > BTW, I found that the dependency between function's comments and > > the modified code is broken at latest patch. Before this is > > committed, please fix it. > > ``` > > diff --git a/src/backend/commands/prepare.c > > b/src/backend/commands/prepare.c > > index 990782e77f..b63d3214df 100644 > > --- a/src/backend/commands/prepare.c > > +++ b/src/backend/commands/prepare.c > > @@ -694,7 +694,8 @@ ExplainExecuteQuery(ExecuteStmt *execstmt, > > IntoClause *into, ExplainState *es, > > /* > > * This set returning function reads all the prepared statements and > > - * returns a set of (name, statement, prepare_time, param_types, > > - * from_sql). > > + * returns a set of (name, statement, prepare_time, param_types, > > from_sql, > > + * generic_plans, custom_plans, last_plan). > > */ > > Datum > > pg_prepared_statement(PG_FUNCTION_ARGS) > > ``` > > Thanks for reviewing! > > I've fixed it. + TupleDescInitEntry(tupdesc, (AttrNumber) 8, "last_plan", This could be a problem if we showed the last plan in this view. I think "last_plan_type" would be better. + if (prep_stmt->plansource->last_plan_type == PLAN_CACHE_TYPE_CUSTOM) + values[7] = CStringGetTextDatum("custom"); + else if (prep_stmt->plansource->last_plan_type == PLAN_CACHE_TYPE_GENERIC) + values[7] = CStringGetTextDatum("generic"); + else + nulls[7] = true; Using swith-case prevents future additional type (if any) from being unhandled. I think we are recommending that as a convension. regards. -- Kyotaro Horiguchi NTT Open Source Software Center |
On 2020-06-10 18:00, Kyotaro Horiguchi wrote:
> > + TupleDescInitEntry(tupdesc, (AttrNumber) 8, "last_plan", > > This could be a problem if we showed the last plan in this view. I > think "last_plan_type" would be better. > > + if (prep_stmt->plansource->last_plan_type == > PLAN_CACHE_TYPE_CUSTOM) > + values[7] = CStringGetTextDatum("custom"); > + else if (prep_stmt->plansource->last_plan_type == > PLAN_CACHE_TYPE_GENERIC) > + values[7] = CStringGetTextDatum("generic"); > + else > + nulls[7] = true; > > Using swith-case prevents future additional type (if any) from being > unhandled. I think we are recommending that as a convension. I've attached a patch that reflects your comments. Regards, -- Atsushi Torikoshi |
On 2020/06/11 14:59, torikoshia wrote: > On 2020-06-10 18:00, Kyotaro Horiguchi wrote: > >> >> + TupleDescInitEntry(tupdesc, (AttrNumber) 8, "last_plan", >> >> This could be a problem if we showed the last plan in this view. I >> think "last_plan_type" would be better. >> >> + if (prep_stmt->plansource->last_plan_type == PLAN_CACHE_TYPE_CUSTOM) >> + values[7] = CStringGetTextDatum("custom"); >> + else if (prep_stmt->plansource->last_plan_type == PLAN_CACHE_TYPE_GENERIC) >> + values[7] = CStringGetTextDatum("generic"); >> + else >> + nulls[7] = true; >> >> Using swith-case prevents future additional type (if any) from being >> unhandled. I think we are recommending that as a convension. > > Thanks for your reviewing! > > I've attached a patch that reflects your comments. Thanks for the patch! Here are the comments. + Number of times generic plan was choosen + Number of times custom plan was choosen Typo: "choosen" should be "chosen"? + <entry role="catalog_table_entry"><para role="column_definition"> + <structfield>last_plan_type</structfield> <type>text</type> + </para> + <para> + Tells the last plan type was generic or custom. If the prepared + statement has not executed yet, this field is null + </para></entry> Could you tell me how this information is expected to be used? I think that generic_plans and custom_plans are useful when investigating the cause of performance drop by cached plan mode. But I failed to get how much useful last_plan_type is. Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION |
On 2020-07-06 22:16, Fujii Masao wrote:
> On 2020/06/11 14:59, torikoshia wrote: >> On 2020-06-10 18:00, Kyotaro Horiguchi wrote: >> >>> >>> + TupleDescInitEntry(tupdesc, (AttrNumber) 8, "last_plan", >>> >>> This could be a problem if we showed the last plan in this view. I >>> think "last_plan_type" would be better. >>> >>> + if (prep_stmt->plansource->last_plan_type == >>> PLAN_CACHE_TYPE_CUSTOM) >>> + values[7] = CStringGetTextDatum("custom"); >>> + else if (prep_stmt->plansource->last_plan_type == >>> PLAN_CACHE_TYPE_GENERIC) >>> + values[7] = CStringGetTextDatum("generic"); >>> + else >>> + nulls[7] = true; >>> >>> Using swith-case prevents future additional type (if any) from being >>> unhandled. I think we are recommending that as a convension. >> >> Thanks for your reviewing! >> >> I've attached a patch that reflects your comments. > > Thanks for the patch! Here are the comments. > + 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. to ensure whether generic or custom plan was chosen for specific queries in a development environment. Of course, we can know it from adding EXPLAIN and ensuring whether $n is contained in the plan, but I feel using the view is easier to use and understand. Regards, -- Atsushi Torikoshi NTT DATA CORPORATION |
On 2020/07/08 10:14, torikoshia wrote: > On 2020-07-06 22:16, Fujii Masao wrote: >> On 2020/06/11 14:59, torikoshia wrote: >>> On 2020-06-10 18:00, Kyotaro Horiguchi wrote: >>> >>>> >>>> + TupleDescInitEntry(tupdesc, (AttrNumber) 8, "last_plan", >>>> >>>> This could be a problem if we showed the last plan in this view. I >>>> think "last_plan_type" would be better. >>>> >>>> + if (prep_stmt->plansource->last_plan_type == PLAN_CACHE_TYPE_CUSTOM) >>>> + values[7] = CStringGetTextDatum("custom"); >>>> + else if (prep_stmt->plansource->last_plan_type == PLAN_CACHE_TYPE_GENERIC) >>>> + values[7] = CStringGetTextDatum("generic"); >>>> + else >>>> + nulls[7] = true; >>>> >>>> Using swith-case prevents future additional type (if any) from being >>>> unhandled. I think we are recommending that as a convension. >>> >>> Thanks for your reviewing! >>> >>> I've attached a patch that reflects your comments. >> >> Thanks for the patch! Here are the comments. > > Thanks for your review! > >> + Number of times generic plan was choosen >> + Number of times custom plan was choosen >> >> Typo: "choosen" should be "chosen"? > > Thanks, fixed them. > >> + <entry role="catalog_table_entry"><para role="column_definition"> >> + <structfield>last_plan_type</structfield> <type>text</type> >> + </para> >> + <para> >> + Tells the last plan type was generic or custom. If the prepared >> + statement has not executed yet, this field is null >> + </para></entry> >> >> Could you tell me how this information is expected to be used? >> I think that generic_plans and custom_plans are useful when investigating >> the cause of performance drop by cached plan mode. But I failed to get >> how much useful last_plan_type is. > > This may be an exceptional case, but I once had a case needed > to ensure whether generic or custom plan was chosen for specific > queries in a development environment. In your case, probably you had to ensure that the last multiple (or every) executions chose generic or custom plan? If yes, I'm afraid that displaying only the last plan mode is not enough for your case. No? So it seems better to check generic_plans or custom_plans columns in the view rather than last_plan_type even in your case. Thought? Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION |
On 2020-07-08 16:41, Fujii Masao wrote:
> On 2020/07/08 10:14, torikoshia wrote: >> On 2020-07-06 22:16, Fujii Masao wrote: >>> On 2020/06/11 14:59, torikoshia wrote: >>>> On 2020-06-10 18:00, Kyotaro Horiguchi wrote: >>>> >>>>> >>>>> + TupleDescInitEntry(tupdesc, (AttrNumber) 8, "last_plan", >>>>> >>>>> This could be a problem if we showed the last plan in this view. I >>>>> think "last_plan_type" would be better. >>>>> >>>>> + if (prep_stmt->plansource->last_plan_type == >>>>> PLAN_CACHE_TYPE_CUSTOM) >>>>> + values[7] = CStringGetTextDatum("custom"); >>>>> + else if (prep_stmt->plansource->last_plan_type == >>>>> PLAN_CACHE_TYPE_GENERIC) >>>>> + values[7] = CStringGetTextDatum("generic"); >>>>> + else >>>>> + nulls[7] = true; >>>>> >>>>> Using swith-case prevents future additional type (if any) from >>>>> being >>>>> unhandled. I think we are recommending that as a convension. >>>> >>>> Thanks for your reviewing! >>>> >>>> I've attached a patch that reflects your comments. >>> >>> Thanks for the patch! Here are the comments. >> >> Thanks for your review! >> >>> + Number of times generic plan was choosen >>> + Number of times custom plan was choosen >>> >>> Typo: "choosen" should be "chosen"? >> >> Thanks, fixed them. >> >>> + <entry role="catalog_table_entry"><para >>> role="column_definition"> >>> + <structfield>last_plan_type</structfield> <type>text</type> >>> + </para> >>> + <para> >>> + Tells the last plan type was generic or custom. If the >>> prepared >>> + statement has not executed yet, this field is null >>> + </para></entry> >>> >>> Could you tell me how this information is expected to be used? >>> I think that generic_plans and custom_plans are useful when >>> investigating >>> the cause of performance drop by cached plan mode. But I failed to >>> get >>> how much useful last_plan_type is. >> >> This may be an exceptional case, but I once had a case needed >> to ensure whether generic or custom plan was chosen for specific >> queries in a development environment. > > In your case, probably you had to ensure that the last multiple (or > every) > executions chose generic or custom plan? If yes, I'm afraid that > displaying > only the last plan mode is not enough for your case. No? > So it seems better to check generic_plans or custom_plans columns in > the > view rather than last_plan_type even in your case. Thought? Yeah, I now feel last_plan is not so necessary and only the numbers of generic/custom plan is enough. If there are no objections, I'm going to remove this column and related codes. Regards, -- Atsushi Torikoshi NTT DATA CORPORATION |
Free forum by Nabble | Edit this page |