An obsolete comment of pg_stat_statements
Hello.
I noticed obsolete lines in the function comment of pgss_store().
* If queryId is 0 then this is a utility statement for which we couldn't
* compute a queryId during parse analysis, and we should compute a suitable
* queryId internally.
Previously the function actually calculates queryId using
pgss_hash_string when the given queryId is 0, but since 14 the
function simply rejects to work. We can just drop the paragraph. Or
we can emphasize the change of the behavior by describing the current
behavior for the value.
The attached patch is doing the latter.
* queryId is supposed to be a valid value, otherwise this function dosen't
* calucate it by its own as before then returns immediately.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
Attachments:
revise_comment_of_pgss_store.patchtext/x-patch; charset=us-asciiDownload
diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index 726ba59e2b..59291a8334 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -1191,10 +1191,6 @@ pgss_ProcessUtility(PlannedStmt *pstmt, const char *queryString,
/*
* Store some statistics for a statement.
*
- * If queryId is 0 then this is a utility statement for which we couldn't
- * compute a queryId during parse analysis, and we should compute a suitable
- * queryId internally.
- *
* If jstate is not NULL then we're trying to create an entry for which
* we have no statistics as yet; we just want to record the normalized
* query string. total_time, rows, bufusage and walusage are ignored in this
@@ -1202,6 +1198,9 @@ pgss_ProcessUtility(PlannedStmt *pstmt, const char *queryString,
*
* If kind is PGSS_PLAN or PGSS_EXEC, its value is used as the array position
* for the arrays in the Counters field.
+ *
+ * queryId is supposed to be a valid value, otherwise this function dosen't
+ * calucate it by its own as before then returns immediately.
*/
static void
pgss_store(const char *query, uint64 queryId,
At Mon, 22 Nov 2021 15:38:23 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in
* queryId is supposed to be a valid value, otherwise this function dosen't
* calucate it by its own as before then returns immediately.
Mmm. That's bad. This is the correted version.
* queryId is supposed to be a valid value, otherwise this function doesn't
* calculate it by its own as before then returns immediately.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
Attachments:
revise_comment_of_pgss_store.patchtext/x-patch; charset=us-asciiDownload
diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index 726ba59e2b..7bd7ecf7b5 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -1191,10 +1191,6 @@ pgss_ProcessUtility(PlannedStmt *pstmt, const char *queryString,
/*
* Store some statistics for a statement.
*
- * If queryId is 0 then this is a utility statement for which we couldn't
- * compute a queryId during parse analysis, and we should compute a suitable
- * queryId internally.
- *
* If jstate is not NULL then we're trying to create an entry for which
* we have no statistics as yet; we just want to record the normalized
* query string. total_time, rows, bufusage and walusage are ignored in this
@@ -1202,6 +1198,9 @@ pgss_ProcessUtility(PlannedStmt *pstmt, const char *queryString,
*
* If kind is PGSS_PLAN or PGSS_EXEC, its value is used as the array position
* for the arrays in the Counters field.
+ *
+ * queryId is supposed to be a valid value, otherwise this function doesn't
+ * calculate it by its own as before then returns immediately.
*/
static void
pgss_store(const char *query, uint64 queryId,
On Mon, Nov 22, 2021 at 2:48 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
At Mon, 22 Nov 2021 15:38:23 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in
* queryId is supposed to be a valid value, otherwise this function dosen't
* calucate it by its own as before then returns immediately.Mmm. That's bad. This is the correted version.
* queryId is supposed to be a valid value, otherwise this function doesn't
* calculate it by its own as before then returns immediately.
Ah good catch! Indeed the semantics changed and I missed that comment.
I think that the new comment should be a bit more precise about what
is a valid value and should probably not refer to a previous version
of the code. How about something like:
- * If queryId is 0 then this is a utility statement for which we couldn't
- * compute a queryId during parse analysis, and we should compute a suitable
- * queryId internally.
+ * If queryId is 0 then no query fingerprinting source has been enabled, so we
+ * act as if the extension was disabled: silently exit without doing any work.
*At Mon, 22 Nov 2021 22:50:04 +0800, Julien Rouhaud <rjuju123@gmail.com> wrote in
On Mon, Nov 22, 2021 at 2:48 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:At Mon, 22 Nov 2021 15:38:23 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in
* queryId is supposed to be a valid value, otherwise this function dosen't
* calucate it by its own as before then returns immediately.Mmm. That's bad. This is the correted version.
* queryId is supposed to be a valid value, otherwise this function doesn't
* calculate it by its own as before then returns immediately.Ah good catch! Indeed the semantics changed and I missed that comment.
I think that the new comment should be a bit more precise about what
is a valid value and should probably not refer to a previous version
of the code. How about something like:- * If queryId is 0 then this is a utility statement for which we couldn't - * compute a queryId during parse analysis, and we should compute a suitable - * queryId internally. + * If queryId is 0 then no query fingerprinting source has been enabled, so we + * act as if the extension was disabled: silently exit without doing any work. *
Thanks! Looks better. It is used as-is in the attached.
And I will register this to the next CF.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
Attachments:
0001-Fix-a-function-comment.patchtext/x-patch; charset=us-asciiDownload
From 1563bd869cb8da080e3488e64116da402f79be8c Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Date: Fri, 24 Dec 2021 15:25:57 +0900
Subject: [PATCH] Fix a function comment
Commit 5fd9dfa5f50e4906c35133a414ebec5b6d518493 forgot to fix the
comment. Fix it so that it desribes the new behavior.
Author: Julien Rouhaud
Reviewed-by: Kyotaro Horiguchi
---
contrib/pg_stat_statements/pg_stat_statements.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index 726ba59e2b..3224da9275 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -1191,9 +1191,8 @@ pgss_ProcessUtility(PlannedStmt *pstmt, const char *queryString,
/*
* Store some statistics for a statement.
*
- * If queryId is 0 then this is a utility statement for which we couldn't
- * compute a queryId during parse analysis, and we should compute a suitable
- * queryId internally.
+ * If queryId is 0 then no query fingerprinting source has been enabled, so we
+ * act as if the extension was disabled: silently exit without doing any work.
*
* If jstate is not NULL then we're trying to create an entry for which
* we have no statistics as yet; we just want to record the normalized
--
2.27.0
On Fri, Dec 24, 2021 at 03:32:10PM +0900, Kyotaro Horiguchi wrote:
Thanks! Looks better. It is used as-is in the attached.
And I will register this to the next CF.
Do we really need to have this comment in the function header? The
same is explained a couple of lines down so this feels like a
duplicate, and it is hard to miss it with the code shaped as-is (aka
the relationship between compute_query_id and queryId and the
consequences on what's stored in this case).
--
Michael
On Fri, Dec 24, 2021 at 09:02:10PM +0900, Michael Paquier wrote:
Do we really need to have this comment in the function header? The
same is explained a couple of lines down so this feels like a
duplicate, and it is hard to miss it with the code shaped as-is (aka
the relationship between compute_query_id and queryId and the
consequences on what's stored in this case).
The simpler the better here. So, I have just removed this comment
after thinking more about this.
--
Michael
At Mon, 3 Jan 2022 17:36:25 +0900, Michael Paquier <michael@paquier.xyz> wrote in
On Fri, Dec 24, 2021 at 09:02:10PM +0900, Michael Paquier wrote:
Do we really need to have this comment in the function header? The
same is explained a couple of lines down so this feels like a
duplicate, and it is hard to miss it with the code shaped as-is (aka
the relationship between compute_query_id and queryId and the
consequences on what's stored in this case).The simpler the better here. So, I have just removed this comment
after thinking more about this.
I'm fine with it. Thanks!
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center