On 2023-01-23 09:35, Michael Paquier wrote:
On Fri, Jan 20, 2023 at 12:32:58PM +0900, Michael Paquier wrote:
FWIW, no objections from here. This maps with EXPLAIN where the query
ID is only printed under VERBOSE.
While looking at this change, I have been wondering about something..
Isn't the knowledge of the query ID something that should be pushed
within ExplainPrintPlan() so as we don't duplicate in two places the
checks that show it? In short, the patch ignores the case where
compute_query_id = regress in auto_explain.
Thanks!
It seems better and updated the patch.
ExplainPrintTriggers() is kind of different because there is
auto_explain_log_triggers. Still, we could add a flag in ExplainState
deciding if the triggers should be printed, so as it would be possible
to move ExplainPrintTriggers() and ExplainPrintJITSummary() within
ExplainPrintPlan(), as well? The same kind of logic could be applied
for the planning time and the buffer usage if these are tracked in
ExplainState rather than being explicit arguments of ExplainOnePlan().
Not to mention that this reduces the differences between
ExplainOneUtility() and ExplainOnePlan().
Hmm, this refactoring would worth considering, but should be done in
another patch?
Leaving this comment aside, I think that this should have at least one
regression test in 001_auto_explain.pl, where query_log() can be
called while the verbose GUC of auto_explain is enabled.
Agreed.
Added a test for queryid logging.
--
Michael
--
Regards,
--
Atsushi Torikoshi
NTT DATA CORPORATION
From c7bd5419ea921fa9f670d57b27611fb39c05bbc7 Mon Sep 17 00:00:00 2001
From: Atsushi Torikoshi <torikos...@oss.nttdata.com>
Date: Tue, 24 Jan 2023 22:23:21 +0900
Subject: [PATCH v3] Make auto_explain log query identifier.
When auto_explain.log_verbose is on, auto_explain should record logs
equivalent to VERBOSE option of EXPLAIN. Howerver, when
compute_query_id is on, query identifiers are only printed in
VERBOSE option of EXPLAIN.
This patch makes auto_explain also print query identifier.
---
contrib/auto_explain/t/001_auto_explain.pl | 12 ++++++++
src/backend/commands/explain.c | 32 +++++++++++-----------
2 files changed, 28 insertions(+), 16 deletions(-)
diff --git a/contrib/auto_explain/t/001_auto_explain.pl b/contrib/auto_explain/t/001_auto_explain.pl
index 0cf093c88e..408b439bfd 100644
--- a/contrib/auto_explain/t/001_auto_explain.pl
+++ b/contrib/auto_explain/t/001_auto_explain.pl
@@ -106,6 +106,18 @@ unlike(
qr/Query Parameters:/,
"query parameters not logged when disabled, text mode");
+# Query Identifier.
+$log_contents = query_log(
+ $node,
+ "SELECT * FROM pg_class;",
+ { "auto_explain.log_verbose" => "on",
+ "compute_query_id" => "on"});
+
+like(
+ $log_contents,
+ qr/Query Identifier:/,
+ "query identifier logged, text mode");
+
# JSON format.
$log_contents = query_log(
$node,
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index 5212a64b1e..a0311ce9dc 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -604,22 +604,6 @@ ExplainOnePlan(PlannedStmt *plannedstmt, IntoClause *into, ExplainState *es,
/* Create textual dump of plan tree */
ExplainPrintPlan(es, queryDesc);
- /*
- * COMPUTE_QUERY_ID_REGRESS means COMPUTE_QUERY_ID_AUTO, but we don't show
- * the queryid in any of the EXPLAIN plans to keep stable the results
- * generated by regression test suites.
- */
- if (es->verbose && plannedstmt->queryId != UINT64CONST(0) &&
- compute_query_id != COMPUTE_QUERY_ID_REGRESS)
- {
- /*
- * Output the queryid as an int64 rather than a uint64 so we match
- * what would be seen in the BIGINT pg_stat_statements.queryid column.
- */
- ExplainPropertyInteger("Query Identifier", NULL, (int64)
- plannedstmt->queryId, es);
- }
-
/* Show buffer usage in planning */
if (bufusage)
{
@@ -791,6 +775,22 @@ ExplainPrintPlan(ExplainState *es, QueryDesc *queryDesc)
* don't match the built-in defaults.
*/
ExplainPrintSettings(es);
+
+ /*
+ * COMPUTE_QUERY_ID_REGRESS means COMPUTE_QUERY_ID_AUTO, but we don't show
+ * the queryid in any of the EXPLAIN plans to keep stable the results
+ * generated by regression test suites.
+ */
+ if (es->verbose && queryDesc->plannedstmt->queryId != UINT64CONST(0) &&
+ compute_query_id != COMPUTE_QUERY_ID_REGRESS)
+ {
+ /*
+ * Output the queryid as an int64 rather than a uint64 so we match
+ * what would be seen in the BIGINT pg_stat_statements.queryid column.
+ */
+ ExplainPropertyInteger("Query Identifier", NULL, (int64)
+ queryDesc->plannedstmt->queryId, es);
+ }
}
/*
base-commit: 6c6d6ba3ee2c160b53f727cf8e612014b316d6e4
--
2.25.1
