On 2020-09-17 13:46, Michael Paquier wrote:
On Fri, Jul 31, 2020 at 06:47:48PM +0900, torikoshia wrote:
Oops, sorry about that.
I just fixed it there for now.
The regression tests of the patch look unstable, and the CF bot is
reporting a failure here:
https://travis-ci.org/github/postgresql-cfbot/postgresql/builds/727833416
--
Michael
Thank you for letting me know!
I'd like to reach a basic agreement on how we expose the
generic/custom plan information in pgss first.
Given the discussion so far, adding a new attribute to pgss key
is not appropriate since it can easily increase the number of
entries in pgss.
OTOH, just exposing the number of times generic/custom plan was
chosen seems not enough to know whether performance is degraded.
I'm now thinking about exposing not only the number of times
generic/custom plan was chosen but also some performance
metrics like 'total_time' for both generic and custom plans.
Attached a poc patch which exposes total, min, max, mean and
stddev time for both generic and custom plans.
=# SELECT * FROM =# SELECT * FROM pg_stat_statements;
-[ RECORD 1
]-------+---------------------------------------------------------
userid | 10
dbid | 12878
queryid | 4617094108938234366
query | PREPARE pr1 AS SELECT * FROM pg_class WHERE
relname = $1
plans | 0
total_plan_time | 0
min_plan_time | 0
max_plan_time | 0
mean_plan_time | 0
stddev_plan_time | 0
calls | 6
total_exec_time | 0.46600699999999995
min_exec_time | 0.029376000000000003
max_exec_time | 0.237413
mean_exec_time | 0.07766783333333334
stddev_exec_time | 0.07254973134206326
generic_calls | 1
total_generic_time | 0.045334000000000006
min_generic_time | 0.045334000000000006
max_generic_time | 0.045334000000000006
mean_generic_time | 0.045334000000000006
stddev_generic_time | 0
custom_calls | 5
total_custom_time | 0.42067299999999996
min_custom_time | 0.029376000000000003
max_custom_time | 0.237413
mean_custom_time | 0.0841346
stddev_custom_time | 0.07787966226583164
...
In this patch, exposing new columns is mandatory, but I think
it's better to make it optional by adding a GUC something
like 'pgss.track_general_custom_plans.
I also feel it makes the number of columns too many.
Just adding the total time may be sufficient.
Any thoughts?
Regards,
--
Atsushi Torikoshi
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..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 *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)
+ plantype = PGSS_PLAN_TYPE_GENERIC;
+ else
+ plantype = PGSS_PLAN_TYPE_CUSTOM;
+ }
+
/*
* 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
@@ -1100,6 +1128,8 @@ pgss_ExecutorEnd(QueryDesc *queryDesc)
&queryDesc->totaltime->walusage,
NULL);
}
+ /* Reset the plan type. */
+ plantype = PGSS_PLAN_TYPE_NONE;
if (prev_ExecutorEnd)
prev_ExecutorEnd(queryDesc);
@@ -1224,6 +1254,41 @@ pgss_hash_string(const char *str, int len)
len, 0));
}
+/* Note: Caller must acquire SpinLock on pgssEntry before using this function. */
+static void
+kind_dependent_accumulation(volatile pgssEntry *e, pgssStoreKind kind, double total_time)
+{
+ e->counters.calls[kind] += 1;
+ e->counters.total_time[kind] += total_time;
+
+ if (e->counters.calls[kind] == 1)
+ {
+ e->counters.min_time[kind] = total_time;
+ e->counters.max_time[kind] = total_time;
+ e->counters.mean_time[kind] = total_time;
+ }
+ else
+ {
+ /*
+ * Welford's method for accurately computing variance. See
+ * <http://www.johndcook.com/blog/standard_deviation/>
+ */
+ double old_mean = e->counters.mean_time[kind];
+
+ e->counters.mean_time[kind] +=
+ (total_time - old_mean) / e->counters.calls[kind];
+ e->counters.sum_var_time[kind] +=
+ (total_time - old_mean) * (total_time - e->counters.mean_time[kind]);
+
+ /* calculate min and max time */
+ if (e->counters.min_time[kind] > total_time)
+ e->counters.min_time[kind] = total_time;
+ if (e->counters.max_time[kind] < total_time)
+ e->counters.max_time[kind] = total_time;
+ }
+}
+
+
/*
* Store some statistics for a statement.
*
@@ -1396,34 +1461,20 @@ pgss_store(const char *query, uint64 queryId,
if (IS_STICKY(e->counters))
e->counters.usage = USAGE_INIT;
- e->counters.calls[kind] += 1;
- e->counters.total_time[kind] += total_time;
+ kind_dependent_accumulation(e, kind, total_time);
- if (e->counters.calls[kind] == 1)
- {
- e->counters.min_time[kind] = total_time;
- e->counters.max_time[kind] = total_time;
- e->counters.mean_time[kind] = total_time;
- }
- else
+ /*
+ * Accumulate information about general and custom plan.
+ * TODO: It would be better to be optional.
+ */
+ if (kind == PGSS_EXEC)
{
- /*
- * Welford's method for accurately computing variance. See
- * <http://www.johndcook.com/blog/standard_deviation/>
- */
- double old_mean = e->counters.mean_time[kind];
-
- e->counters.mean_time[kind] +=
- (total_time - old_mean) / e->counters.calls[kind];
- e->counters.sum_var_time[kind] +=
- (total_time - old_mean) * (total_time - e->counters.mean_time[kind]);
-
- /* calculate min and max time */
- if (e->counters.min_time[kind] > total_time)
- e->counters.min_time[kind] = total_time;
- if (e->counters.max_time[kind] < total_time)
- e->counters.max_time[kind] = total_time;
+ if (plantype == PGSS_PLAN_TYPE_GENERIC)
+ kind_dependent_accumulation(e, PGSS_EXEC_GENERAL, total_time);
+ else if (plantype == PGSS_PLAN_TYPE_CUSTOM)
+ kind_dependent_accumulation(e, PGSS_EXEC_CUSTOM, total_time);
}
+
e->counters.rows += rows;
e->counters.shared_blks_hit += bufusage->shared_blks_hit;
e->counters.shared_blks_read += bufusage->shared_blks_read;
@@ -1488,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 44
+#define PG_STAT_STATEMENTS_COLS 44 /* maximum of above */
/*
* Retrieve statement statistics.
diff --git a/src/backend/utils/cache/plancache.c b/src/backend/utils/cache/plancache.c
index 50d6ad28b4..47dc74a634 100644
--- a/src/backend/utils/cache/plancache.c
+++ b/src/backend/utils/cache/plancache.c
@@ -1217,12 +1217,14 @@ GetCachedPlan(CachedPlanSource *plansource, ParamListInfo boundParams,
plan = BuildCachedPlan(plansource, qlist, boundParams, queryEnv);
/* Accumulate total costs of custom plans */
plansource->total_custom_cost += cached_plan_cost(plan, true);
+ plan->is_generic = false;
plansource->num_custom_plans++;
}
else
{
plansource->num_generic_plans++;
+ plan->is_generic = true;
}
Assert(plan != NULL);
diff --git a/src/include/utils/plancache.h b/src/include/utils/plancache.h
index 4901568553..4edc15602a 100644
--- a/src/include/utils/plancache.h
+++ b/src/include/utils/plancache.h
@@ -158,6 +158,7 @@ typedef struct CachedPlan
int generation; /* parent's generation number for this plan */
int refcount; /* count of live references to this struct */
MemoryContext context; /* context containing this CachedPlan */
+ bool is_generic; /* is this plan generic or not? */
} CachedPlan;
/*
