On Wed, Jul 17, 2024 at 11:32:49AM +0200, Anthonin Bonnefoy wrote: > Wouldn't it be enough to call pgstat_report_query_id in ExecutorRun > and ProcessUtility? With those changes [1], both normal statements and > utility statements called through extended protocol will correctly > report the query_id.
Interesting, and this position is really tempting. By doing so you would force the query ID to be set to the one from the CTAS and EXPLAIN, because these would be executed before the inner queries, and pgstat_report_query_id() with its non-force option does not overwrite what would be already set (aka what should be the top-level query ID). Using ExecutorRun() feels consistent with the closest thing I've touched in this area lately in 1d477a907e63, because that's the only code path that we are sure to take depending on the portal execution (two execution scenarios depending on how rows are retrieved, as far as I recall). The comment should be perhaps more consistent with the executor start counterpart. So I would be OK with that.. The location before the hook of ProcessUtility is tempting, as it would take care of the case of PortalRunMulti(). However.. Joining with a point from Sami upthread.. This is still not enough in the case of where we have a holdStore, no? This is the case where we would do *one* ExecutorRun(), followed up by a scan of the tuplestore in more than one execute message. The v2 proposed upthread, by positioning a query ID to be set in PortalRunSelect(), is still positioning that in two places. Hmm... How about being much more aggressive and just do the whole business in exec_execute_message(), just before we do the PortalRun()? I mean, that's the source of all our problems, and we know the statements that the portal will work on so we could go through the list, grab the first planned query and set the query ID based on that, without caring about the portal patterns we would need to think about. > [1] > https://github.com/bonnefoa/postgres/commit/bf4b332d7b481549c6d9cfa70db51e39a305b9b2 Or use the following to download the patch, that I am attaching here: https://github.com/bonnefoa/postgres/commit/bf4b332d7b481549c6d9cfa70db51e39a305b9b2.patch Please attach things to your emails, if your repository disappears for a reason or another we would lose knowledge in the archives of the community lists. -- Michael
From bf4b332d7b481549c6d9cfa70db51e39a305b9b2 Mon Sep 17 00:00:00 2001
From: Anthonin Bonnefoy <anthonin.bonne...@datadoghq.com>
Date: Wed, 17 Jul 2024 10:27:35 +0200
Subject: [PATCH] Report query_id in ExecutorRun for extended protocol queries
When using extended protocol, exec_execute_message will reset
MyBEEntry's query_id when calling pgstat_report_activity. The query_id
will never be set after that since it is only set in parse analysis and
ExecutorStart. Because of that, all queries executed through extended
protocol will report an empty query_id in pg_stat_activity and the
query_id won't be propagated to parallel workers.
The patch fixes the issue by setting query_id in ExecutorRun, similarly
to what is already done in ExecutorStart.
---
src/backend/executor/execMain.c | 8 ++++++++
src/backend/tcop/utility.c | 7 +++++++
2 files changed, 15 insertions(+)
diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index 4d7c92d63c198..69cca12723923 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -298,6 +298,14 @@ ExecutorRun(QueryDesc *queryDesc,
ScanDirection direction, uint64 count,
bool execute_once)
{
+ /*
+ * When using extended protocol, query_id will be reset during
+ * pgstat_report_activity and never set since we don't go through parse
+ * analysis or ExecutorStart. Similar to what's done in ExecutorStart,
+ * report the queryId we know from the queryDesc.
+ */
+ pgstat_report_query_id(queryDesc->plannedstmt->queryId, false);
+
if (ExecutorRun_hook)
(*ExecutorRun_hook) (queryDesc, direction, count, execute_once);
else
diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
index fa66b8017edea..8cbd95701d93f 100644
--- a/src/backend/tcop/utility.c
+++ b/src/backend/tcop/utility.c
@@ -63,6 +63,7 @@
#include "storage/fd.h"
#include "tcop/utility.h"
#include "utils/acl.h"
+#include "utils/backend_status.h"
#include "utils/guc.h"
#include "utils/lsyscache.h"
@@ -510,6 +511,12 @@ ProcessUtility(PlannedStmt *pstmt,
Assert(queryString != NULL); /* required as of 8.4 */
Assert(qc == NULL || qc->commandTag == CMDTAG_UNKNOWN);
+ /*
+ * Utility statement executed through extended protocol won't have
+ * query_id set, report it here
+ */
+ pgstat_report_query_id(pstmt->queryId, false);
+
/*
* We provide a function hook variable that lets loadable plugins get
* control when ProcessUtility is called. Such a plugin would normally
signature.asc
Description: PGP signature
