On Thu, Sep 26, 2024 at 11:01:12PM -0500, Sami Imseih wrote: >> I am not sure. The GUCs pretty much enforce this behavior and I doubt >> that these are going to break moving on. Of course they would, but we >> are usually careful enough about that as long as it is possible to >> grep for them. For example see the BRIN case in pageinspect. > > Yes, I see pageinspect does the same thing for the BRIN case. > That is probably OK for this case also.
Okay, I've applied this part then to fix the query ID reporting for these parallel workers. If people would like a backpatch, please let me know. While thinking more about the assertion check in the executor over the weekend, I've found two things that are not right about it, as of: - It is OK to not set the query ID if we don't have a query string to map to. This is something that came up to me because of the parallel VACUUM case, the query string given to the parallel workers is optional because we don't have a query string in the case of autovacuum. This is not an issue currently because autovacuum does not support parallel jobs (see "tab->at_params.nworkers = -1" in autovacuum.c), but if we support parallel jobs in autovacuum at some point the assertion would fail. BRIN and btree always expect a query string, AFAIK. - The GUC track_activities. We don't really test it in any tests and it is enabled by default, so that's really easy to miss. I have been able to trigger an assertion failure with something like that: SET compute_query_id = on; SET track_activities = off; SELECT 1; The first point is just some prevention for the future. The second case is something we should fix and test. I am attaching a patch that addresses both. Note that the test case cannot use a transaction block as query IDs are only reported for the top queries, and we can do a scan of pg_stat_activity to see if the query ID is set. The assertion was getting more complicated, so I have hidden that behind a macro in execMain.c. All that should complete this project. Thoughts? -- Michael
From c6d5042a0ad0d77bac91300b5d5218552f536968 Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Mon, 30 Sep 2024 10:06:32 +0900
Subject: [PATCH] Expand assertion check for query ID in executor
track_activities and debug_query_strind problems. Blah.
---
src/backend/executor/execMain.c | 18 +++++++++++++++---
src/test/regress/expected/guc.out | 21 +++++++++++++++++++++
src/test/regress/sql/guc.sql | 12 ++++++++++++
3 files changed, 48 insertions(+), 3 deletions(-)
diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index 728cdee480..85f697aaa1 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -71,6 +71,18 @@ ExecutorEnd_hook_type ExecutorEnd_hook = NULL;
/* Hook for plugin to get control in ExecCheckPermissions() */
ExecutorCheckPerms_hook_type ExecutorCheckPerms_hook = NULL;
+/*
+ * Check that the query ID is set, which is something that happens only
+ * if compute_query_id is enabled (or a module forced it), if track_activities
+ * is enabled, and if a client provided a query string to map with the query
+ * ID set.
+ */
+#define EXEC_CHECK_QUERY_ID \
+do { \
+ Assert(!IsQueryIdEnabled() || !pgstat_track_activities || \
+ !debug_query_string || pgstat_get_my_query_id() != 0); \
+} while(0)
+
/* decls for local routines only used within this module */
static void InitPlan(QueryDesc *queryDesc, int eflags);
static void CheckValidRowMarkRel(Relation rel, RowMarkType markType);
@@ -297,7 +309,7 @@ ExecutorRun(QueryDesc *queryDesc,
bool execute_once)
{
/* If enabled, the query ID should be set. */
- Assert(!IsQueryIdEnabled() || pgstat_get_my_query_id() != 0);
+ EXEC_CHECK_QUERY_ID;
if (ExecutorRun_hook)
(*ExecutorRun_hook) (queryDesc, direction, count, execute_once);
@@ -408,7 +420,7 @@ void
ExecutorFinish(QueryDesc *queryDesc)
{
/* If enabled, the query ID should be set. */
- Assert(!IsQueryIdEnabled() || pgstat_get_my_query_id() != 0);
+ EXEC_CHECK_QUERY_ID;
if (ExecutorFinish_hook)
(*ExecutorFinish_hook) (queryDesc);
@@ -471,7 +483,7 @@ void
ExecutorEnd(QueryDesc *queryDesc)
{
/* If enabled, the query ID should be set. */
- Assert(!IsQueryIdEnabled() || pgstat_get_my_query_id() != 0);
+ EXEC_CHECK_QUERY_ID;
if (ExecutorEnd_hook)
(*ExecutorEnd_hook) (queryDesc);
diff --git a/src/test/regress/expected/guc.out b/src/test/regress/expected/guc.out
index 455b6d6c0c..14edb42704 100644
--- a/src/test/regress/expected/guc.out
+++ b/src/test/regress/expected/guc.out
@@ -824,6 +824,27 @@ set default_with_oids to f;
-- Should not allow to set it to true.
set default_with_oids to t;
ERROR: tables declared WITH OIDS are not supported
+-- Test that disabling track_activities disables query ID reporting in
+-- pg_stat_activity.
+SET compute_query_id = on;
+SET track_activities = on;
+SELECT query_id IS NOT NULL AS qid_set FROM pg_stat_activity
+ WHERE pid = pg_backend_pid();
+ qid_set
+---------
+ t
+(1 row)
+
+SET track_activities = off;
+SELECT query_id IS NOT NULL AS qid_set FROM pg_stat_activity
+ WHERE pid = pg_backend_pid();
+ qid_set
+---------
+ f
+(1 row)
+
+RESET track_activities;
+RESET compute_query_id;
-- Test GUC categories and flag patterns
SELECT pg_settings_get_flags(NULL);
pg_settings_get_flags
diff --git a/src/test/regress/sql/guc.sql b/src/test/regress/sql/guc.sql
index dc79761955..2be7ab2940 100644
--- a/src/test/regress/sql/guc.sql
+++ b/src/test/regress/sql/guc.sql
@@ -319,6 +319,18 @@ set default_with_oids to f;
-- Should not allow to set it to true.
set default_with_oids to t;
+-- Test that disabling track_activities disables query ID reporting in
+-- pg_stat_activity.
+SET compute_query_id = on;
+SET track_activities = on;
+SELECT query_id IS NOT NULL AS qid_set FROM pg_stat_activity
+ WHERE pid = pg_backend_pid();
+SET track_activities = off;
+SELECT query_id IS NOT NULL AS qid_set FROM pg_stat_activity
+ WHERE pid = pg_backend_pid();
+RESET track_activities;
+RESET compute_query_id;
+
-- Test GUC categories and flag patterns
SELECT pg_settings_get_flags(NULL);
SELECT pg_settings_get_flags('does_not_exist');
--
2.45.2
signature.asc
Description: PGP signature
