On 5/1/24 10:07, Imseih (AWS), Sami wrote:
Here is a new rev of the patch which deals with the scenario
mentioned by Andrei [1] in which the queryId may change
due to a cached query invalidation.
[1]
https://www.postgresql.org/message-id/724348C9-8023-41BC-895E-80634E79A538%40amazon.com
I discovered the current state of queryId reporting and found that it
may be unlogical: Postgres resets queryId right before query execution
in simple protocol and doesn't reset it at all in extended protocol and
other ways to execute queries.
I think we should generally report it when the backend executes a job
related to the query with that queryId. This means it would reset the
queryId at the end of the query execution.
However, the process of setting up the queryId is more complex. Should
we set it at the beginning of query execution? This seems logical, but
what about the planning process? If an extension plans a query without
the intention to execute it for speculative reasons, should we still
show the queryId? Perhaps we should reset the state right after planning
to accurately reflect the current queryId.
See in the attachment some sketch for that - it needs to add queryId
reset on abortion.
--
regards, Andrei Lepikhov
diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index 4d7c92d63c..a4d38a157f 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -470,6 +470,12 @@ ExecutorEnd(QueryDesc *queryDesc)
(*ExecutorEnd_hook) (queryDesc);
else
standard_ExecutorEnd(queryDesc);
+
+ /*
+ * Report at the end of query execution. Don't change it for nested
+ * queries.
+ */
+ pgstat_report_query_id(0, false);
}
void
diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
index 032818423f..ba29dc5bc7 100644
--- a/src/backend/optimizer/plan/planner.c
+++ b/src/backend/optimizer/plan/planner.c
@@ -58,6 +58,7 @@
#include "parser/parse_relation.h"
#include "parser/parsetree.h"
#include "partitioning/partdesc.h"
+#include "pgstat.h"
#include "utils/lsyscache.h"
#include "utils/rel.h"
#include "utils/selfuncs.h"
@@ -280,6 +281,13 @@ planner(Query *parse, const char *query_string, int cursorOptions,
result = (*planner_hook) (parse, query_string, cursorOptions, boundParams);
else
result = standard_planner(parse, query_string, cursorOptions, boundParams);
+
+ /*
+ * Reset queryId at the end of planning job. Executor code will set it up
+ * again on the ExecutorStart call.
+ */
+ pgstat_report_query_id(0, false);
+
return result;
}
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 2dff28afce..10e2529cf6 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -1108,8 +1108,6 @@ exec_simple_query(const char *query_string)
const char *cmdtagname;
size_t cmdtaglen;
- pgstat_report_query_id(0, true);
-
/*
* Get the command name for use in status display (it also becomes the
* default completion tag, down inside PortalRun). Set ps_status and
