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+3-4
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+3-4
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+2-4
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