Re: Add comment explaining why queryid is int64 in pg_stat_statements

On Sun, May 18, 2025 at 3:48 AM Shaik Mohammad Mujeeb <
mujeeb(dot)sk(at)zohocorp(dot)com> wrote:

> 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.
>

As stated in the commit message, changing the representation of queryId
from uint32 to uint64 significantly reduces the chances of two different
queries generating the same queryId.

Since queryId is not incremented sequentially, the overflow issue you
referenced
should not be a concern. Furthermore, converting between int64 and uint64
does not result in any loss of information.

>
> 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
> <michael(at)paquier(dot)xyz <michael(at)paquier(dot)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
>
>
>
>

--
Regards
Junwang Zhao