public inbox for [email protected]
help / color / mirror / Atom feedFrom: Imseih (AWS), Sami <[email protected]>
To: Drouvot, Bertrand <[email protected]>
To: Michael Paquier <[email protected]>
Cc: David Zhang <[email protected]>
Cc: [email protected] <[email protected]>
Subject: Re: [BUG] pg_stat_statements and extended query protocol
Date: Wed, 22 Mar 2023 21:35:23 +0000
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<ZA0/[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
> What about using an uint64 for calls? That seems more appropriate to me (even if
> queryDesc->totaltime->calls will be passed (which is int64), but that's already
> also the case for the "rows" argument and queryDesc->totaltime->rows_processed)
That's fair
> I'm not sure it's worth mentioning that the new counters are "currently" used with the ExecutorRun.
Sure, I suppose these fields could be used outside of ExecutorRun. Good point.
> Also, I wonder if "rows" (and not rows_processed) would not be a better naming.
Agree.
I went with rows_processed initially, since it was accumulating es_processed,
but as the previous point, this instrumentation could be used outside of
ExecutorRun.
v3 addresses the comments.
Regards,
--
Sami Imseih
Amazon Web Services (AWS)
Attachments:
[application/octet-stream] v3-0001-Correct-accumulation-of-counters-for-extended-query-.patch (4.2K, ../[email protected]/2-v3-0001-Correct-accumulation-of-counters-for-extended-query-.patch)
download | inline diff:
From 9427777ccc6fc84a087b9845bce58054b628458d Mon Sep 17 00:00:00 2001
From: EC2 Default User <[email protected]>
Date: Wed, 22 Mar 2023 20:14:51 +0000
Subject: [PATCH 1/1] Correct accumulation of counters for extended query
protocol
In pg_stat_statements, EState->es_processed cannot reliably
be used to count the rows processed for a statement that
goes through extended query protocol. This is because, such
a statement may go through ExecutorRun more than once, and
es_processed is reset after every call. This fix addresses
this issue by accuumulating the # of calls and row processed
in Instrumentation and the correct totals are available to
ExecutorEnd ( and pgss_store ) before the Instrumentation
is destroyed.
Discussion: https://www.postgresql.org/message-id/flat/EBE6C507-9EB6-4142-9E4D-38B1673363A7%40amazon.com
---
.../pg_stat_statements/pg_stat_statements.c | 18 ++++++++++++++----
src/include/executor/instrument.h | 2 ++
2 files changed, 16 insertions(+), 4 deletions(-)
diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index 5285c3f7fa..f6d5ac9402 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -340,7 +340,7 @@ static void pgss_store(const char *query, uint64 queryId,
int query_location, int query_len,
pgssStoreKind kind,
double total_time, uint64 rows,
- const BufferUsage *bufusage,
+ uint64 calls, const BufferUsage *bufusage,
const WalUsage *walusage,
const struct JitInstrumentation *jitusage,
JumbleState *jstate);
@@ -857,6 +857,7 @@ pgss_post_parse_analyze(ParseState *pstate, Query *query, JumbleState *jstate)
PGSS_INVALID,
0,
0,
+ 0,
NULL,
NULL,
NULL,
@@ -942,6 +943,7 @@ pgss_planner(Query *parse,
PGSS_PLAN,
INSTR_TIME_GET_MILLISEC(duration),
0,
+ 1,
&bufusage,
&walusage,
NULL,
@@ -1014,6 +1016,12 @@ pgss_ExecutorRun(QueryDesc *queryDesc, ScanDirection direction, uint64 count,
exec_nested_level--;
}
PG_END_TRY();
+
+ if (queryDesc->totaltime)
+ {
+ queryDesc->totaltime->calls++;
+ queryDesc->totaltime->rows += queryDesc->estate->es_processed;
+ }
}
/*
@@ -1060,7 +1068,8 @@ pgss_ExecutorEnd(QueryDesc *queryDesc)
queryDesc->plannedstmt->stmt_len,
PGSS_EXEC,
queryDesc->totaltime->total * 1000.0, /* convert to msec */
- queryDesc->estate->es_processed,
+ queryDesc->totaltime->rows,
+ queryDesc->totaltime->calls,
&queryDesc->totaltime->bufusage,
&queryDesc->totaltime->walusage,
queryDesc->estate->es_jit ? &queryDesc->estate->es_jit->instr : NULL,
@@ -1191,6 +1200,7 @@ pgss_ProcessUtility(PlannedStmt *pstmt, const char *queryString,
PGSS_EXEC,
INSTR_TIME_GET_MILLISEC(duration),
rows,
+ 1,
&bufusage,
&walusage,
NULL,
@@ -1225,7 +1235,7 @@ pgss_store(const char *query, uint64 queryId,
int query_location, int query_len,
pgssStoreKind kind,
double total_time, uint64 rows,
- const BufferUsage *bufusage,
+ uint64 calls, const BufferUsage *bufusage,
const WalUsage *walusage,
const struct JitInstrumentation *jitusage,
JumbleState *jstate)
@@ -1350,7 +1360,7 @@ pgss_store(const char *query, uint64 queryId,
if (IS_STICKY(e->counters))
e->counters.usage = USAGE_INIT;
- e->counters.calls[kind] += 1;
+ e->counters.calls[kind] += calls;
e->counters.total_time[kind] += total_time;
if (e->counters.calls[kind] == 1)
diff --git a/src/include/executor/instrument.h b/src/include/executor/instrument.h
index 87e5e2183b..33f30832d7 100644
--- a/src/include/executor/instrument.h
+++ b/src/include/executor/instrument.h
@@ -88,6 +88,8 @@ typedef struct Instrumentation
double nfiltered2; /* # of tuples removed by "other" quals */
BufferUsage bufusage; /* total buffer usage */
WalUsage walusage; /* total WAL usage */
+ int64 calls; /* total calls */
+ int64 rows; /* total rows */
} Instrumentation;
typedef struct WorkerInstrumentation
--
2.39.2
view thread (22+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected]
Subject: Re: [BUG] pg_stat_statements and extended query protocol
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox