On 2020/03/27 19:00, Julien Rouhaud wrote:
On Thu, Mar 26, 2020 at 02:22:47PM +0100, Julien Rouhaud wrote:
On Thu, Mar 26, 2020 at 08:08:35PM +0900, Fujii Masao wrote:
Here are other comments.
- if (jstate)
+ if (kind == PGSS_JUMBLE)
Why is PGSS_JUMBLE necessary? ISTM that we can still use jstate here, instead.
If it's ok to remove PGSS_JUMBLE, we can define PGSS_NUM_KIND(=2) instead
and replace 2 in, e.g., total_time[2] with PGSS_NUM_KIND. Thought?
Yes, we could be using jstate here. I originally used that to avoid passing
PGSS_EXEC (or the other one) as a way to say "ignore this information as
there's the jstate which says it's yet another meaning". If that's not an
issue, I can change that as PGSS_NUM_KIND will clearly improve the explicit "2"
all over the place.
Done, passing PGSS_PLAN when jumble is intended, with a comment saying that the
pgss_kind is ignored in that case.
+ <entry><structfield>total_time</structfield></entry>
+ <entry><type>double precision</type></entry>
+ <entry></entry>
+ <entry>
+ Total time spend planning and executing the statement, in milliseconds
+ </entry>
+ </row>
pg_stat_statements view has this column but the function not.
We should make both have the column or not at all, for consistency?
I'm not sure if it's good thing to expose the sum of total_plan_time
and total_exec_time as total_time. If some users want that, they can
easily calculate it from total_plan_time and total_exec_time by using
their own logic.
I think we originally added it as a way to avoid too much compatibility break,
and also because it seems like a field most users will be interested in anyway.
Now that I'm thinking about it again, I indeed think it was a mistake to have
that in view part only. Not mainly for consistency, but for users who would be
interested in the total_time field while not wanting to pay the overhead of
retrieving the query text if they don't need it. So I'll change that!
Done
Thanks for updating the patch! But I'm still wondering if it's really
good thing to expose total_time. For example, when the query fails
with an error many times and "calls" becomes very different from
"plans", "total_plan_time" + "total_exec_time" is really what the users
are interested in? Some users may be interested in the sum of mean
times, but it's not exposed...
So what I'd like to say is that the information that users are interested
in would vary on each situation and case. At least for me it seems
enough for pgss to report only the basic information. Then users
can calculate to get the numbers (like total_time) they're interested in,
from those basic information.
But of course, I'd like to hear more opinions about this...
+ if (api_version >= PGSS_V1_8)
+ values[i++] = Int64GetDatumFast(tmp.total_time[0] +
+
tmp.total_time[1]);
BTW, Int64GetDatumFast() should be Float8GetDatumFast()?
+ nested_level++;
+ PG_TRY();
In old thread [1], Tom Lane commented the usage of nested_level
in the planner hook. There seems no reply to that so far. What's
your opinion about that comment?
[1] https://www.postgresql.org/message-id/28980.1515803...@sss.pgh.pa.us
Oh thanks, I didn't noticed this part of the discussion. I agree with Tom's
concern, and I think that having a specific nesting level variable for the
planner is the best workaround, so I'll implement that.
Done.
I also exported BufferUsageAccumDiff as mentioned previously, as it seems
clearner and will avoid future useless code churn, and run pgindent.
Many thanks!! I'm thinking to commit this part separately.
So I made that patch based on your patch. Attached.
Regards,
--
Fujii Masao
NTT DATA CORPORATION
Advanced Platform Technology Group
Research and Development Headquarters
diff --git a/contrib/pg_stat_statements/pg_stat_statements.c
b/contrib/pg_stat_statements/pg_stat_statements.c
index 20dc8c605b..50c345378d 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -1016,30 +1016,8 @@ pgss_ProcessUtility(PlannedStmt *pstmt, const char
*queryString,
rows = (qc && qc->commandTag == CMDTAG_COPY) ? qc->nprocessed :
0;
/* calc differences of buffer counters. */
- bufusage.shared_blks_hit =
- pgBufferUsage.shared_blks_hit -
bufusage_start.shared_blks_hit;
- bufusage.shared_blks_read =
- pgBufferUsage.shared_blks_read -
bufusage_start.shared_blks_read;
- bufusage.shared_blks_dirtied =
- pgBufferUsage.shared_blks_dirtied -
bufusage_start.shared_blks_dirtied;
- bufusage.shared_blks_written =
- pgBufferUsage.shared_blks_written -
bufusage_start.shared_blks_written;
- bufusage.local_blks_hit =
- pgBufferUsage.local_blks_hit -
bufusage_start.local_blks_hit;
- bufusage.local_blks_read =
- pgBufferUsage.local_blks_read -
bufusage_start.local_blks_read;
- bufusage.local_blks_dirtied =
- pgBufferUsage.local_blks_dirtied -
bufusage_start.local_blks_dirtied;
- bufusage.local_blks_written =
- pgBufferUsage.local_blks_written -
bufusage_start.local_blks_written;
- bufusage.temp_blks_read =
- pgBufferUsage.temp_blks_read -
bufusage_start.temp_blks_read;
- bufusage.temp_blks_written =
- pgBufferUsage.temp_blks_written -
bufusage_start.temp_blks_written;
- bufusage.blk_read_time = pgBufferUsage.blk_read_time;
- INSTR_TIME_SUBTRACT(bufusage.blk_read_time,
bufusage_start.blk_read_time);
- bufusage.blk_write_time = pgBufferUsage.blk_write_time;
- INSTR_TIME_SUBTRACT(bufusage.blk_write_time,
bufusage_start.blk_write_time);
+ memset(&bufusage, 0, sizeof(BufferUsage));
+ BufferUsageAccumDiff(&bufusage, &pgBufferUsage,
&bufusage_start);
pgss_store(queryString,
0, /* signal that it's a
utility stmt */
diff --git a/src/backend/executor/instrument.c
b/src/backend/executor/instrument.c
index bc1d42bf64..042e10f96b 100644
--- a/src/backend/executor/instrument.c
+++ b/src/backend/executor/instrument.c
@@ -21,8 +21,6 @@ BufferUsage pgBufferUsage;
static BufferUsage save_pgBufferUsage;
static void BufferUsageAdd(BufferUsage *dst, const BufferUsage *add);
-static void BufferUsageAccumDiff(BufferUsage *dst,
- const
BufferUsage *add, const BufferUsage *sub);
/* Allocate new instrumentation structure(s) */
@@ -203,7 +201,7 @@ BufferUsageAdd(BufferUsage *dst, const BufferUsage *add)
}
/* dst += add - sub */
-static void
+void
BufferUsageAccumDiff(BufferUsage *dst,
const BufferUsage *add,
const BufferUsage *sub)
diff --git a/src/include/executor/instrument.h
b/src/include/executor/instrument.h
index f48d46aede..3825a5ac1f 100644
--- a/src/include/executor/instrument.h
+++ b/src/include/executor/instrument.h
@@ -81,5 +81,7 @@ extern void InstrAggNode(Instrumentation *dst,
Instrumentation *add);
extern void InstrStartParallelQuery(void);
extern void InstrEndParallelQuery(BufferUsage *result);
extern void InstrAccumParallelQuery(BufferUsage *result);
+extern void BufferUsageAccumDiff(BufferUsage *dst,
+ const
BufferUsage *add, const BufferUsage *sub);
#endif /* INSTRUMENT_H */
