Hi Michael Paquier, > I don't quite see the value in the comment addition you are suggesting
> here: all the user-facing features related to query IDs use signed 64b > integers, and are documented as such in the official docs. The fact > that we store an unsigned value in the backend core code is an > internal implementation artifact, the important point is that we have > a value stored in 8 bytes. Originally, queryId used to be of type uint32, which fit into int64 without any overflow. However, after commit cff440d36 (which changed queryId to uint64), overflow started happening when assigning it to an int64. In that commit, queryId was updated to uint64 in most places - except in pg_stat_statements_internal(). Given that this was an intentional choice, I feel a brief comment should have been included in that commit itself to clarify the reasoning. Although the commit was made back in 2017, this discrepancy still causes confusion. I found myself revisiting and analyzing it simply because there was no explanatory comment. Others might raise the same question again in the future unless this is clarified explicitly. I also noticed that a similar comment is already present in ExplainPrintPlan(): /* * Output the queryid as an int64 rather than a uint64 so we match * what would be seen in the BIGINT pg_stat_statements.queryid column. */ Since such a comment exists there, I believe it would be equally reasonable and helpful to include a similar one in pg_stat_statements_internal() as well. Of course, if this still seems unnecessary, I’m happy to drop the suggestion - just wanted to put it forward in case it helps reduce ambiguity for future readers or contributors. Thanks and Regards, Shaik Mohammad Mujeeb Member Technical Staff Zoho Corp ---- On Sat, 17 May 2025 18:19:23 +0530 Michael Paquier <mich...@paquier.xyz> wrote --- On Fri, May 16, 2025 at 04:05:01PM +0530, Shaik Mohammad Mujeeb wrote: > This conversion is intentional - most likely to match the bigint > type of the queryid column in pg_stat_statements. However, without > an explicit comment, this can be misleading. A beginner reading this > might misinterpret it as an unintentional overflow or bug and raise > unnecessary concerns. Therefore, it´s worth adding a brief comment > clarifying the intent behind this assignment. I don't quite see the value in the comment addition you are suggesting here: all the user-facing features related to query IDs use signed 64b integers, and are documented as such in the official docs. The fact that we store an unsigned value in the backend core code is an internal implementation artifact, the important point is that we have a value stored in 8 bytes. -- Michael
