Re: Is it useful to record whether plans are generic or custom?
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,
Re: Is it useful to record whether plans are generic or custom?
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?
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.diff --git a/contrib/pg_stat_statements/expected/pg_stat_statements.out b/contrib/pg_stat_statements/expected/pg_stat_statements.out index 16158525ca..887c4b2be8 100644 --- a/contrib/pg_stat_statements/expected/pg_stat_statements.out +++ b/contrib/pg_stat_statements/expected/pg_stat_statements.out @@ -251,6 +251,72 @@ FROM pg_stat_statements ORDER BY query COLLATE "C"; UPDATE pgss_test SET b = $1 WHERE a > $2 | 1 |3 | t | t | t (7 rows) +-- +-- Track the number of generic plan +-- +CREATE TABLE pgss_test (i int, j int, k int); +SELECT pg_stat_statements_reset(); + pg_stat_statements_reset +-- + +(1 row) + +SET plan_cache_mode TO force_generic_plan; +SET pg_stat_statements.track_utility = TRUE; +PREPARE pgss_p1 AS SELECT i FROM pgss_test WHERE i = $1; +EXECUTE pgss_p1(1); + i +--- +(0 rows) + +-- EXPLAIN ANALZE should be recorded +PREPARE pgss_p2 AS SELECT j FROM pgss_test WHERE j = $1; +EXPLAIN (ANALYZE, COSTS OFF, SUMMARY OFF, TIMING OFF) EXECUTE pgss_p2(1); + QUERY PLAN +--- + Seq Scan on pgss_test (actual rows=0 loops=1) + Filter: (j = $1) +(2 rows) + +-- Nested Portal +PREPARE pgss_p3 AS SELECT k FROM pgss_test WHERE k = $1; +BEGIN; +DECLARE pgss_c1 CURSOR FOR SELECT name FROM pg_prepared_statements; +FETCH IN pgss_c1; + name +- + pgss_p2 +(1 row) + +EXECUTE pgss_p3(1); + k +--- +(0 rows) + +FETCH IN pgss_c1; + name +- + pgss_p1 +(1 row) + +COMMIT; +SELECT calls, generic_calls, query FROM pg_stat_statements; + calls | generic_calls | query +---+---+-- + 1 | 0 | DECLARE pgss_c1 CURSOR FOR SELECT name FROM pg_prepared_statements + 0 | 0 | SELECT calls, generic_calls, query FROM pg_stat_statements + 1 | 1 | PREPARE pgss_p1 AS SELECT i FROM pgss_test WHERE i = $1 + 2 | 0 | FETCH IN pgss_c1 + 1 | 0 | BEGIN + 1 | 0 | SELECT pg_stat_statements_reset() + 1 | 1 | EXPLAIN (ANALYZE, COSTS OFF, SUMMARY OFF, TIMING OFF) EXECUTE pgss_p2(1) + 1 | 0 | COMMIT + 1 | 1 | PREPARE pgss_p3 AS SELECT k FROM pgss_test WHERE k = $1 +(9 rows) + +SET pg_stat_statements.track_utility = FALSE; +DEALLOCATE ALL; +DROP TABLE pgss_test; -- -- pg_stat_statements.track = none -- diff --git a/contrib/pg_stat_statements/pg_stat_statements--1.7--1.8.sql b/contrib/pg_stat_statements/pg_stat_statements--1.7--1.8.sql index 0f63f08f7e..7fdef315ae 100644 --- a/contrib/pg_stat_statements/pg_stat_statements--1.7--1.8.sql +++ b/contrib/pg_stat_statements/pg_stat_statements--1.7--1.8.sql @@ -44,7 +44,8 @@ CREATE FUNCTION pg_stat_statements(IN showtext boolean, OUT blk_write_time float8, OUT wal_records int8, OUT wal_fpi int8, -OUT wal_bytes numeric +OUT wal_bytes numeric, +OUT generic_calls int8 ) RETURNS SETOF record AS 'MODULE_PATHNAME', 'pg_stat_statements_1_8' diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c index 62cccbfa44..b14919c989 100644 --- a/contrib/pg_stat_statements/pg_stat_statements.c +++ b/contrib/pg_stat_statements/pg_stat_statements.c @@ -81,6 +81,7 @@ #include "utils/acl.h" #include "utils/builtins.h" #include "utils/memutils.h" +#include "utils/plancache.h" #include "utils/timestamp.h" PG_MODULE_MAGIC; @@ -192,6 +193,7 @@ typedef struct Counters int64 wal_records; /* # of WAL records generated */ int64 wal_fpi; /* # of WAL full page images generated */ uint64 wal_bytes; /* total amount of WAL generated in bytes */ + int64 generic_calls; /* # of times generic plans executed */ } Counters; /* @@ -1446,6 +1448,10 @@ pgss_store(const char *query, uint64 queryId, if (e->counters.max_time[kind] < total_time) e->counters.max_time[kind] = total_time; } + + if (kind == PGSS_EXEC && is_plan_type_generic) + e->counters.generic_calls += 1; + e->counters.rows += rows; e->counters.shared_blks_hit += bufusage->shared_blks_hit; e->counters.shared_blks_read += bufusage->shared_blks_read; @@ -1510,8 +1516,8 @@ pg_stat_statements_reset(PG_FUNCTION_ARGS) #define PG_STAT_STATEMENTS_COLS_V1_1 18 #define PG_STAT_STATEMENTS_COLS_V1_2 19 #define PG_STAT_STATEMENTS_COLS_V1_3 23
Re: Is it useful to record whether plans are generic or custom?
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?
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,diff --git a/contrib/pg_stat_statements/expected/pg_stat_statements.out b/contrib/pg_stat_statements/expected/pg_stat_statements.out index 16158525ca..887c4b2be8 100644 --- a/contrib/pg_stat_statements/expected/pg_stat_statements.out +++ b/contrib/pg_stat_statements/expected/pg_stat_statements.out @@ -251,6 +251,72 @@ FROM pg_stat_statements ORDER BY query COLLATE "C"; UPDATE pgss_test SET b = $1 WHERE a > $2 | 1 |3 | t | t | t (7 rows) +-- +-- Track the number of generic plan +-- +CREATE TABLE pgss_test (i int, j int, k int); +SELECT pg_stat_statements_reset(); + pg_stat_statements_reset +-- + +(1 row) + +SET plan_cache_mode TO force_generic_plan; +SET pg_stat_statements.track_utility = TRUE; +PREPARE pgss_p1 AS SELECT i FROM pgss_test WHERE i = $1; +EXECUTE pgss_p1(1); + i +--- +(0 rows) + +-- EXPLAIN ANALZE should be recorded +PREPARE pgss_p2 AS SELECT j FROM pgss_test WHERE j = $1; +EXPLAIN (ANALYZE, COSTS OFF, SUMMARY OFF, TIMING OFF) EXECUTE pgss_p2(1); + QUERY PLAN +--- + Seq Scan on pgss_test (actual rows=0 loops=1) + Filter: (j = $1) +(2 rows) + +-- Nested Portal +PREPARE pgss_p3 AS SELECT k FROM pgss_test WHERE k = $1; +BEGIN; +DECLARE pgss_c1 CURSOR FOR SELECT name FROM pg_prepared_statements; +FETCH IN pgss_c1; + name +- + pgss_p2 +(1 row) + +EXECUTE pgss_p3(1); + k +--- +(0 rows) + +FETCH IN pgss_c1; + name +- + pgss_p1 +(1 row) + +COMMIT; +SELECT calls, generic_calls, query FROM pg_stat_statements; + calls | generic_calls | query +---+---+-- + 1 | 0 | DECLARE pgss_c1 CURSOR FOR SELECT name FROM pg_prepared_statements + 0 | 0 | SELECT calls, generic_calls, query FROM pg_stat_statements + 1 | 1 | PREPARE pgss_p1 AS SELECT i FROM pgss_test WHERE i = $1 + 2 | 0 | FETCH IN pgss_c1 + 1 | 0 | BEGIN + 1 | 0 | SELECT pg_stat_statements_reset() + 1 | 1 | EXPLAIN (ANALYZE, COSTS OFF, SUMMARY OFF, TIMING OFF) EXECUTE
Re: Is it useful to record whether plans are generic or custom?
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 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 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?
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?
On 2021-02-04 11:19, Kyotaro Horiguchi wrote:
At Thu, 04 Feb 2021 10:16:47 +0900, torikoshia
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
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 Torikoshidiff --git a/contrib/pg_stat_statements/pg_stat_statements--1.7--1.8.sql b/contrib/pg_stat_statements/pg_stat_statements--1.7--1.8.sql
index 0f63f08f7e..7fdef315ae 100644
--- a/contrib/pg_stat_statements/pg_stat_statements--1.7--1.8.sql
+++ b/contrib/pg_stat_statements/pg_stat_statements--1.7--1.8.sql
@@ -44,7 +44,8 @@ CREATE FUNCTION pg_stat_statements(IN showtext boolean,
OUT blk_write_time float8,
OUT wal_records int8,
OUT wal_fpi int8,
-OUT wal_bytes numeric
+OUT wal_bytes numeric,
+OUT generic_calls int8
)
RETURNS SETOF record
AS 'MODULE_PATHNAME', 'pg_stat_statements_1_8'
diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index 62cccbfa44..f5801016d6 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -77,10 +77,12 @@
#include "storage/fd.h"
#include "storage/ipc.h"
#include "storage/spin.h"
+#include "tcop/pquery.h"
#include "tcop/utility.h"
#include "utils/acl.h"
#include "utils/builtins.h"
#include "utils/memutils.h"
+#include "utils/plancache.h"
#include "utils/timestamp.h"
PG_MODULE_MAGIC;
@@ -192,6 +194,7 @@ typedef struct Counters
int64 wal_records; /* # of WAL records generated */
int64 wal_fpi; /* # of WAL full page images generated */
uint64 wal_bytes; /* total amount of WAL generated in bytes */
+ int64 generic_calls; /* # of times generic plans executed */
} Counters;
/*
@@ -277,6 +280,10 @@ static int exec_nested_level = 0;
/* Current nesting depth of planner calls */
static int plan_nested_level = 0;
+/* Current and previous plan type */
+static bool is_plan_type_generic = false;
+static bool is_prev_plan_type_generic = false;
+
/* Saved hook values in case of unload */
static shmem_startup_hook_type prev_shmem_startup_hook = NULL;
static post_parse_analyze_hook_type prev_post_parse_analyze_hook = NULL;
@@ -1034,6 +1041,23 @@ pgss_ExecutorStart(QueryDesc *queryDesc, int eflags)
*/
if (pgss_enabled(exec_nested_level) && queryDesc->plannedstmt->queryId != UINT64CONST(0))
{
+ /*
+ * Since ActivePortal is not available at ExecutorEnd, we preserve
+ * the current and previous plan type here.
+ * Previous one is necessary since portals can be nested.
+ */
+ Assert(ActivePortal);
+
+ is_prev_plan_type_generic = is_plan_type_generic;
+
+ if (ActivePortal->cplan)
+ {
+ if (ActivePortal->cplan->is_generic)
+is_plan_type_generic = true;
+ else
+is_plan_type_generic = false;
+ }
+
/*
* Set up to track total elapsed time in ExecutorRun. Make sure the
* space is allocated in the per-query context so it will go away at
@@ -1122,6 +1146,9 @@ pgss_ExecutorEnd(QueryDesc *queryDesc)
NULL);
}
+ /* Storing has done. Set is_plan_type_generic back to the original. */
+ is_plan_type_generic = is_prev_plan_type_generic;
+
if (prev_ExecutorEnd)
prev_ExecutorEnd(queryDesc);
else
@@ -1427,6 +1454,8 @@ pgss_store(const char *query, uint64 queryId,
e->counters.max_time[kind] = total_time;
e->counters.mean_time[kind] = total_time;
}
+ else if (kind == PGSS_EXEC && is_plan_type_generic)
+ e->counters.generic_calls += 1;
else
{
/*
@@ -1510,8 +1539,8 @@ pg_stat_statements_reset(PG_FUNCTION_ARGS)
#define PG_STAT_STATEMENTS_COLS_V1_1 18
#define PG_STAT_STATEMENTS_COLS_V1_2 19
#define PG_STAT_STATEMENTS_COLS_V1_3 23
-#define PG_STAT_STATEMENTS_COLS_V1_8 32
-#define PG_STAT_STATEMENTS_COLS 32 /* maximum of above */
+#define PG_STAT_STATEMENTS_COLS_V1_8 33
+#define
Re: Is it useful to record whether plans are generic or custom?
At Thu, 04 Feb 2021 10:16:47 +0900, torikoshia 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 > 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?
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 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?
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?
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?
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?
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?
At Tue, 12 Jan 2021 20:36:58 +0900, torikoshia 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?
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?
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 Torikoshidiff --git a/contrib/pg_stat_statements/pg_stat_statements--1.7--1.8.sql b/contrib/pg_stat_statements/pg_stat_statements--1.7--1.8.sql
index 0f63f08f7e..7fdef315ae 100644
--- a/contrib/pg_stat_statements/pg_stat_statements--1.7--1.8.sql
+++ b/contrib/pg_stat_statements/pg_stat_statements--1.7--1.8.sql
@@ -44,7 +44,8 @@ CREATE FUNCTION pg_stat_statements(IN showtext boolean,
OUT blk_write_time float8,
OUT wal_records int8,
OUT wal_fpi int8,
-OUT wal_bytes numeric
+OUT wal_bytes numeric,
+OUT generic_calls int8
)
RETURNS SETOF record
AS 'MODULE_PATHNAME', 'pg_stat_statements_1_8'
diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index 72a117fc19..171c39f857 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -77,10 +77,12 @@
#include "storage/fd.h"
#include "storage/ipc.h"
#include "storage/spin.h"
+#include "tcop/pquery.h"
#include "tcop/utility.h"
#include "utils/acl.h"
#include "utils/builtins.h"
#include "utils/memutils.h"
+#include "utils/plancache.h"
#include "utils/timestamp.h"
PG_MODULE_MAGIC;
@@ -192,6 +194,7 @@ typedef struct Counters
int64 wal_records; /* # of WAL records generated */
int64 wal_fpi; /* # of WAL full page images generated */
uint64 wal_bytes; /* total amount of WAL generated in bytes */
+ int64 generic_calls; /* # of times generic plans executed */
} Counters;
/*
@@ -277,6 +280,9 @@ static int exec_nested_level = 0;
/* Current nesting depth of planner calls */
static int plan_nested_level = 0;
+/* Current plan type */
+static bool is_plan_type_generic = false;
+
/* Saved hook values in case of unload */
static shmem_startup_hook_type prev_shmem_startup_hook = NULL;
static post_parse_analyze_hook_type prev_post_parse_analyze_hook = NULL;
@@ -1034,6 +1040,20 @@ pgss_ExecutorStart(QueryDesc *queryDesc, int eflags)
*/
if (pgss_enabled(exec_nested_level) && queryDesc->plannedstmt->queryId != UINT64CONST(0))
{
+ /*
+ * Since ActivePortal is not available at ExecutorEnd, we preserve
+ * the plan type here.
+ */
+ Assert(ActivePortal);
+
+ if (ActivePortal->cplan)
+ {
+ if (ActivePortal->cplan->is_generic)
+is_plan_type_generic = true;
+ else
+is_plan_type_generic = false;
+ }
+
/*
* Set up to track total elapsed time in ExecutorRun. Make sure the
* space is allocated in the per-query context so it will go away at
@@ -1427,6 +1447,8 @@ pgss_store(const char *query, uint64 queryId,
e->counters.max_time[kind] = total_time;
e->counters.mean_time[kind] = total_time;
}
+ else if (kind == PGSS_EXEC && is_plan_type_generic)
+ e->counters.generic_calls += 1;
else
{
/*
@@ -1510,8 +1532,8 @@ pg_stat_statements_reset(PG_FUNCTION_ARGS)
#define PG_STAT_STATEMENTS_COLS_V1_1 18
#define PG_STAT_STATEMENTS_COLS_V1_2 19
#define PG_STAT_STATEMENTS_COLS_V1_3 23
-#define
Re: Is it useful to record whether plans are generic or custom?
At Fri, 04 Dec 2020 15:03:25 +0900, torikoshia 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?
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?
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?
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?
út 17. 11. 2020 v 15:21 odesílatel torikoshia 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?
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?
čt 12. 11. 2020 v 2:50 odesílatel torikoshia 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?
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?
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?
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.466006995
min_exec_time | 0.0293760003
max_exec_time | 0.237413
mean_exec_time | 0.077667834
stddev_exec_time| 0.07254973134206326
generic_calls | 1
total_generic_time | 0.0453340006
min_generic_time| 0.0453340006
max_generic_time| 0.0453340006
mean_generic_time | 0.0453340006
stddev_generic_time | 0
custom_calls| 5
total_custom_time | 0.420672996
min_custom_time | 0.0293760003
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 Torikoshidiff --git a/contrib/pg_stat_statements/pg_stat_statements--1.7--1.8.sql b/contrib/pg_stat_statements/pg_stat_statements--1.7--1.8.sql
index 0f63f08f7e..d118422b2a 100644
--- a/contrib/pg_stat_statements/pg_stat_statements--1.7--1.8.sql
+++ b/contrib/pg_stat_statements/pg_stat_statements--1.7--1.8.sql
@@ -29,6 +29,18 @@ CREATE FUNCTION pg_stat_statements(IN showtext boolean,
OUT max_exec_time float8,
OUT mean_exec_time float8,
OUT stddev_exec_time float8,
+OUT generic_calls int8,
+OUT total_generic_time float8,
+OUT min_generic_time float8,
+OUT max_generic_time float8,
+OUT mean_generic_time float8,
+OUT stddev_generic_time float8,
+OUT custom_calls int8,
+OUT total_custom_time float8,
+OUT min_custom_time float8,
+OUT max_custom_time float8,
+OUT mean_custom_time float8,
+OUT stddev_custom_time float8,
OUT rows int8,
OUT shared_blks_hit int8,
OUT shared_blks_read int8,
diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index 1eac9edaee..73d82a632d 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -78,9 +78,11 @@
#include "storage/ipc.h"
#include "storage/spin.h"
#include "tcop/utility.h"
+#include "tcop/pquery.h"
#include "utils/acl.h"
#include "utils/builtins.h"
#include "utils/memutils.h"
+#include "utils/plancache.h"
PG_MODULE_MAGIC;
@@ -138,6 +140,8 @@ typedef enum pgssStoreKind
*/
PGSS_PLAN = 0,
PGSS_EXEC,
+ PGSS_EXEC_GENERAL,
+ PGSS_EXEC_CUSTOM,
PGSS_NUMKIND/* Must be last value of this enum */
} pgssStoreKind;
@@ -258,6 +262,13 @@ typedef struct pgssJumbleState
int highest_extern_param_id;
} pgssJumbleState;
+typedef enum pgssPlanType
+{
+ PGSS_PLAN_TYPE_NONE,
+ PGSS_PLAN_TYPE_GENERIC,
+ PGSS_PLAN_TYPE_CUSTOM,
+} pgssPlanType;
+
/* Local variables */
/* Current nesting depth of ExecutorRun+ProcessUtility calls */
@@ -266,6 +277,9 @@ static int exec_nested_level = 0;
/* Current nesting depth of planner calls */
static int plan_nested_level = 0;
+/* Current plan type */
+static pgssPlanType plantype = PGSS_PLAN_TYPE_NONE;
+
/* Saved hook values in case of unload */
static shmem_startup_hook_type prev_shmem_startup_hook = NULL;
static post_parse_analyze_hook_type prev_post_parse_analyze_hook = NULL;
@@ -1013,6 +1027,20 @@ pgss_ExecutorStart(QueryDesc
Re: Is it useful to record whether plans are generic or custom?
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 signature.asc Description: PGP signature
RE: Is it useful to record whether plans are generic or custom?
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?
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, 1 or 2 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?
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 CORPORATIONFrom 793eafad8e988b6754c9d89e0ea14b64b07eef81 Mon Sep 17 00:00:00 2001 From: Atsushi Torikoshi Date: Fri, 31 Jul 2020 17:52:14 +0900 Subject: [PATCH] Previously the number of custom and generic plans are recoreded only in pg_prepared_statements, meaning we could only track them regarding current session. This patch records them in pg_stat_statements and it enables to track them regarding all sessions of the PostgreSQL instance. --- .../pg_stat_statements--1.6--1.7.sql | 5 ++- .../pg_stat_statements--1.7--1.8.sql | 1 + .../pg_stat_statements/pg_stat_statements.c | 44 +++ src/backend/utils/cache/plancache.c | 2 + src/include/utils/plancache.h | 1 + 5 files changed, 42 insertions(+), 11 deletions(-) diff --git a/contrib/pg_stat_statements/pg_stat_statements--1.6--1.7.sql b/contrib/pg_stat_statements/pg_stat_statements--1.6--1.7.sql index 6fc3fed4c9..fd7aa05c92 100644 --- a/contrib/pg_stat_statements/pg_stat_statements--1.6--1.7.sql +++ b/contrib/pg_stat_statements/pg_stat_statements--1.6--1.7.sql @@ -12,11 +12,12 @@ DROP FUNCTION pg_stat_statements_reset(); /* Now redefine */ CREATE FUNCTION pg_stat_statements_reset(IN userid Oid DEFAULT 0, IN dbid Oid DEFAULT 0, - IN queryid bigint DEFAULT 0 + IN queryid bigint DEFAULT 0, + IN generic_plan int DEFAULT -1 ) RETURNS void AS 'MODULE_PATHNAME', 'pg_stat_statements_reset_1_7' LANGUAGE C STRICT PARALLEL SAFE; -- Don't want this to be available to non-superusers. -REVOKE ALL ON FUNCTION pg_stat_statements_reset(Oid, Oid, bigint) FROM PUBLIC; +REVOKE ALL ON FUNCTION pg_stat_statements_reset(Oid, Oid, bigint, int) FROM PUBLIC; diff --git a/contrib/pg_stat_statements/pg_stat_statements--1.7--1.8.sql b/contrib/pg_stat_statements/pg_stat_statements--1.7--1.8.sql index 0f63f08f7e..0d7c4e7343 100644 --- a/contrib/pg_stat_statements/pg_stat_statements--1.7--1.8.sql +++ b/contrib/pg_stat_statements/pg_stat_statements--1.7--1.8.sql @@ -15,6 +15,7 @@ DROP FUNCTION pg_stat_statements(boolean); CREATE FUNCTION pg_stat_statements(IN showtext boolean, OUT userid oid, OUT dbid oid, +OUT generic_plan bool, OUT queryid bigint, OUT query text, OUT plans int8, diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c index 6b91c62c31..14c580a95e 100644 --- a/contrib/pg_stat_statements/pg_stat_statements.c +++ b/contrib/pg_stat_statements/pg_stat_statements.c @@ -78,9 +78,11 @@ #include "storage/ipc.h" #include "storage/spin.h" #include "tcop/utility.h" +#include "tcop/pquery.h" #include "utils/acl.h" #include "utils/builtins.h" #include "utils/memutils.h" +#include "utils/plancache.h" PG_MODULE_MAGIC; @@ -156,6 +158,7 @@ typedef struct pgssHashKey Oid userid; /* user OID */ Oid dbid; /* database OID */ uint64 queryid; /* query identifier */ + bool is_generic_plan; } pgssHashKey; /* @@ -266,6 +269,9 @@ static int exec_nested_level = 0; /* Current nesting depth of planner calls */ static
RE: Is it useful to record whether plans are generic or custom?
>> 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, 1 or 2 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?
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?
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 CORPORATIONFrom 793eafad8e988b6754c9d89e0ea14b64b07eef81 Mon Sep 17 00:00:00 2001 From: Atsushi Torikoshi Date: Wed, 22 Jul 2020 16:00:04 +0900 Subject: [PATCH] [poc] Previously the number of custom and generic plans are recoreded only in pg_prepared_statements, meaning we could only track them regarding current session. This patch records them in pg_stat_statements and it enables to track them regarding all sessions of the PostgreSQL instance. --- .../pg_stat_statements--1.6--1.7.sql | 3 +- .../pg_stat_statements--1.7--1.8.sql | 1 + .../pg_stat_statements/pg_stat_statements.c | 44 +++ src/backend/utils/cache/plancache.c | 2 + src/include/utils/plancache.h | 1 + 5 files changed, 41 insertions(+), 10 deletions(-) diff --git a/contrib/pg_stat_statements/pg_stat_statements--1.6--1.7.sql b/contrib/pg_stat_statements/pg_stat_statements--1.6--1.7.sql index 6fc3fed4c9..5ab0a26b77 100644 --- a/contrib/pg_stat_statements/pg_stat_statements--1.6--1.7.sql +++ b/contrib/pg_stat_statements/pg_stat_statements--1.6--1.7.sql @@ -12,7 +12,8 @@ DROP FUNCTION pg_stat_statements_reset(); /* Now redefine */ CREATE FUNCTION pg_stat_statements_reset(IN userid Oid DEFAULT 0, IN dbid Oid DEFAULT 0, - IN queryid bigint DEFAULT 0 + IN queryid bigint DEFAULT 0, + IN generic_plan int DEFAULT -1 ) RETURNS void AS 'MODULE_PATHNAME', 'pg_stat_statements_reset_1_7' diff --git a/contrib/pg_stat_statements/pg_stat_statements--1.7--1.8.sql b/contrib/pg_stat_statements/pg_stat_statements--1.7--1.8.sql index 0f63f08f7e..0d7c4e7343 100644 --- a/contrib/pg_stat_statements/pg_stat_statements--1.7--1.8.sql +++ b/contrib/pg_stat_statements/pg_stat_statements--1.7--1.8.sql @@ -15,6 +15,7 @@ DROP FUNCTION pg_stat_statements(boolean); CREATE FUNCTION pg_stat_statements(IN showtext boolean, OUT userid oid, OUT dbid oid, +OUT generic_plan bool, OUT queryid bigint, OUT query text, OUT plans int8, diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c index 14cad19afb..5d74dc04cd 100644 --- a/contrib/pg_stat_statements/pg_stat_statements.c +++ b/contrib/pg_stat_statements/pg_stat_statements.c @@ -78,9 +78,11 @@ #include "storage/ipc.h" #include "storage/spin.h" #include "tcop/utility.h" +#include "tcop/pquery.h" #include "utils/acl.h" #include "utils/builtins.h" #include "utils/memutils.h" +#include "utils/plancache.h" PG_MODULE_MAGIC; @@ -156,6 +158,7 @@ typedef struct pgssHashKey Oid userid; /* user OID */ Oid dbid; /* database OID */ uint64 queryid; /* query identifier */ + bool is_generic_plan; } pgssHashKey; /* @@ -266,6 +269,9 @@ static int exec_nested_level = 0; /* Current nesting depth of planner calls */ static int plan_nested_level = 0; +/* Current plan type */ +static bool is_generic_plan = false; + /* Saved hook values in case of unload */ static shmem_startup_hook_type prev_shmem_startup_hook = NULL; static post_parse_analyze_hook_type prev_post_parse_analyze_hook = NULL; @@ -367,7 +373,7 @@ static char *qtext_fetch(Size query_offset, int query_len, char *buffer, Size buffer_size); static bool need_gc_qtexts(void); static void gc_qtexts(void); -static void entry_reset(Oid userid, Oid dbid, uint64 queryid); +static void entry_reset(Oid userid, Oid dbid, uint64 queryid, int generic_plan); static void AppendJumble(pgssJumbleState *jstate,
Re: Is it useful to record whether plans are generic or custom?
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.
+ role="column_definition">
+ last_plan_type
text
+
+
+ Tells the last plan type was generic or custom. If the
prepared
+ statement has not executed yet, this field is null
+
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?
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.
+
+ last_plan_type text
+
+
+ Tells the last plan type was generic or custom. If the prepared
+ statement has not executed yet, this field is null
+
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?
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.
+
+ last_plan_type text
+
+
+ Tells the last plan type was generic or custom. If the prepared
+ statement has not executed yet, this field is null
+
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
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index e9cdff4864..de69487bec 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -10830,6 +10830,24 @@ SELECT * FROM pg_locks pl LEFT JOIN pg_prepared_xacts
ppx
frontend/backend protocol
+
+
+
+ generic_plans int8
+
+
+ Number of times generic plan was chosen
+
+
+
+
+
+ custom_plans int8
+
+
+ Number of times custom plan was chosen
+
+
diff --git a/src/backend/commands/prepare.c b/src/backend/commands/prepare.c
index 80d6df8ac1..4b18be5b27 100644
--- a/src/backend/commands/prepare.c
+++ b/src/backend/commands/prepare.c
@@ -694,7 +694,8 @@ ExplainExecuteQuery(ExecuteStmt *execstmt, IntoClause
*into, ExplainState *es,
/*
* This set returning function reads all the prepared statements and
- * returns a set of (name,
Re: Is it useful to record whether plans are generic or custom?
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.
+ role="column_definition">
+ last_plan_type
text
+
+
+ Tells the last plan type was generic or custom. If the
prepared
+ statement has not executed yet, this field is null
+
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 CORPORATIONFrom e3517479cea76e88f3ab8626d14452b36288dcb5 Mon Sep 17 00:00:00 2001
From: Atsushi Torikoshi
Date: Tue, 14 Jul 2020 10:27:32 +0900
Subject: [PATCH] We didn't have an easy way to find how many times generic and
ustom plans of a prepared statement have been executed. This patch exposes
the numbers of both plans in pg_prepared_statements.
---
doc/src/sgml/catalogs.sgml | 18
src/backend/commands/prepare.c | 15 +++---
src/backend/utils/cache/plancache.c | 17
src/include/catalog/pg_proc.dat | 6 ++--
src/include/utils/plancache.h | 3 +-
src/test/regress/expected/plancache.out | 37 -
src/test/regress/expected/rules.out | 6 ++--
src/test/regress/sql/plancache.sql | 12 +++-
8 files changed, 96 insertions(+), 18 deletions(-)
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index e9cdff4864..8bc6d685f4 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -10830,6 +10830,24 @@ SELECT * FROM pg_locks pl LEFT JOIN pg_prepared_xacts ppx
frontend/backend protocol
+
+
+
+ generic_plans int8
+
+
+Number of times generic plan was chosen
+
+
+
+
+
+ custom_plans int8
+
+
+Number of times custom plan was chosen
+
+
diff --git a/src/backend/commands/prepare.c b/src/backend/commands/prepare.c
index 80d6df8ac1..4b18be5b27 100644
--- a/src/backend/commands/prepare.c
+++ b/src/backend/commands/prepare.c
@@ -694,7 +694,8 @@ ExplainExecuteQuery(ExecuteStmt *execstmt, IntoClause *into, ExplainState *es,
/*
* This set returning function reads all the prepared statements and
- * returns a set of (name, statement, prepare_time, param_types, from_sql).
+ * returns a set of (name, statement, prepare_time, param_types, from_sql,
+ * generic_plans, custom_plans).
*/
Datum
Re: Is it useful to record whether plans are generic or custom?
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.
+
+ last_plan_type text
+
+
+ Tells the last plan type was generic or custom. If the prepared
+ statement has not executed yet, this field is null
+
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?
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.
+ role="column_definition">
+ last_plan_type text
+
+
+ Tells the last plan type was generic or custom. If the
prepared
+ statement has not executed yet, this field is null
+
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 CORPORATIONFrom e417fc58d51ab0c06de34a563d580c7d4017a1db Mon Sep 17 00:00:00 2001
From: Atsushi Torikoshi
Date: Tue, 14 Jul 2020 20:57:16 +0900
Subject: [PATCH] We didn't have an easy way to find how many times generic and
custom plans of a prepared statement have been executed. This patch exposes
the numbers of both plans in pg_prepared_statements.
---
doc/src/sgml/catalogs.sgml| 18 ++
src/backend/commands/prepare.c| 15 +---
src/backend/utils/cache/plancache.c | 17 +
src/include/catalog/pg_proc.dat | 6 ++--
src/include/utils/plancache.h | 3 +-
src/test/regress/expected/prepare.out | 51 +++
src/test/regress/expected/rules.out | 6 ++--
src/test/regress/sql/prepare.sql | 15
8 files changed, 115 insertions(+), 16 deletions(-)
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index e9cdff4864..8bc6d685f4 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -10830,6 +10830,24 @@ SELECT * FROM pg_locks pl LEFT JOIN pg_prepared_xacts ppx
frontend/backend protocol
+
+
+
+ generic_plans int8
+
+
+Number of times generic plan was chosen
+
+
+
+
+
+ custom_plans int8
+
+
+Number of times custom plan was chosen
+
+
diff --git a/src/backend/commands/prepare.c b/src/backend/commands/prepare.c
index 80d6df8ac1..4b18be5b27 100644
--- a/src/backend/commands/prepare.c
+++ b/src/backend/commands/prepare.c
@@ -694,7 +694,8 @@ ExplainExecuteQuery(ExecuteStmt *execstmt, IntoClause *into, ExplainState *es,
/*
* This set returning function reads all the prepared statements and
- * returns a set of (name, statement, prepare_time, param_types, from_sql).
+ * returns a set of (name, statement, prepare_time, param_types, from_sql,
+ * generic_plans, custom_plans).
*/
Datum
pg_prepared_statement(PG_FUNCTION_ARGS)
@@ -723,7 +724,7 @@ pg_prepared_statement(PG_FUNCTION_ARGS)
* build tupdesc for result tuples. This must match the definition of the
* pg_prepared_statements view in system_views.sql
*/
- tupdesc = CreateTemplateTupleDesc(5);
+ tupdesc = CreateTemplateTupleDesc(7);
TupleDescInitEntry(tupdesc, (AttrNumber) 1, "name",
TEXTOID, -1, 0);
TupleDescInitEntry(tupdesc, (AttrNumber) 2, "statement",
@@ -734,6 +735,10 @@ pg_prepared_statement(PG_FUNCTION_ARGS)
REGTYPEARRAYOID, -1, 0);
Re: Is it useful to record whether plans are generic or custom?
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.
+ role="column_definition">
+ last_plan_type text
+
+
+ Tells the last plan type was generic or custom. If the
prepared
+ statement has not executed yet, this field is null
+
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?
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.
+
+ last_plan_type text
+
+
+ Tells the last plan type was generic or custom. If the prepared
+ statement has not executed yet, this field is null
+
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?
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.
+ role="column_definition">
+ last_plan_type text
+
+
+Tells the last plan type was generic or custom. If the
prepared
+statement has not executed yet, this field is null
+
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 CORPORATIONFrom f2155b1a9f3b500eca490827dd044f5c0f704dd3 Mon Sep 17 00:00:00 2001
From: Atsushi Torikoshi
Date: Wed, 8 Jul 2020 09:17:22 +0900
Subject: [PATCH] We didn't have an easy way to find how many times generic or
custom plans of a prepared statement have been executed. This patch exposes
such numbers of both plans and the last plan type in pg_prepared_statements.
From 08183ee7ef827761f1ed38d2ab62b4f8f748baca Mon Sep 17 00:00:00 2001
---
doc/src/sgml/catalogs.sgml| 28 +++
src/backend/commands/prepare.c| 29 ---
src/backend/utils/cache/plancache.c | 23
src/include/catalog/pg_proc.dat | 6 ++--
src/include/utils/plancache.h | 12 ++-
src/test/regress/expected/prepare.out | 51 +++
src/test/regress/expected/rules.out | 7 ++--
src/test/regress/sql/prepare.sql | 15
8 files changed, 155 insertions(+), 16 deletions(-)
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 003d278370..146dc2208b 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -10829,6 +10829,34 @@ SELECT * FROM pg_locks pl LEFT JOIN pg_prepared_xacts ppx
frontend/backend protocol
+
+
+
+ generic_plans bigint
+
+
+Number of times generic plan was chosen
+
+
+
+
+
+ custom_plans bigint
+
+
+Number of times custom plan was chosen
+
+
+
+
+
+ last_plan_type text
+
+
+Tells the last plan type was generic or custom. If the prepared
+statement has not executed yet, this field is null
+
+
diff --git a/src/backend/commands/prepare.c b/src/backend/commands/prepare.c
index 80d6df8ac1..b4b1865d48 100644
--- a/src/backend/commands/prepare.c
+++ b/src/backend/commands/prepare.c
@@ -694,7 +694,8 @@ ExplainExecuteQuery(ExecuteStmt *execstmt, IntoClause *into, ExplainState *es,
/*
* This set returning function reads all the prepared statements and
- * returns a set of (name, statement, prepare_time, param_types, from_sql).
+ * returns a set of (name, statement, prepare_time, param_types, from_sql,
+ * generic_plans, custom_plans, last_plan_type).
*/
Datum
pg_prepared_statement(PG_FUNCTION_ARGS)
@@ -723,7 +724,7 @@ pg_prepared_statement(PG_FUNCTION_ARGS)
* build tupdesc for result tuples. This must match the definition of the
* pg_prepared_statements view in system_views.sql
*/
- tupdesc = CreateTemplateTupleDesc(5);
+ tupdesc = CreateTemplateTupleDesc(8);
TupleDescInitEntry(tupdesc, (AttrNumber) 1, "name",
TEXTOID, -1, 0);
TupleDescInitEntry(tupdesc, (AttrNumber) 2, "statement",
@@ -734,6 +735,12 @@ pg_prepared_statement(PG_FUNCTION_ARGS)
REGTYPEARRAYOID, -1, 0);
TupleDescInitEntry(tupdesc, (AttrNumber) 5, "from_sql",
BOOLOID, -1, 0);
+ TupleDescInitEntry(tupdesc, (AttrNumber) 6, "generic_plans",
+ INT8OID, -1, 0);
+ TupleDescInitEntry(tupdesc,
Re: Is it useful to record whether plans are generic or custom?
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"?
+
+ last_plan_type text
+
+
+Tells the last plan type was generic or custom. If the prepared
+statement has not executed yet, this field is null
+
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?
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 TorikoshiFrom f2155b1a9f3b500eca490827dd044f5c0f704dd3 Mon Sep 17 00:00:00 2001
From: Atsushi Torikoshi
Date: Thu, 11 Jun 2020 11:35:25 +0900
Subject: [PATCH] We didn't have an easy way to find how many times generic or
custom plans of a prepared statement have been executed. This patch exposes
such numbers of both plans and the last plan type in pg_prepared_statements.
fixed comments
---
doc/src/sgml/catalogs.sgml| 28 +++
src/backend/commands/prepare.c| 29 ---
src/backend/utils/cache/plancache.c | 23
src/include/catalog/pg_proc.dat | 6 ++--
src/include/utils/plancache.h | 12 ++-
src/test/regress/expected/prepare.out | 51 +++
src/test/regress/expected/rules.out | 7 ++--
src/test/regress/sql/prepare.sql | 15
8 files changed, 155 insertions(+), 16 deletions(-)
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 700271fd40..ac9e56d5b3 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -10829,6 +10829,34 @@ SELECT * FROM pg_locks pl LEFT JOIN pg_prepared_xacts ppx
frontend/backend protocol
+
+
+
+ generic_plans bigint
+
+
+Number of times generic plan was choosen
+
+
+
+
+
+ custom_plans bigint
+
+
+Number of times custom plan was choosen
+
+
+
+
+
+ last_plan_type text
+
+
+Tells the last plan type was generic or custom. If the prepared
+statement has not executed yet, this field is null
+
+
diff --git a/src/backend/commands/prepare.c b/src/backend/commands/prepare.c
index 80d6df8ac1..b4b1865d48 100644
--- a/src/backend/commands/prepare.c
+++ b/src/backend/commands/prepare.c
@@ -694,7 +694,8 @@ ExplainExecuteQuery(ExecuteStmt *execstmt, IntoClause *into, ExplainState *es,
/*
* This set returning function reads all the prepared statements and
- * returns a set of (name, statement, prepare_time, param_types, from_sql).
+ * returns a set of (name, statement, prepare_time, param_types, from_sql,
+ * generic_plans, custom_plans, last_plan_type).
*/
Datum
pg_prepared_statement(PG_FUNCTION_ARGS)
@@ -723,7 +724,7 @@ pg_prepared_statement(PG_FUNCTION_ARGS)
* build tupdesc for result tuples. This must match the definition of the
* pg_prepared_statements view in system_views.sql
*/
- tupdesc = CreateTemplateTupleDesc(5);
+ tupdesc = CreateTemplateTupleDesc(8);
TupleDescInitEntry(tupdesc, (AttrNumber) 1, "name",
TEXTOID, -1, 0);
TupleDescInitEntry(tupdesc, (AttrNumber) 2, "statement",
@@ -734,6 +735,12 @@ pg_prepared_statement(PG_FUNCTION_ARGS)
REGTYPEARRAYOID, -1, 0);
TupleDescInitEntry(tupdesc, (AttrNumber) 5, "from_sql",
BOOLOID, -1, 0);
+ TupleDescInitEntry(tupdesc, (AttrNumber) 6, "generic_plans",
+ INT8OID, -1, 0);
+ TupleDescInitEntry(tupdesc, (AttrNumber) 7, "custom_plans",
+ INT8OID, -1, 0);
+ TupleDescInitEntry(tupdesc, (AttrNumber) 8, "last_plan_type",
+ TEXTOID, -1, 0);
/*
* We put all the tuples into a tuplestore in one scan of the hashtable.
@@ -755,8 +762,8 @@ pg_prepared_statement(PG_FUNCTION_ARGS)
hash_seq_init(_seq, prepared_queries);
while ((prep_stmt = hash_seq_search(_seq)) != NULL)
{
- Datum values[5];
- bool nulls[5];
+ Datum values[8];
+ bool nulls[8];
MemSet(nulls, 0, sizeof(nulls));
@@ -766,6 +773,20 @@ pg_prepared_statement(PG_FUNCTION_ARGS)
values[3] = build_regtype_array(prep_stmt->plansource->param_types,
prep_stmt->plansource->num_params);
values[4] = BoolGetDatum(prep_stmt->from_sql);
+ values[5] = Int64GetDatumFast(prep_stmt->plansource->num_generic_plans);
+ values[6] = Int64GetDatumFast(prep_stmt->plansource->num_custom_plans);
+
+ switch (prep_stmt->plansource->last_plan_type)
+ {
+case PLAN_CACHE_TYPE_CUSTOM:
+ values[7] = CStringGetTextDatum("custom");
+ break;
+case PLAN_CACHE_TYPE_GENERIC:
+ values[7] = CStringGetTextDatum("generic");
Re: Is it useful to record whether plans are generic or custom?
At Wed, 10 Jun 2020 10:50:58 +0900, torikoshia
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?
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 TorikoshiFrom f2155b1a9f3b500eca490827dd044f5c0f704dd3 Mon Sep 17 00:00:00 2001
From: Atsushi Torikoshi
Date: Tue, 10 Jun 2020 10:22:20 +0900
Subject: [PATCH] We didn't have an easy way to find how many times generic or
custom plans of a prepared statement have been executed. This patch exposes
such numbers of both plans and the last plan type in pg_prepared_statements.
---
doc/src/sgml/catalogs.sgml| 28 +++
src/backend/commands/prepare.c| 24 ++---
src/backend/utils/cache/plancache.c | 23
src/include/catalog/pg_proc.dat | 6 ++--
src/include/utils/plancache.h | 12 ++-
src/test/regress/expected/prepare.out | 51 +++
src/test/regress/expected/rules.out | 7 ++--
src/test/regress/sql/prepare.sql | 15
8 files changed, 150 insertions(+), 16 deletions(-)
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 700271fd40..9e96a5518e 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -10829,6 +10829,34 @@ SELECT * FROM pg_locks pl LEFT JOIN pg_prepared_xacts ppx
frontend/backend protocol
+
+
+
+ generic_plans bigint
+
+
+Number of times generic plan was choosen
+
+
+
+
+
+ custom_plans bigint
+
+
+Number of times custom plan was choosen
+
+
+
+
+
+ last_plan text
+
+
+Tells the last plan was generic or custom. If the prepared
+statement has not executed yet, this field is null
+
+
diff --git a/src/backend/commands/prepare.c b/src/backend/commands/prepare.c
index 80d6df8ac1..d60cf6f67a 100644
--- a/src/backend/commands/prepare.c
+++ b/src/backend/commands/prepare.c
@@ -694,7 +694,8 @@ ExplainExecuteQuery(ExecuteStmt *execstmt, IntoClause *into, ExplainState *es,
/*
* This set returning function reads all the prepared statements and
- * returns a set of (name, statement, prepare_time, param_types, from_sql).
+ * returns a set of (name, statement, prepare_time, param_types, from_sql,
+ * generic_plans, custom_plans, last_plan).
*/
Datum
pg_prepared_statement(PG_FUNCTION_ARGS)
@@ -723,7 +724,7 @@ pg_prepared_statement(PG_FUNCTION_ARGS)
* build tupdesc for result tuples. This must match the definition of the
* pg_prepared_statements view in system_views.sql
*/
- tupdesc = CreateTemplateTupleDesc(5);
+ tupdesc = CreateTemplateTupleDesc(8);
TupleDescInitEntry(tupdesc, (AttrNumber) 1, "name",
TEXTOID, -1, 0);
TupleDescInitEntry(tupdesc, (AttrNumber) 2, "statement",
@@ -734,6 +735,12 @@ pg_prepared_statement(PG_FUNCTION_ARGS)
REGTYPEARRAYOID, -1, 0);
TupleDescInitEntry(tupdesc, (AttrNumber) 5, "from_sql",
BOOLOID, -1, 0);
+ TupleDescInitEntry(tupdesc, (AttrNumber) 6, "generic_plans",
+ INT8OID, -1, 0);
+ TupleDescInitEntry(tupdesc, (AttrNumber) 7, "custom_plans",
+ INT8OID, -1, 0);
+ TupleDescInitEntry(tupdesc, (AttrNumber) 8, "last_plan",
+ TEXTOID, -1, 0);
/*
* We put all the tuples into a tuplestore in one scan of the hashtable.
@@ -755,8 +762,8 @@ pg_prepared_statement(PG_FUNCTION_ARGS)
hash_seq_init(_seq, prepared_queries);
while ((prep_stmt = hash_seq_search(_seq)) != NULL)
{
- Datum values[5];
- bool nulls[5];
+ Datum values[8];
+ bool nulls[8];
MemSet(nulls, 0, sizeof(nulls));
@@ -766,6 +773,15 @@ pg_prepared_statement(PG_FUNCTION_ARGS)
values[3] = build_regtype_array(prep_stmt->plansource->param_types,
prep_stmt->plansource->num_params);
values[4] = BoolGetDatum(prep_stmt->from_sql);
+ values[5] = Int64GetDatumFast(prep_stmt->plansource->num_generic_plans);
+ values[6] = Int64GetDatumFast(prep_stmt->plansource->num_custom_plans);
+
+ if (prep_stmt->plansource->last_plan_type == PLAN_CACHE_TYPE_CUSTOM)
+values[7] = CStringGetTextDatum("custom");
+ else if (prep_stmt->plansource->last_plan_type == PLAN_CACHE_TYPE_GENERIC)
+values[7] =
Re: Is it useful to record whether plans are generic or custom?
On 2020-06-04 17:04, Atsushi Torikoshi wrote:
On Mon, May 25, 2020 at 10:54 AM Atsushi Torikoshi
wrote:
On Thu, May 21, 2020 at 5:10 PM Kyotaro Horiguchi
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?
On Mon, May 25, 2020 at 10:54 AM Atsushi Torikoshi wrote:
> On Thu, May 21, 2020 at 5:10 PM Kyotaro Horiguchi
> 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
>
0003-Expose-counters-of-plancache-to-pg_prepared_statement.patch
Description: Binary data
Re: Is it useful to record whether plans are generic or custom?
Thanks for writing a patch!
On Thu, May 21, 2020 at 5:10 PM Kyotaro Horiguchi
wrote:
> At Thu, 21 May 2020 12:18:16 +0900, Fujii Masao <
> masao.fu...@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.3105
> 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
> 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...@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
0002-Expose-counters-of-plancache-to-pg_prepared_statement.patch
Description: Binary data
Re: Is it useful to record whether plans are generic or custom?
Hi Torikoshi-san!
On 2020/05/21 17:10, Kyotaro Horiguchi wrote:
At Thu, 21 May 2020 12:18:16 +0900, Fujii Masao
wrote in
On 2020/05/20 21:56, Atsushi Torikoshi wrote:
On Wed, May 20, 2020 at 1:32 PM Kyotaro Horiguchi
mailto:horikyota@gmail.com>> wrote:
At Tue, 19 May 2020 22:56:17 +0900, Atsushi Torikoshi
mailto:ato...@gmail.com>> wrote in
> On Sat, May 16, 2020 at 6:01 PM legrand legrand
> mailto:legrand_legr...@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.3105
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
diff --git a/src/backend/commands/prepare.c b/src/backend/commands/prepare.c
index 80d6df8ac1..63de4fdb78 100644
--- a/src/backend/commands/prepare.c
+++ b/src/backend/commands/prepare.c
@@ -723,7 +723,7 @@ pg_prepared_statement(PG_FUNCTION_ARGS)
* build tupdesc for result tuples. This must match the definition of
the
* pg_prepared_statements view in system_views.sql
*/
- tupdesc = CreateTemplateTupleDesc(5);
+ tupdesc = CreateTemplateTupleDesc(6);
TupleDescInitEntry(tupdesc, (AttrNumber) 1, "name",
TEXTOID, -1, 0);
TupleDescInitEntry(tupdesc, (AttrNumber) 2, "statement",
@@ -734,6 +734,8 @@ pg_prepared_statement(PG_FUNCTION_ARGS)
REGTYPEARRAYOID, -1, 0);
TupleDescInitEntry(tupdesc, (AttrNumber) 5, "from_sql",
BOOLOID, -1, 0);
+ TupleDescInitEntry(tupdesc, (AttrNumber) 6, "is_generic_plan",
+ BOOLOID, -1, 0);
/*
* We put all the tuples into a tuplestore in one scan of the hashtable.
@@ -755,8 +757,8 @@ pg_prepared_statement(PG_FUNCTION_ARGS)
hash_seq_init(_seq, prepared_queries);
while ((prep_stmt = hash_seq_search(_seq)) != NULL)
{
- Datum values[5];
- boolnulls[5];
+ Datum values[6];
+ boolnulls[6];
MemSet(nulls, 0, sizeof(nulls));
@@ -766,6 +768,7 @@ pg_prepared_statement(PG_FUNCTION_ARGS)
values[3] =
build_regtype_array(prep_stmt->plansource->param_types,
Re: Is it useful to record whether plans are generic or custom?
At Thu, 21 May 2020 12:18:16 +0900, Fujii Masao
wrote in
>
>
> On 2020/05/20 21:56, Atsushi Torikoshi wrote:
> > On Wed, May 20, 2020 at 1:32 PM Kyotaro Horiguchi
> > mailto:horikyota@gmail.com>> wrote:
> > At Tue, 19 May 2020 22:56:17 +0900, Atsushi Torikoshi
> > mailto:ato...@gmail.com>> wrote in
> > > On Sat, May 16, 2020 at 6:01 PM legrand legrand
> > > mailto:legrand_legr...@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.3105
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 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
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
+
+
+
+ calls bigint
+
+
+ Number of times the prepared statement was executed
+
+
+
+
+
+ custom_calls bigint
+
+
+ Number of times generic plan is executed for the prepared statement
+
+
+
+
+
+ plan_geneation bigint
+
+
+ Number of times execution plan was generated for the prepared statement
+
+
+
+
+
+ generic_cost double precision
+
+
+ Estimated cost of generic plan. NULL if not yet planned.
+
+
+
+
+
+ custom_cost double precision
+
+
+ Estimated cost of custom plans.
Re: Is it useful to record whether plans are generic or custom?
On 2020/05/20 21:56, Atsushi Torikoshi wrote:
On Wed, May 20, 2020 at 1:32 PM Kyotaro Horiguchi mailto:horikyota@gmail.com>> wrote:
At Tue, 19 May 2020 22:56:17 +0900, Atsushi Torikoshi mailto:ato...@gmail.com>> wrote in
> On Sat, May 16, 2020 at 6:01 PM legrand legrand mailto:legrand_legr...@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?
On Wed, May 20, 2020 at 1:32 PM Kyotaro Horiguchi
wrote:
> At Tue, 19 May 2020 22:56:17 +0900, Atsushi Torikoshi
> wrote in
> > On Sat, May 16, 2020 at 6:01 PM legrand legrand <
> legrand_legr...@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?
At Tue, 19 May 2020 22:56:17 +0900, Atsushi Torikoshi wrote
in
> On Sat, May 16, 2020 at 6:01 PM legrand legrand
> 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?
On Sat, May 16, 2020 at 6:01 PM legrand legrand 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?
> 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?
On Thu, May 14, 2020 at 2:28 AM legrand legrand 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?
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
