Small fix on query_id_enabled
Hi,
I found the comment on query_id_enabled looks inaccurate because this is
never set to true when compute_query_id is ON.
/* True when compute_query_id is ON, or AUTO and a module requests them */
bool query_id_enabled = false;
Should we fix this as following (just fixing the place of a comma) ?
/* True when compute_query_id is ON or AUTO, and a module requests them */
Also, I think the name is a bit confusing for the same reason, that is,
query_id_enabled may be false even when query id is computed in fact.
Actually, this does not matter because we use IsQueryIdEnabled to check
if query id is enabled, instead of referring to a global variable
(query_id_enabled or compute_query_id). But, just for making a code a bit
more readable, how about renaming this to query_id_required which seems to
stand for the meaning more correctly?
I attached a patch for above fixes.
Although renaming might not be acceptable since changing global variables
may affect third party tools, I think the comment should be fixed at least.
IMHO, it seems better to make this variable static not to be accessed directly.
However, I left it as is because this is used in a static inline function.
Regards,
Yugo Nagata
--
Yugo NAGATA <nagata@sraoss.co.jp>
Attachments:
fix_query_id_enabled.patchtext/x-diff; name=fix_query_id_enabled.patchDownload
diff --git a/src/backend/nodes/queryjumblefuncs.c b/src/backend/nodes/queryjumblefuncs.c
index e489bfceb5..e4fbcf0d9f 100644
--- a/src/backend/nodes/queryjumblefuncs.c
+++ b/src/backend/nodes/queryjumblefuncs.c
@@ -42,8 +42,8 @@
/* GUC parameters */
int compute_query_id = COMPUTE_QUERY_ID_AUTO;
-/* True when compute_query_id is ON, or AUTO and a module requests them */
-bool query_id_enabled = false;
+/* True when compute_query_id is ON or AUTO, and a module requests them */
+bool query_id_required = false;
static void AppendJumble(JumbleState *jstate,
const unsigned char *item, Size size);
@@ -145,7 +145,7 @@ void
EnableQueryId(void)
{
if (compute_query_id != COMPUTE_QUERY_ID_OFF)
- query_id_enabled = true;
+ query_id_required = true;
}
/*
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index a066800a1c..4bcaf6d71c 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -532,7 +532,7 @@ typedef struct
pg_time_t first_syslogger_file_time;
bool redirection_done;
bool IsBinaryUpgrade;
- bool query_id_enabled;
+ bool query_id_required;
int max_safe_fds;
int MaxBackends;
#ifdef WIN32
@@ -6103,7 +6103,7 @@ save_backend_variables(BackendParameters *param, Port *port, BackgroundWorker *w
param->redirection_done = redirection_done;
param->IsBinaryUpgrade = IsBinaryUpgrade;
- param->query_id_enabled = query_id_enabled;
+ param->query_id_required = query_id_required;
param->max_safe_fds = max_safe_fds;
param->MaxBackends = MaxBackends;
@@ -6348,7 +6348,7 @@ restore_backend_variables(BackendParameters *param, Port **port, BackgroundWorke
redirection_done = param->redirection_done;
IsBinaryUpgrade = param->IsBinaryUpgrade;
- query_id_enabled = param->query_id_enabled;
+ query_id_required = param->query_id_required;
max_safe_fds = param->max_safe_fds;
MaxBackends = param->MaxBackends;
diff --git a/src/include/nodes/queryjumble.h b/src/include/nodes/queryjumble.h
index f1c55c8067..3b2e1f8018 100644
--- a/src/include/nodes/queryjumble.h
+++ b/src/include/nodes/queryjumble.h
@@ -67,7 +67,7 @@ extern const char *CleanQuerytext(const char *query, int *location, int *len);
extern JumbleState *JumbleQuery(Query *query);
extern void EnableQueryId(void);
-extern PGDLLIMPORT bool query_id_enabled;
+extern PGDLLIMPORT bool query_id_required;
/*
* Returns whether query identifier computation has been enabled, either
@@ -80,7 +80,7 @@ IsQueryIdEnabled(void)
return false;
if (compute_query_id == COMPUTE_QUERY_ID_ON)
return true;
- return query_id_enabled;
+ return query_id_required;
}
#endif /* QUERYJUMBLE_H */
Hi,
On Fri, Feb 09, 2024 at 03:38:23PM +0900, Yugo NAGATA wrote:
I found the comment on query_id_enabled looks inaccurate because this is
never set to true when compute_query_id is ON./* True when compute_query_id is ON, or AUTO and a module requests them */
bool query_id_enabled = false;Should we fix this as following (just fixing the place of a comma) ?
/* True when compute_query_id is ON or AUTO, and a module requests them */
Agreed.
Also, I think the name is a bit confusing for the same reason, that is,
query_id_enabled may be false even when query id is computed in fact.Actually, this does not matter because we use IsQueryIdEnabled to check
if query id is enabled, instead of referring to a global variable
(query_id_enabled or compute_query_id). But, just for making a code a bit
more readable, how about renaming this to query_id_required which seems to
stand for the meaning more correctly?
-1 for renaming to avoid breaking extensions that might access it. We should
simply document for compute_query_id and query_id_enabled declaration that one
should instead use IsQueryIdEnabled() if they're interested in whether the core
queryid are computed or not.
On Fri, Feb 09, 2024 at 04:37:23PM +0800, Julien Rouhaud wrote:
On Fri, Feb 09, 2024 at 03:38:23PM +0900, Yugo NAGATA wrote:
Also, I think the name is a bit confusing for the same reason, that is,
query_id_enabled may be false even when query id is computed in fact.Actually, this does not matter because we use IsQueryIdEnabled to check
if query id is enabled, instead of referring to a global variable
(query_id_enabled or compute_query_id). But, just for making a code a bit
more readable, how about renaming this to query_id_required which seems to
stand for the meaning more correctly?-1 for renaming to avoid breaking extensions that might access it. We should
simply document for compute_query_id and query_id_enabled declaration that one
should instead use IsQueryIdEnabled() if they're interested in whether the core
queryid are computed or not.
Agreed. A renaming would involve more pain than gain. Improving the
comments around how to all that would be good enough, my 2c.
--
Michael
On Sat, 10 Feb 2024 10:19:15 +0900
Michael Paquier <michael@paquier.xyz> wrote:
On Fri, Feb 09, 2024 at 04:37:23PM +0800, Julien Rouhaud wrote:
On Fri, Feb 09, 2024 at 03:38:23PM +0900, Yugo NAGATA wrote:
Also, I think the name is a bit confusing for the same reason, that is,
query_id_enabled may be false even when query id is computed in fact.Actually, this does not matter because we use IsQueryIdEnabled to check
if query id is enabled, instead of referring to a global variable
(query_id_enabled or compute_query_id). But, just for making a code a bit
more readable, how about renaming this to query_id_required which seems to
stand for the meaning more correctly?-1 for renaming to avoid breaking extensions that might access it. We should
simply document for compute_query_id and query_id_enabled declaration that one
should instead use IsQueryIdEnabled() if they're interested in whether the core
queryid are computed or not.Agreed. A renaming would involve more pain than gain. Improving the
comments around how to all that would be good enough, my 2c.
Thank you both for your comments.
I agreed with not renaming it.
I attached a updated patch that adds comments noting to use IsQueryIdEnabled()
instead of accessing the variables directly.
Regards,
Yugo Nagata
--
Yugo NAGATA <nagata@sraoss.co.jp>
Attachments:
v2_fix_comment_on_query_id_enabled.patchtext/x-diff; name=v2_fix_comment_on_query_id_enabled.patchDownload
diff --git a/src/backend/nodes/queryjumblefuncs.c b/src/backend/nodes/queryjumblefuncs.c
index e489bfceb5..82f725baaa 100644
--- a/src/backend/nodes/queryjumblefuncs.c
+++ b/src/backend/nodes/queryjumblefuncs.c
@@ -42,7 +42,13 @@
/* GUC parameters */
int compute_query_id = COMPUTE_QUERY_ID_AUTO;
-/* True when compute_query_id is ON, or AUTO and a module requests them */
+/*
+ * True when compute_query_id is ON or AUTO, and a module requests them.
+ *
+ * Note that IsQueryIdEnabled() should be used instead of checking
+ * query_id_enabled or compute_query_id directly when we want to know
+ * whether query identifiers are computed in the core or not.
+ */
bool query_id_enabled = false;
static void AppendJumble(JumbleState *jstate,
On Tue, Feb 13, 2024 at 01:13:43AM +0900, Yugo NAGATA wrote:
I attached an updated patch that adds comments noting to use IsQueryIdEnabled()
instead of accessing the variables directly.
Sounds good to me, thanks.
--
Michael
On Tue, Feb 13, 2024 at 05:28:32PM +0900, Michael Paquier wrote:
On Tue, Feb 13, 2024 at 01:13:43AM +0900, Yugo NAGATA wrote:
I attached an updated patch that adds comments noting to use IsQueryIdEnabled()
instead of accessing the variables directly.Sounds good to me, thanks.
+1!
On Tue, Feb 13, 2024 at 11:23:47PM +0800, Julien Rouhaud wrote:
+1!
Okay, applied as-is, then.
--
Michael