On Fri, Mar 22, 2019 at 11:46 PM legrand legrand
<legrand_legr...@hotmail.com> wrote:
>
> Here is a rebased and corrected version .
This patch has multiple trailing whitespace, indent and coding style
issues. You should consider running pg_indent before submitting a
patch. I attach the diff after running pgindent if you want more
details about the various issues.
- * Track statement execution times across a whole database cluster.
+ * Track statement planning and execution times across a whole cluster.
if we're changing this, we should also fix the fact that's it's not
tracking only the time but various resources?
+ /* calc differences of buffer counters. */
+ bufusage.shared_blks_hit =
+ pgBufferUsage.shared_blks_hit - bufusage_start.shared_blks_hit;
[...]
This is an exact duplication of pgss_ProcessUtility(), it's probably
better to create a macro or a function for that instead.
+ pgss_store("",
+ parse->queryId, /* signal that it's a
utility stmt */
+ -1,
the comment makes no sense, and also you can't pass an empty query
string / unknown len. There's no guarantee that the entry for the
given queryId won't have been evicted, and in this case you'll create
a new and unrelated entry.
@@ -832,13 +931,13 @@ pgss_post_parse_analyze(ParseState *pstate, Query *query)
* the normalized string would be the same as the query text anyway, so
* there's no need for an early entry.
*/
- if (jstate.clocations_count > 0)
pgss_store(pstate->p_sourcetext,
Why did you remove this? pgss_store() isn't free, and asking to
generate a normalized query for a query that doesn't have any constant
or storing the entry early won't do anything useful AFAICT. Though if
that's useful, you definitely can't remove the test without adapting
the comment and the indentation.
@@ -1249,15 +1351,19 @@ pgss_store(const char *query, uint64 queryId,
if (e->counters.calls == 0)
e->counters.usage = USAGE_INIT;
- e->counters.calls += 1;
- e->counters.total_time += total_time;
- if (e->counters.calls == 1)
+ if (planning_time == 0)
+ {
+ e->counters.calls += 1;
+ e->counters.total_time += total_time;
+ }
+
+ if (e->counters.calls == 1 && planning_time == 0)
{
e->counters.min_time = total_time;
e->counters.max_time = total_time;
e->counters.mean_time = total_time;
}
- else
+ else if(planning_time == 0)
{
/*
* Welford's method for accurately computing variance. See
@@ -1276,6 +1382,9 @@ pgss_store(const char *query, uint64 queryId,
if (e->counters.max_time < total_time)
e->counters.max_time = total_time;
}
+ if (planning_time > 0)
+ e->counters.plans += 1;
+ e->counters.planning_time += planning_time;
there are 4 tests to check if planning_time is zero or not, it's quite
messy. Could you refactor the code to avoid so many tests? It would
probably be useful to add some asserts to check that we don't provide
both planning_time == 0 and execution related values. The function's
comment would also need to be adapted to mention the new rationale
with planning_time.
* hash table entry for the PREPARE (with hash calculated from the query
* string), and then a different one with the same query string (but hash
* calculated from the query tree) would be used to accumulate costs of
- * ensuing EXECUTEs. This would be confusing, and inconsistent with other
- * cases where planning time is not included at all.
+ * ensuing EXECUTEs.
the comment about confusing behavior is still valid.
>
> Columns naming has not been modified, I would propose to change it to:
> - plans: ok
> - planning_time --> plan_time
> - calls: ok
> - total_time --> exec_time
> - {min,max,mean,stddev}_time: ok
> - new total_time (being the sum of plan_time and exec_time)
plan_time and exec_time are accumulated counters, so we need to keep
the total_ prefix in any case. I think it's ok to break the function
output names if we keep some kind of compatibility at the view level
(which can keep total_time as the sum of total_plan_time and
total_exec_time), so current queries against the view wouldn't break,
and get what they probably wanted.
diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index 3da490b66d..8c20c91200 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -304,8 +304,8 @@ PG_FUNCTION_INFO_V1(pg_stat_statements);
static void pgss_shmem_startup(void);
static void pgss_shmem_shutdown(int code, Datum arg);
static PlannedStmt *pgss_planner_hook(Query *parse,
- int cursorOptions,
- ParamListInfo boundParams);
+ int cursorOptions,
+ ParamListInfo boundParams);
static void pgss_post_parse_analyze(ParseState *pstate, Query *query);
static void pgss_ExecutorStart(QueryDesc *queryDesc, int eflags);
static void pgss_ExecutorRun(QueryDesc *queryDesc,
@@ -789,19 +789,20 @@ error:
/*
* Planner hook: forward to regular planner, but measure planning time.
*/
-static PlannedStmt *pgss_planner_hook(Query *parse,
- int cursorOptions,
- ParamListInfo boundParams)
+static PlannedStmt *
+pgss_planner_hook(Query *parse,
+ int cursorOptions,
+ ParamListInfo boundParams)
{
PlannedStmt *result;
BufferUsage bufusage_start,
bufusage;
- bufusage_start = pgBufferUsage;
+ bufusage_start = pgBufferUsage;
if (pgss_enabled())
{
- instr_time start;
- instr_time duration;
+ instr_time start;
+ instr_time duration;
INSTR_TIME_SET_CURRENT(start);
@@ -850,8 +851,8 @@ static PlannedStmt *pgss_planner_hook(Query *parse,
bufusage.blk_write_time = pgBufferUsage.blk_write_time;
INSTR_TIME_SUBTRACT(bufusage.blk_write_time, bufusage_start.blk_write_time);
- pgss_store("",
- parse->queryId, /* signal that it's a utility stmt */
+ pgss_store("",
+ parse->queryId, /* signal that it's a utility stmt */
-1,
0,
0,
@@ -931,15 +932,15 @@ pgss_post_parse_analyze(ParseState *pstate, Query *query)
* the normalized string would be the same as the query text anyway, so
* there's no need for an early entry.
*/
- pgss_store(pstate->p_sourcetext,
- query->queryId,
- query->stmt_location,
- query->stmt_len,
- 0,
- 0,
- 0,
- NULL,
- &jstate);
+ pgss_store(pstate->p_sourcetext,
+ query->queryId,
+ query->stmt_location,
+ query->stmt_len,
+ 0,
+ 0,
+ 0,
+ NULL,
+ &jstate);
}
/*
@@ -1044,7 +1045,7 @@ pgss_ExecutorEnd(QueryDesc *queryDesc)
queryDesc->plannedstmt->stmt_location,
queryDesc->plannedstmt->stmt_len,
queryDesc->totaltime->total * 1000.0, /* convert to msec */
- 0,
+ 0,
queryDesc->estate->es_processed,
&queryDesc->totaltime->bufusage,
NULL);
@@ -1255,8 +1256,9 @@ pgss_store(const char *query, uint64 queryId,
queryId = pgss_hash_string(query, query_len);
/*
- * If we are unlucky enough to get a hash of zero(invalid), use queryID
- * as 2 instead, queryID 1 is already in use for normal statements.
+ * If we are unlucky enough to get a hash of zero(invalid), use
+ * queryID as 2 instead, queryID 1 is already in use for normal
+ * statements.
*/
if (queryId == UINT64CONST(0))
queryId = UINT64CONST(2);
@@ -1355,7 +1357,7 @@ pgss_store(const char *query, uint64 queryId,
{
e->counters.calls += 1;
e->counters.total_time += total_time;
- }
+ }
if (e->counters.calls == 1 && planning_time == 0)
{
@@ -1363,7 +1365,7 @@ pgss_store(const char *query, uint64 queryId,
e->counters.max_time = total_time;
e->counters.mean_time = total_time;
}
- else if(planning_time == 0)
+ else if (planning_time == 0)
{
/*
* Welford's method for accurately computing variance. See
