Add comment explaining why queryid is int64 in pg_stat_statements

Started by Shaik Mohammad Mujeeb8 months ago32 messages

mujeeb.sk@zohocorp.com

8 months ago

1 attachment(s)

Hi Developers,

In pg_stat_statements.c, the function pg_stat_statements_internal() declares the queryid variable as int64, but assigns it a value of type uint64. At first glance, this might appear to be an overflow issue. However, I think this is intentional - the queryid is cast to int64 to match the bigint type of the queryid column in the pg_stat_statements view.

Please find the attached patch, which adds a clarifying comment to make the rationale explicit and avoid potential confusion for future readers.

Thanks and Regards,
Shaik Mohammad Mujeeb
Member Technical Staff
Zoho Corp

Attachments:

add_comment_for_why_queryid_is_int64.patchapplication/octet-stream; name=add_comment_for_why_queryid_is_int64.patchDownload

diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index 9778407cba3..31ef85402e0 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -1812,6 +1812,8 @@ pg_stat_statements_internal(FunctionCallInfo fcinfo,
 		int			i = 0;
 		Counters	tmp;
 		double		stddev;
+		
+		/* Using int64 for queryid to match the bigint type of the queryid column in pg_stat_statements */
 		int64		queryid = entry->key.queryid;
 		TimestampTz stats_since;
 		TimestampTz minmax_stats_since;

Import Notes

Reply to msg id not found:

Ilia Evdokimov

ilya.evdokimov@tantorlabs.com

8 months ago

In reply to: Shaik Mohammad Mujeeb (#1)

Re: Add comment explaining why queryid is int64 in pg_stat_statements

On 15.05.2025 10:08, Shaik Mohammad Mujeeb wrote:

Hi Developers,

In pg_stat_statements.c, the function /pg_stat_statements_internal()/
declares the /queryid/ variable as *int64*, but assigns it a value of
type *uint64*. At first glance, this might appear to be an overflow
issue. However, I think this is intentional - the queryid is cast to
/int64/ *to match the bigint type of the queryid column in the
pg_stat_statements view*.

Please find the attached patch, which adds a clarifying comment to
make the rationale explicit and avoid potential confusion for future
readers.

Thanks and Regards,
Shaik Mohammad Mujeeb
Member Technical Staff
Zoho Corp

I don't think the comment is necessary here. There are no arithmetic or
logical operations performed on it. It is only passed as a Datum.

--
Best regards,
Ilia Evdokimov,
Tantor Labs LLC.

Shaik Mohammad Mujeeb

mujeeb.sk@zohocorp.com

8 months ago

In reply to: Ilia Evdokimov (#2)

Re: Add comment explaining why queryid is int64 in pg_stat_statements

Hi Ilia Evdokimov,

While it's true that no arithmetic or logical operations are performed on queryid after the assignment, the overflow technically occurs at the point of assignment itself. For example, entry->key.queryid holds the value 12747288675711951805 as a uint64, but after assigning it to queryid (which is an int64), it becomes -5699455397997599811 due to overflow.

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.

Thanks & Regards,
Shaik Mohammad Mujeeb
Member Technical Staff

Zoho Corp

---- On Fri, 16 May 2025 15:12:41 +0530 Ilia Evdokimov <ilya.evdokimov@tantorlabs.com> wrote ---

On 15.05.2025 10:08, Shaik Mohammad
Mujeeb wrote:

I don't think the comment is necessary here. There are no
arithmetic or logical operations performed on it. It is only
passed as a Datum.

--
Best regards,
Ilia Evdokimov,
Tantor Labs LLC.

Hi Developers,

In pg_stat_statements.c, the function pg_stat_statements_internal() declares the queryid variable as int64, but
assigns it a value of type uint64. At first glance,
this might appear to be an overflow issue. However, I think
this is intentional - the queryid is cast to int64 to
match the bigint type of the queryid column in the
pg_stat_statements view.

Please find the attached patch, which adds a clarifying
comment to make the rationale explicit and avoid potential
confusion for future readers.

Thanks and Regards,
Shaik Mohammad Mujeeb
Member Technical Staff
Zoho Corp

wenhui qiu

qiuwenhuifx@gmail.com

8 months ago

In reply to: Shaik Mohammad Mujeeb (#3)

Re: Add comment explaining why queryid is int64 in pg_stat_statements

Hi Shaik

While it's true that no arithmetic or logical operations are performed on

queryid after the assignment, the overflow technically > occurs at the
point of assignment itself. For example, *entry->key.queryid* holds the
value *12747288675711951805* as a

*uint64*, but after assigning it to *queryid* (which is an *int64*), it

becomes *-5699455397997599811* *due to overflow*.

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.

+1 agree

On Sat, May 17, 2025 at 1:54 AM Shaik Mohammad Mujeeb <
mujeeb.sk@zohocorp.com> wrote:

Show quoted text

Hi Ilia Evdokimov,

While it's true that no arithmetic or logical operations are performed on
queryid after the assignment, the overflow technically occurs at the
point of assignment itself. For example, *entry->key.queryid* holds the
value *12747288675711951805* as a *uint64*, but after assigning it to
*queryid* (which is an *int64*), it becomes *-5699455397997599811* *due
to overflow*.

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.

Thanks & Regards,
Shaik Mohammad Mujeeb
Member Technical Staff
Zoho Corp

---- On Fri, 16 May 2025 15:12:41 +0530 *Ilia Evdokimov
<ilya.evdokimov@tantorlabs.com <ilya.evdokimov@tantorlabs.com>>* wrote ---

On 15.05.2025 10:08, Shaik Mohammad Mujeeb wrote:

I don't think the comment is necessary here. There are no arithmetic or
logical operations performed on it. It is only passed as a Datum.

--
Best regards,
Ilia Evdokimov,
Tantor Labs LLC.

Hi Developers,

In pg_stat_statements.c, the function *pg_stat_statements_internal()*
declares the *queryid* variable as *int64*, but assigns it a value of
type *uint64*. At first glance, this might appear to be an overflow
issue. However, I think this is intentional - the queryid is cast to
*int64* *to match the bigint type of the queryid column in the
pg_stat_statements view*.

Please find the attached patch, which adds a clarifying comment to make
the rationale explicit and avoid potential confusion for future readers.

Thanks and Regards,
Shaik Mohammad Mujeeb
Member Technical Staff
Zoho Corp

Michael Paquier

michael@paquier.xyz

8 months ago

In reply to: Shaik Mohammad Mujeeb (#3)

Re: Add comment explaining why queryid is int64 in pg_stat_statements

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

Shaik Mohammad Mujeeb

mujeeb.sk@zohocorp.com

8 months ago

In reply to: Michael Paquier (#5)

Re: Add comment explaining why queryid is int64 in pg_stat_statements

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

Junwang Zhao

zhjwpku@gmail.com

8 months ago

In reply to: Shaik Mohammad Mujeeb (#6)

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.sk@zohocorp.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@paquier.xyz <michael@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

--
Regards
Junwang Zhao

Peter Eisentraut

peter@eisentraut.org

8 months ago

In reply to: Michael Paquier (#5)

Re: Add comment explaining why queryid is int64 in pg_stat_statements

On 17.05.25 14:49, Michael Paquier 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.

There is another, new instance of this confusion. When query IDs are
printed to the log, we use a signed-integer format, but the value is
unsigned (see log_status_format(), write_jsonlog()). This works
correctly in practice, but it triggers warnings from
-Wwarnings-format-signedness (not in the default warnings set, but
useful to clean up occasionally) and maybe other tools along those
lines. This is new because in the past this warning was hidden by
additional casts. If we want to keep this arrangement, maybe we should
create an explicit function to convert query IDs for output, and then
explain all this there. Or why not store query IDs as int64_t
internally, too?

Sami Imseih

samimseih@gmail.com

8 months ago

In reply to: Peter Eisentraut (#8)

Re: Add comment explaining why queryid is int64 in pg_stat_statements

FWIW, all the hash function SQL interfaces, \df hash*,
have this behavior in which the result is a signed (int/bigint),
but the internal representation of the hash is an unsigned (int/bigint).

I am not sure why a comment is needed specifically for pg_stat_statements

--
Sami Imseih
Amazon Web Services (AWS)

#10

David Rowley

dgrowleyml@gmail.com

8 months ago

In reply to: Peter Eisentraut (#8)

1 attachment(s)

Re: Add comment explaining why queryid is int64 in pg_stat_statements

On Tue, 20 May 2025 at 03:05, Peter Eisentraut <peter@eisentraut.org> wrote:

Or why not store query IDs as int64_t
internally, too?

I had the same thought. Changing to int64 seems like a good and less
bug-prone tidy-up. I expected we ended up with uint64 as the previous
type was uint32, and uint64 is the natural selection for widening that
to 64 bits.

Aside from the struct field types changing for Query.queryId,
PlannedStmt.queryId and PgBackendStatus.st_query_id, the
external-facing changes are limited to:

1. pgstat_report_query_id() now returns int64 instead of uint64
2. pgstat_get_my_query_id() now returns int64 instead of uint64
3. pgstat_report_query_id()'s first input parameter is now int64

If we were to clean this up, then I expect it's fine to wait until
v19, as it's not really a problem that's new to v18.

David

Attachments:

v1-0001-Change-internal-queryid-type-to-int64.patchapplication/octet-stream; name=v1-0001-Change-internal-queryid-type-to-int64.patchDownload

From 09c7cdb518bf0e88d354e27a3929c6c4c3b36549 Mon Sep 17 00:00:00 2001
From: David Rowley <dgrowley@gmail.com>
Date: Tue, 20 May 2025 12:43:18 +1200
Subject: [PATCH v1] Change internal queryid type to int64

Previously, this was uint64, which perhaps was chosen in cff440d36 as the
previous type was uint32 prior to that widening.

Having this as uint64 does require us to have to cast the value in
various places since the value is output in its signed form in various
places, such as EXPLAIN VERBOSE and in the pg_stat_statements.queryid
column.

We should likely note down this change in the release notes "Source
Code" section as some extensions may wish to adjust their code.

Discussion: https://postgr.es/m/50cb0c8b-994b-48f9-a1c4-13039eb3536b@eisentraut.org
---
 .../pg_stat_statements/pg_stat_statements.c   | 36 +++++++++----------
 src/backend/access/brin/brin.c                |  2 +-
 src/backend/access/nbtree/nbtsort.c           |  2 +-
 src/backend/commands/explain.c                |  8 ++---
 src/backend/commands/vacuumparallel.c         |  2 +-
 src/backend/nodes/gen_node_support.pl         |  5 +++
 src/backend/nodes/outfuncs.c                  |  6 ++++
 src/backend/nodes/queryjumblefuncs.c          | 22 ++++++------
 src/backend/nodes/readfuncs.c                 |  6 ++++
 src/backend/rewrite/rewriteHandler.c          |  2 +-
 src/backend/tcop/postgres.c                   |  4 +--
 src/backend/utils/activity/backend_status.c   | 12 +++----
 src/backend/utils/adt/pgstatfuncs.c           |  4 +--
 src/include/nodes/parsenodes.h                |  2 +-
 src/include/nodes/plannodes.h                 |  2 +-
 src/include/utils/backend_status.h            |  6 ++--
 16 files changed, 67 insertions(+), 54 deletions(-)

diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index 9778407cba3..7d7bcc68dfa 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -144,7 +144,7 @@ typedef struct pgssHashKey
 {
 	Oid			userid;			/* user OID */
 	Oid			dbid;			/* database OID */
-	uint64		queryid;		/* query identifier */
+	int64		queryid;		/* query identifier */
 	bool		toplevel;		/* query executed at top level */
 } pgssHashKey;
 
@@ -346,7 +346,7 @@ static void pgss_ProcessUtility(PlannedStmt *pstmt, const char *queryString,
 								ProcessUtilityContext context, ParamListInfo params,
 								QueryEnvironment *queryEnv,
 								DestReceiver *dest, QueryCompletion *qc);
-static void pgss_store(const char *query, uint64 queryId,
+static void pgss_store(const char *query, int64 queryId,
 					   int query_location, int query_len,
 					   pgssStoreKind kind,
 					   double total_time, uint64 rows,
@@ -370,7 +370,7 @@ static char *qtext_fetch(Size query_offset, int query_len,
 						 char *buffer, Size buffer_size);
 static bool need_gc_qtexts(void);
 static void gc_qtexts(void);
-static TimestampTz entry_reset(Oid userid, Oid dbid, uint64 queryid, bool minmax_only);
+static TimestampTz entry_reset(Oid userid, Oid dbid, int64 queryid, bool minmax_only);
 static char *generate_normalized_query(JumbleState *jstate, const char *query,
 									   int query_loc, int *query_len_p);
 static void fill_in_constant_lengths(JumbleState *jstate, const char *query,
@@ -852,7 +852,7 @@ pgss_post_parse_analyze(ParseState *pstate, Query *query, JumbleState *jstate)
 	{
 		if (pgss_track_utility && IsA(query->utilityStmt, ExecuteStmt))
 		{
-			query->queryId = UINT64CONST(0);
+			query->queryId = INT64CONST(0);
 			return;
 		}
 	}
@@ -899,7 +899,7 @@ pgss_planner(Query *parse,
 	 */
 	if (pgss_enabled(nesting_level)
 		&& pgss_track_planning && query_string
-		&& parse->queryId != UINT64CONST(0))
+		&& parse->queryId != INT64CONST(0))
 	{
 		instr_time	start;
 		instr_time	duration;
@@ -1008,7 +1008,7 @@ pgss_ExecutorStart(QueryDesc *queryDesc, int eflags)
 	 * counting of optimizable statements that are directly contained in
 	 * utility statements.
 	 */
-	if (pgss_enabled(nesting_level) && queryDesc->plannedstmt->queryId != UINT64CONST(0))
+	if (pgss_enabled(nesting_level) && queryDesc->plannedstmt->queryId != INT64CONST(0))
 	{
 		/*
 		 * Set up to track total elapsed time in ExecutorRun.  Make sure the
@@ -1076,9 +1076,9 @@ pgss_ExecutorFinish(QueryDesc *queryDesc)
 static void
 pgss_ExecutorEnd(QueryDesc *queryDesc)
 {
-	uint64		queryId = queryDesc->plannedstmt->queryId;
+	int64		queryId = queryDesc->plannedstmt->queryId;
 
-	if (queryId != UINT64CONST(0) && queryDesc->totaltime &&
+	if (queryId != INT64CONST(0) && queryDesc->totaltime &&
 		pgss_enabled(nesting_level))
 	{
 		/*
@@ -1139,7 +1139,7 @@ pgss_ProcessUtility(PlannedStmt *pstmt, const char *queryString,
 	 * only.
 	 */
 	if (enabled)
-		pstmt->queryId = UINT64CONST(0);
+		pstmt->queryId = INT64CONST(0);
 
 	/*
 	 * If it's an EXECUTE statement, we don't track it and don't increment the
@@ -1286,7 +1286,7 @@ pgss_ProcessUtility(PlannedStmt *pstmt, const char *queryString,
  * for the arrays in the Counters field.
  */
 static void
-pgss_store(const char *query, uint64 queryId,
+pgss_store(const char *query, int64 queryId,
 		   int query_location, int query_len,
 		   pgssStoreKind kind,
 		   double total_time, uint64 rows,
@@ -1312,7 +1312,7 @@ pgss_store(const char *query, uint64 queryId,
 	 * Nothing to do if compute_query_id isn't enabled and no other module
 	 * computed a query identifier.
 	 */
-	if (queryId == UINT64CONST(0))
+	if (queryId == INT64CONST(0))
 		return;
 
 	/*
@@ -1522,11 +1522,11 @@ pg_stat_statements_reset_1_7(PG_FUNCTION_ARGS)
 {
 	Oid			userid;
 	Oid			dbid;
-	uint64		queryid;
+	int64		queryid;
 
 	userid = PG_GETARG_OID(0);
 	dbid = PG_GETARG_OID(1);
-	queryid = (uint64) PG_GETARG_INT64(2);
+	queryid = PG_GETARG_INT64(2);
 
 	entry_reset(userid, dbid, queryid, false);
 
@@ -1538,12 +1538,12 @@ pg_stat_statements_reset_1_11(PG_FUNCTION_ARGS)
 {
 	Oid			userid;
 	Oid			dbid;
-	uint64		queryid;
+	int64		queryid;
 	bool		minmax_only;
 
 	userid = PG_GETARG_OID(0);
 	dbid = PG_GETARG_OID(1);
-	queryid = (uint64) PG_GETARG_INT64(2);
+	queryid = PG_GETARG_INT64(2);
 	minmax_only = PG_GETARG_BOOL(3);
 
 	PG_RETURN_TIMESTAMPTZ(entry_reset(userid, dbid, queryid, minmax_only));
@@ -2679,7 +2679,7 @@ if (e) { \
  * Reset entries corresponding to parameters passed.
  */
 static TimestampTz
-entry_reset(Oid userid, Oid dbid, uint64 queryid, bool minmax_only)
+entry_reset(Oid userid, Oid dbid, int64 queryid, bool minmax_only)
 {
 	HASH_SEQ_STATUS hash_seq;
 	pgssEntry  *entry;
@@ -2699,7 +2699,7 @@ entry_reset(Oid userid, Oid dbid, uint64 queryid, bool minmax_only)
 
 	stats_reset = GetCurrentTimestamp();
 
-	if (userid != 0 && dbid != 0 && queryid != UINT64CONST(0))
+	if (userid != 0 && dbid != 0 && queryid != INT64CONST(0))
 	{
 		/* If all the parameters are available, use the fast path. */
 		memset(&key, 0, sizeof(pgssHashKey));
@@ -2722,7 +2722,7 @@ entry_reset(Oid userid, Oid dbid, uint64 queryid, bool minmax_only)
 
 		SINGLE_ENTRY_RESET(entry);
 	}
-	else if (userid != 0 || dbid != 0 || queryid != UINT64CONST(0))
+	else if (userid != 0 || dbid != 0 || queryid != INT64CONST(0))
 	{
 		/* Reset entries corresponding to valid parameters. */
 		hash_seq_init(&hash_seq, pgss_hash);
diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c
index 01e1db7f856..4204088fa0d 100644
--- a/src/backend/access/brin/brin.c
+++ b/src/backend/access/brin/brin.c
@@ -68,7 +68,7 @@ typedef struct BrinShared
 	int			scantuplesortstates;
 
 	/* Query ID, for report in worker processes */
-	uint64		queryid;
+	int64		queryid;
 
 	/*
 	 * workersdonecv is used to monitor the progress of workers.  All parallel
diff --git a/src/backend/access/nbtree/nbtsort.c b/src/backend/access/nbtree/nbtsort.c
index 3794cc924ad..9d70e89c1f3 100644
--- a/src/backend/access/nbtree/nbtsort.c
+++ b/src/backend/access/nbtree/nbtsort.c
@@ -105,7 +105,7 @@ typedef struct BTShared
 	int			scantuplesortstates;
 
 	/* Query ID, for report in worker processes */
-	uint64		queryid;
+	int64		queryid;
 
 	/*
 	 * workersdonecv is used to monitor the progress of workers.  All parallel
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index 786ee865f14..159a4749674 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -823,14 +823,10 @@ ExplainPrintPlan(ExplainState *es, QueryDesc *queryDesc)
 	 * the queryid in any of the EXPLAIN plans to keep stable the results
 	 * generated by regression test suites.
 	 */
-	if (es->verbose && queryDesc->plannedstmt->queryId != UINT64CONST(0) &&
+	if (es->verbose && queryDesc->plannedstmt->queryId != INT64CONST(0) &&
 		compute_query_id != COMPUTE_QUERY_ID_REGRESS)
 	{
-		/*
-		 * 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.
-		 */
-		ExplainPropertyInteger("Query Identifier", NULL, (int64)
+		ExplainPropertyInteger("Query Identifier", NULL,
 							   queryDesc->plannedstmt->queryId, es);
 	}
 }
diff --git a/src/backend/commands/vacuumparallel.c b/src/backend/commands/vacuumparallel.c
index 2b9d548cdeb..0feea1d30ec 100644
--- a/src/backend/commands/vacuumparallel.c
+++ b/src/backend/commands/vacuumparallel.c
@@ -63,7 +63,7 @@ typedef struct PVShared
 	 */
 	Oid			relid;
 	int			elevel;
-	uint64		queryid;
+	int64		queryid;
 
 	/*
 	 * Fields for both index vacuum and cleanup.
diff --git a/src/backend/nodes/gen_node_support.pl b/src/backend/nodes/gen_node_support.pl
index 77659b0f760..c8595109b0e 100644
--- a/src/backend/nodes/gen_node_support.pl
+++ b/src/backend/nodes/gen_node_support.pl
@@ -1039,6 +1039,11 @@ _read${n}(void)
 			print $off "\tWRITE_UINT_FIELD($f);\n";
 			print $rff "\tREAD_UINT_FIELD($f);\n" unless $no_read;
 		}
+		elsif ($t eq 'int64')
+		{
+			print $off "\tWRITE_INT64_FIELD($f);\n";
+			print $rff "\tREAD_INT64_FIELD($f);\n" unless $no_read;
+		}
 		elsif ($t eq 'uint64'
 			|| $t eq 'AclMode')
 		{
diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index ceac3fd8620..25e08ba3426 100644
--- a/src/backend/nodes/outfuncs.c
+++ b/src/backend/nodes/outfuncs.c
@@ -51,6 +51,12 @@ static void outDouble(StringInfo str, double d);
 #define WRITE_UINT_FIELD(fldname) \
 	appendStringInfo(str, " :" CppAsString(fldname) " %u", node->fldname)
 
+/* Write a signed integer field (anything written with INT64_FORMAT) */
+#define WRITE_INT64_FIELD(fldname) \
+	appendStringInfo(str, \
+					 " :" CppAsString(fldname) " " INT64_FORMAT, \
+					 node->fldname)
+
 /* Write an unsigned integer field (anything written with UINT64_FORMAT) */
 #define WRITE_UINT64_FIELD(fldname) \
 	appendStringInfo(str, " :" CppAsString(fldname) " " UINT64_FORMAT, \
diff --git a/src/backend/nodes/queryjumblefuncs.c b/src/backend/nodes/queryjumblefuncs.c
index d1e82a63f09..ac3cb3d9caf 100644
--- a/src/backend/nodes/queryjumblefuncs.c
+++ b/src/backend/nodes/queryjumblefuncs.c
@@ -56,7 +56,7 @@ int			compute_query_id = COMPUTE_QUERY_ID_AUTO;
 bool		query_id_enabled = false;
 
 static JumbleState *InitJumble(void);
-static uint64 DoJumble(JumbleState *jstate, Node *node);
+static int64 DoJumble(JumbleState *jstate, Node *node);
 static void AppendJumble(JumbleState *jstate,
 						 const unsigned char *value, Size size);
 static void FlushPendingNulls(JumbleState *jstate);
@@ -141,12 +141,12 @@ JumbleQuery(Query *query)
 	 * If we are unlucky enough to get a hash of zero, use 1 instead for
 	 * normal statements and 2 for utility queries.
 	 */
-	if (query->queryId == UINT64CONST(0))
+	if (query->queryId == INT64CONST(0))
 	{
 		if (query->utilityStmt)
-			query->queryId = UINT64CONST(2);
+			query->queryId = INT64CONST(2);
 		else
-			query->queryId = UINT64CONST(1);
+			query->queryId = INT64CONST(1);
 	}
 
 	return jstate;
@@ -197,7 +197,7 @@ InitJumble(void)
  *		Jumble the given Node using the given JumbleState and return the resulting
  *		jumble hash.
  */
-static uint64
+static int64
 DoJumble(JumbleState *jstate, Node *node)
 {
 	/* Jumble the given node */
@@ -208,9 +208,9 @@ DoJumble(JumbleState *jstate, Node *node)
 		FlushPendingNulls(jstate);
 
 	/* Process the jumble buffer and produce the hash value */
-	return DatumGetUInt64(hash_any_extended(jstate->jumble,
-											jstate->jumble_len,
-											0));
+	return DatumGetInt64(hash_any_extended(jstate->jumble,
+										   jstate->jumble_len,
+										   0));
 }
 
 /*
@@ -256,10 +256,10 @@ AppendJumbleInternal(JumbleState *jstate, const unsigned char *item,
 
 		if (unlikely(jumble_len >= JUMBLE_SIZE))
 		{
-			uint64		start_hash;
+			int64		start_hash;
 
-			start_hash = DatumGetUInt64(hash_any_extended(jumble,
-														  JUMBLE_SIZE, 0));
+			start_hash = DatumGetInt64(hash_any_extended(jumble,
+														 JUMBLE_SIZE, 0));
 			memcpy(jumble, &start_hash, sizeof(start_hash));
 			jumble_len = sizeof(start_hash);
 		}
diff --git a/src/backend/nodes/readfuncs.c b/src/backend/nodes/readfuncs.c
index 64d3a09f765..8c90ab54af8 100644
--- a/src/backend/nodes/readfuncs.c
+++ b/src/backend/nodes/readfuncs.c
@@ -68,6 +68,12 @@
 	token = pg_strtok(&length);		/* get field value */ \
 	local_node->fldname = atoui(token)
 
+/* Read a signed integer field (anything written using INT64_FORMAT) */
+#define READ_INT64_FIELD(fldname) \
+	token = pg_strtok(&length); /* skip :fldname */ \
+	token = pg_strtok(&length); /* get field value */ \
+	local_node->fldname = strtoi64(token, NULL, 10)
+
 /* Read an unsigned integer field (anything written using UINT64_FORMAT) */
 #define READ_UINT64_FIELD(fldname) \
 	token = pg_strtok(&length);		/* skip :fldname */ \
diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c
index f0bce5f9ed9..2ef0e7fbf3a 100644
--- a/src/backend/rewrite/rewriteHandler.c
+++ b/src/backend/rewrite/rewriteHandler.c
@@ -4544,7 +4544,7 @@ build_generation_expression(Relation rel, int attrno)
 List *
 QueryRewrite(Query *parsetree)
 {
-	uint64		input_query_id = parsetree->queryId;
+	int64		input_query_id = parsetree->queryId;
 	List	   *querylist;
 	List	   *results;
 	ListCell   *l;
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 1ae51b1b391..7caf740d396 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -1683,7 +1683,7 @@ exec_bind_message(StringInfo input_message)
 	{
 		Query	   *query = lfirst_node(Query, lc);
 
-		if (query->queryId != UINT64CONST(0))
+		if (query->queryId != INT64CONST(0))
 		{
 			pgstat_report_query_id(query->queryId, false);
 			break;
@@ -2176,7 +2176,7 @@ exec_execute_message(const char *portal_name, long max_rows)
 	{
 		PlannedStmt *stmt = lfirst_node(PlannedStmt, lc);
 
-		if (stmt->queryId != UINT64CONST(0))
+		if (stmt->queryId != INT64CONST(0))
 		{
 			pgstat_report_query_id(stmt->queryId, false);
 			break;
diff --git a/src/backend/utils/activity/backend_status.c b/src/backend/utils/activity/backend_status.c
index e1576e64b6d..9c2ed2cb9e0 100644
--- a/src/backend/utils/activity/backend_status.c
+++ b/src/backend/utils/activity/backend_status.c
@@ -320,7 +320,7 @@ pgstat_bestart_initial(void)
 	lbeentry.st_state = STATE_STARTING;
 	lbeentry.st_progress_command = PROGRESS_COMMAND_INVALID;
 	lbeentry.st_progress_command_target = InvalidOid;
-	lbeentry.st_query_id = UINT64CONST(0);
+	lbeentry.st_query_id = INT64CONST(0);
 	lbeentry.st_plan_id = UINT64CONST(0);
 
 	/*
@@ -599,7 +599,7 @@ pgstat_report_activity(BackendState state, const char *cmd_str)
 			beentry->st_activity_start_timestamp = 0;
 			/* st_xact_start_timestamp and wait_event_info are also disabled */
 			beentry->st_xact_start_timestamp = 0;
-			beentry->st_query_id = UINT64CONST(0);
+			beentry->st_query_id = INT64CONST(0);
 			beentry->st_plan_id = UINT64CONST(0);
 			proc->wait_event_info = 0;
 			PGSTAT_END_WRITE_ACTIVITY(beentry);
@@ -662,7 +662,7 @@ pgstat_report_activity(BackendState state, const char *cmd_str)
 	 */
 	if (state == STATE_RUNNING)
 	{
-		beentry->st_query_id = UINT64CONST(0);
+		beentry->st_query_id = INT64CONST(0);
 		beentry->st_plan_id = UINT64CONST(0);
 	}
 
@@ -683,7 +683,7 @@ pgstat_report_activity(BackendState state, const char *cmd_str)
  * --------
  */
 void
-pgstat_report_query_id(uint64 query_id, bool force)
+pgstat_report_query_id(int64 query_id, bool force)
 {
 	volatile PgBackendStatus *beentry = MyBEEntry;
 
@@ -702,7 +702,7 @@ pgstat_report_query_id(uint64 query_id, bool force)
 	 * command, so ignore the one provided unless it's an explicit call to
 	 * reset the identifier.
 	 */
-	if (beentry->st_query_id != 0 && !force)
+	if (beentry->st_query_id != INT64CONST(0) && !force)
 		return;
 
 	/*
@@ -1134,7 +1134,7 @@ pgstat_get_crashed_backend_activity(int pid, char *buffer, int buflen)
  *
  * Return current backend's query identifier.
  */
-uint64
+int64
 pgstat_get_my_query_id(void)
 {
 	if (!MyBEEntry)
diff --git a/src/backend/utils/adt/pgstatfuncs.c b/src/backend/utils/adt/pgstatfuncs.c
index 97af7c6554f..e980109f245 100644
--- a/src/backend/utils/adt/pgstatfuncs.c
+++ b/src/backend/utils/adt/pgstatfuncs.c
@@ -640,10 +640,10 @@ pg_stat_get_activity(PG_FUNCTION_ARGS)
 				values[28] = BoolGetDatum(false);	/* GSS credentials not
 													 * delegated */
 			}
-			if (beentry->st_query_id == 0)
+			if (beentry->st_query_id == INT64CONST(0))
 				nulls[30] = true;
 			else
-				values[30] = UInt64GetDatum(beentry->st_query_id);
+				values[30] = Int64GetDatum(beentry->st_query_id);
 		}
 		else
 		{
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 4610fc61293..b976a40a75d 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -128,7 +128,7 @@ typedef struct Query
 	 * might not be set; also not stored.  This is the result of the query
 	 * jumble, hence ignored.
 	 */
-	uint64		queryId pg_node_attr(equal_ignore, query_jumble_ignore, read_write_ignore, read_as(0));
+	int64		queryId pg_node_attr(equal_ignore, query_jumble_ignore, read_write_ignore, read_as(0));
 
 	/* do I set the command result tag? */
 	bool		canSetTag pg_node_attr(query_jumble_ignore);
diff --git a/src/include/nodes/plannodes.h b/src/include/nodes/plannodes.h
index 658d76225e4..56cfdca074d 100644
--- a/src/include/nodes/plannodes.h
+++ b/src/include/nodes/plannodes.h
@@ -53,7 +53,7 @@ typedef struct PlannedStmt
 	CmdType		commandType;
 
 	/* query identifier (copied from Query) */
-	uint64		queryId;
+	int64		queryId;
 
 	/* plan identifier (can be set by plugins) */
 	uint64		planId;
diff --git a/src/include/utils/backend_status.h b/src/include/utils/backend_status.h
index 430ccd7d78e..bbebe517501 100644
--- a/src/include/utils/backend_status.h
+++ b/src/include/utils/backend_status.h
@@ -170,7 +170,7 @@ typedef struct PgBackendStatus
 	int64		st_progress_param[PGSTAT_NUM_PROGRESS_PARAM];
 
 	/* query identifier, optionally computed using post_parse_analyze_hook */
-	uint64		st_query_id;
+	int64		st_query_id;
 
 	/* plan identifier, optionally computed using planner_hook */
 	uint64		st_plan_id;
@@ -321,7 +321,7 @@ extern void pgstat_clear_backend_activity_snapshot(void);
 
 /* Activity reporting functions */
 extern void pgstat_report_activity(BackendState state, const char *cmd_str);
-extern void pgstat_report_query_id(uint64 query_id, bool force);
+extern void pgstat_report_query_id(int64 query_id, bool force);
 extern void pgstat_report_plan_id(uint64 plan_id, bool force);
 extern void pgstat_report_tempfile(size_t filesize);
 extern void pgstat_report_appname(const char *appname);
@@ -329,7 +329,7 @@ extern void pgstat_report_xact_timestamp(TimestampTz tstamp);
 extern const char *pgstat_get_backend_current_activity(int pid, bool checkUser);
 extern const char *pgstat_get_crashed_backend_activity(int pid, char *buffer,
 													   int buflen);
-extern uint64 pgstat_get_my_query_id(void);
+extern int64 pgstat_get_my_query_id(void);
 extern uint64 pgstat_get_my_plan_id(void);
 extern BackendType pgstat_get_backend_type_by_proc_number(ProcNumber procNumber);
 
-- 
2.43.0

#11

Michael Paquier

michael@paquier.xyz

8 months ago

In reply to: David Rowley (#10)

Re: Add comment explaining why queryid is int64 in pg_stat_statements

On Tue, May 20, 2025 at 02:03:37PM +1200, David Rowley wrote:

Aside from the struct field types changing for Query.queryId,
PlannedStmt.queryId and PgBackendStatus.st_query_id, the
external-facing changes are limited to:

1. pgstat_report_query_id() now returns int64 instead of uint64
2. pgstat_get_my_query_id() now returns int64 instead of uint64
3. pgstat_report_query_id()'s first input parameter is now int64

If we were to clean this up, then I expect it's fine to wait until
v19, as it's not really a problem that's new to v18.

Hmm. For the query ID, that's not new, but for the plan ID it is. So
it seems to me that there could be also an argument for doing all that
in v18 rather than releasing v18 with the plan ID being unsigned,
keeping a maximum of consistency for both of IDs. AFAIK, this is
something that Lukas's plan storing extension exposes as an int64
value to the user and the SQL interfaces, even if it's true that we
don't expose it in core, yet.
--
Michael

#12

Lukas Fittl

lukas@fittl.com

8 months ago

In reply to: Michael Paquier (#11)

Re: Add comment explaining why queryid is int64 in pg_stat_statements

On Mon, May 19, 2025 at 8:12 PM Michael Paquier <michael@paquier.xyz> wrote:

On Tue, May 20, 2025 at 02:03:37PM +1200, David Rowley wrote:

Aside from the struct field types changing for Query.queryId,
PlannedStmt.queryId and PgBackendStatus.st_query_id, the
external-facing changes are limited to:

1. pgstat_report_query_id() now returns int64 instead of uint64
2. pgstat_get_my_query_id() now returns int64 instead of uint64
3. pgstat_report_query_id()'s first input parameter is now int64

If we were to clean this up, then I expect it's fine to wait until
v19, as it's not really a problem that's new to v18.

Hmm. For the query ID, that's not new, but for the plan ID it is. So
it seems to me that there could be also an argument for doing all that
in v18 rather than releasing v18 with the plan ID being unsigned,
keeping a maximum of consistency for both of IDs. AFAIK, this is
something that Lukas's plan storing extension exposes as an int64
value to the user and the SQL interfaces, even if it's true that we
don't expose it in core, yet.

Yeah, +1 to making this consistent across both query ID and the new plan
ID, and changing both to int64 internally seems best.

Just as an example, in my current work-in-progress version of a new
pg_stat_plans extension the plan ID gets set like this ([0]https://github.com/pganalyze/pg_stat_plans/blob/main/pg_stat_plans.c#L745):

static void
pgsp_calculate_plan_id(PlannedStmt *result)
{
...
result->planId = HashJumbleState(jstate);
...
}

With HashJumbleState currently returning a uint64.

Similarly, when reading out the planID from backends, we do:

values[4] = UInt64GetDatum(beentry->st_plan_id);

if Postgres 18 released as-is, and 19 changed this, we'd have to carry a
version-specific cast from uint64 to int64 in both places. Not a big deal,
but certainly nice to not knowingly introduce this confusion for extension
development.

As an additional data point, the existing pg_stat_monitor extension uses a
uint64 for planID [1]https://github.com/percona/pg_stat_monitor/blob/9c72c2e73d83a6b687ff8e2ddc5072c63afe983b/pg_stat_monitor.h#L212, and pg_store_plans uses a uint32 [2]https://github.com/ossc-db/pg_store_plans/blob/master/pg_store_plans.c#L129. I'd expect
both of these to update to a int64 internally with the new planID being
available in 18.

Thanks,
Lukas

[0]: https://github.com/pganalyze/pg_stat_plans/blob/main/pg_stat_plans.c#L745
https://github.com/pganalyze/pg_stat_plans/blob/main/pg_stat_plans.c#L745
[1]: https://github.com/percona/pg_stat_monitor/blob/9c72c2e73d83a6b687ff8e2ddc5072c63afe983b/pg_stat_monitor.h#L212
https://github.com/percona/pg_stat_monitor/blob/9c72c2e73d83a6b687ff8e2ddc5072c63afe983b/pg_stat_monitor.h#L212
[2]: https://github.com/ossc-db/pg_store_plans/blob/master/pg_store_plans.c#L129
https://github.com/ossc-db/pg_store_plans/blob/master/pg_store_plans.c#L129

--
Lukas Fittl

#13

Michael Paquier

michael@paquier.xyz

8 months ago

In reply to: Lukas Fittl (#12)

Re: Add comment explaining why queryid is int64 in pg_stat_statements

On Mon, May 19, 2025 at 08:43:25PM -0700, Lukas Fittl wrote:

Yeah, +1 to making this consistent across both query ID and the new plan
ID, and changing both to int64 internally seems best.

Any thoughts from others? I'm OK to take the extra step of switching
both fields on HEAD and write a patch for that, relying on what David
has sent as a first step towards that.
--
Michael

#14

David Rowley

dgrowleyml@gmail.com

8 months ago

In reply to: Michael Paquier (#13)

Re: Add comment explaining why queryid is int64 in pg_stat_statements

On Tue, 20 May 2025 at 17:09, Michael Paquier <michael@paquier.xyz> wrote:

On Mon, May 19, 2025 at 08:43:25PM -0700, Lukas Fittl wrote:

Yeah, +1 to making this consistent across both query ID and the new plan
ID, and changing both to int64 internally seems best.

Any thoughts from others? I'm OK to take the extra step of switching
both fields on HEAD and write a patch for that, relying on what David
has sent as a first step towards that.

Given the planId stuff is new and has the same issue, I think that
pushes me towards thinking now is better than later for fixing both.

I'm happy to adjust my patch unless you've started working on it already.

David

#15

Julien Rouhaud

rjuju123@gmail.com

8 months ago

In reply to: Michael Paquier (#13)

Re: Add comment explaining why queryid is int64 in pg_stat_statements

On Tue, May 20, 2025 at 02:09:13PM +0900, Michael Paquier wrote:

On Mon, May 19, 2025 at 08:43:25PM -0700, Lukas Fittl wrote:

Yeah, +1 to making this consistent across both query ID and the new plan
ID, and changing both to int64 internally seems best.

Any thoughts from others? I'm OK to take the extra step of switching
both fields on HEAD and write a patch for that, relying on what David
has sent as a first step towards that.

I don't think it was discussed, but why not go the other way, keep everything
as uint64 and expose an uint64 datatype at the SQL level instead? That's
something I actually want pretty often so I would be happy to have that
natively, whether it's called bigoid, oid8 or anything else. That's probably
too late for pg18 though.

#16

Michael Paquier

michael@paquier.xyz

8 months ago

In reply to: David Rowley (#14)

2 attachment(s)

Re: Add comment explaining why queryid is int64 in pg_stat_statements

On Tue, May 20, 2025 at 05:51:51PM +1200, David Rowley wrote:

Given the planId stuff is new and has the same issue, I think that
pushes me towards thinking now is better than later for fixing both.

I'm happy to adjust my patch unless you've started working on it already.

Here you go with the attached, to-be-applied on top of your own patch.
--
Michael

Attachments:

0001-Change-internal-queryid-type-to-int64.patchtext/x-diff; charset=us-asciiDownload

From fae170fa99506f160807e7810be3864f0933ff4e Mon Sep 17 00:00:00 2001
From: David Rowley <dgrowley@gmail.com>
Date: Tue, 20 May 2025 12:43:18 +1200
Subject: [PATCH 1/2] Change internal queryid type to int64

Previously, this was uint64, which perhaps was chosen in cff440d36 as the
previous type was uint32 prior to that widening.

Having this as uint64 does require us to have to cast the value in
various places since the value is output in its signed form in various
places, such as EXPLAIN VERBOSE and in the pg_stat_statements.queryid
column.

We should likely note down this change in the release notes "Source
Code" section as some extensions may wish to adjust their code.

Discussion: https://postgr.es/m/50cb0c8b-994b-48f9-a1c4-13039eb3536b@eisentraut.org
---
 src/include/nodes/parsenodes.h                |  2 +-
 src/include/nodes/plannodes.h                 |  2 +-
 src/include/utils/backend_status.h            |  6 ++--
 src/backend/access/brin/brin.c                |  2 +-
 src/backend/access/nbtree/nbtsort.c           |  2 +-
 src/backend/commands/explain.c                |  8 ++---
 src/backend/commands/vacuumparallel.c         |  2 +-
 src/backend/nodes/gen_node_support.pl         |  5 +++
 src/backend/nodes/outfuncs.c                  |  6 ++++
 src/backend/nodes/queryjumblefuncs.c          | 22 ++++++------
 src/backend/nodes/readfuncs.c                 |  6 ++++
 src/backend/rewrite/rewriteHandler.c          |  2 +-
 src/backend/tcop/postgres.c                   |  4 +--
 src/backend/utils/activity/backend_status.c   | 12 +++----
 src/backend/utils/adt/pgstatfuncs.c           |  4 +--
 .../pg_stat_statements/pg_stat_statements.c   | 36 +++++++++----------
 16 files changed, 67 insertions(+), 54 deletions(-)

diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 4610fc61293b..b976a40a75d6 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -128,7 +128,7 @@ typedef struct Query
 	 * might not be set; also not stored.  This is the result of the query
 	 * jumble, hence ignored.
 	 */
-	uint64		queryId pg_node_attr(equal_ignore, query_jumble_ignore, read_write_ignore, read_as(0));
+	int64		queryId pg_node_attr(equal_ignore, query_jumble_ignore, read_write_ignore, read_as(0));
 
 	/* do I set the command result tag? */
 	bool		canSetTag pg_node_attr(query_jumble_ignore);
diff --git a/src/include/nodes/plannodes.h b/src/include/nodes/plannodes.h
index 658d76225e47..56cfdca074de 100644
--- a/src/include/nodes/plannodes.h
+++ b/src/include/nodes/plannodes.h
@@ -53,7 +53,7 @@ typedef struct PlannedStmt
 	CmdType		commandType;
 
 	/* query identifier (copied from Query) */
-	uint64		queryId;
+	int64		queryId;
 
 	/* plan identifier (can be set by plugins) */
 	uint64		planId;
diff --git a/src/include/utils/backend_status.h b/src/include/utils/backend_status.h
index 430ccd7d78e4..bbebe517501f 100644
--- a/src/include/utils/backend_status.h
+++ b/src/include/utils/backend_status.h
@@ -170,7 +170,7 @@ typedef struct PgBackendStatus
 	int64		st_progress_param[PGSTAT_NUM_PROGRESS_PARAM];
 
 	/* query identifier, optionally computed using post_parse_analyze_hook */
-	uint64		st_query_id;
+	int64		st_query_id;
 
 	/* plan identifier, optionally computed using planner_hook */
 	uint64		st_plan_id;
@@ -321,7 +321,7 @@ extern void pgstat_clear_backend_activity_snapshot(void);
 
 /* Activity reporting functions */
 extern void pgstat_report_activity(BackendState state, const char *cmd_str);
-extern void pgstat_report_query_id(uint64 query_id, bool force);
+extern void pgstat_report_query_id(int64 query_id, bool force);
 extern void pgstat_report_plan_id(uint64 plan_id, bool force);
 extern void pgstat_report_tempfile(size_t filesize);
 extern void pgstat_report_appname(const char *appname);
@@ -329,7 +329,7 @@ extern void pgstat_report_xact_timestamp(TimestampTz tstamp);
 extern const char *pgstat_get_backend_current_activity(int pid, bool checkUser);
 extern const char *pgstat_get_crashed_backend_activity(int pid, char *buffer,
 													   int buflen);
-extern uint64 pgstat_get_my_query_id(void);
+extern int64 pgstat_get_my_query_id(void);
 extern uint64 pgstat_get_my_plan_id(void);
 extern BackendType pgstat_get_backend_type_by_proc_number(ProcNumber procNumber);
 
diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c
index 01e1db7f856b..4204088fa0d7 100644
--- a/src/backend/access/brin/brin.c
+++ b/src/backend/access/brin/brin.c
@@ -68,7 +68,7 @@ typedef struct BrinShared
 	int			scantuplesortstates;
 
 	/* Query ID, for report in worker processes */
-	uint64		queryid;
+	int64		queryid;
 
 	/*
 	 * workersdonecv is used to monitor the progress of workers.  All parallel
diff --git a/src/backend/access/nbtree/nbtsort.c b/src/backend/access/nbtree/nbtsort.c
index 3794cc924ad4..9d70e89c1f3c 100644
--- a/src/backend/access/nbtree/nbtsort.c
+++ b/src/backend/access/nbtree/nbtsort.c
@@ -105,7 +105,7 @@ typedef struct BTShared
 	int			scantuplesortstates;
 
 	/* Query ID, for report in worker processes */
-	uint64		queryid;
+	int64		queryid;
 
 	/*
 	 * workersdonecv is used to monitor the progress of workers.  All parallel
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index 786ee865f147..159a47496743 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -823,14 +823,10 @@ ExplainPrintPlan(ExplainState *es, QueryDesc *queryDesc)
 	 * the queryid in any of the EXPLAIN plans to keep stable the results
 	 * generated by regression test suites.
 	 */
-	if (es->verbose && queryDesc->plannedstmt->queryId != UINT64CONST(0) &&
+	if (es->verbose && queryDesc->plannedstmt->queryId != INT64CONST(0) &&
 		compute_query_id != COMPUTE_QUERY_ID_REGRESS)
 	{
-		/*
-		 * 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.
-		 */
-		ExplainPropertyInteger("Query Identifier", NULL, (int64)
+		ExplainPropertyInteger("Query Identifier", NULL,
 							   queryDesc->plannedstmt->queryId, es);
 	}
 }
diff --git a/src/backend/commands/vacuumparallel.c b/src/backend/commands/vacuumparallel.c
index 2b9d548cdeb1..0feea1d30ec3 100644
--- a/src/backend/commands/vacuumparallel.c
+++ b/src/backend/commands/vacuumparallel.c
@@ -63,7 +63,7 @@ typedef struct PVShared
 	 */
 	Oid			relid;
 	int			elevel;
-	uint64		queryid;
+	int64		queryid;
 
 	/*
 	 * Fields for both index vacuum and cleanup.
diff --git a/src/backend/nodes/gen_node_support.pl b/src/backend/nodes/gen_node_support.pl
index 77659b0f7602..c8595109b0e1 100644
--- a/src/backend/nodes/gen_node_support.pl
+++ b/src/backend/nodes/gen_node_support.pl
@@ -1039,6 +1039,11 @@ _read${n}(void)
 			print $off "\tWRITE_UINT_FIELD($f);\n";
 			print $rff "\tREAD_UINT_FIELD($f);\n" unless $no_read;
 		}
+		elsif ($t eq 'int64')
+		{
+			print $off "\tWRITE_INT64_FIELD($f);\n";
+			print $rff "\tREAD_INT64_FIELD($f);\n" unless $no_read;
+		}
 		elsif ($t eq 'uint64'
 			|| $t eq 'AclMode')
 		{
diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index ceac3fd86201..25e08ba3426b 100644
--- a/src/backend/nodes/outfuncs.c
+++ b/src/backend/nodes/outfuncs.c
@@ -51,6 +51,12 @@ static void outDouble(StringInfo str, double d);
 #define WRITE_UINT_FIELD(fldname) \
 	appendStringInfo(str, " :" CppAsString(fldname) " %u", node->fldname)
 
+/* Write a signed integer field (anything written with INT64_FORMAT) */
+#define WRITE_INT64_FIELD(fldname) \
+	appendStringInfo(str, \
+					 " :" CppAsString(fldname) " " INT64_FORMAT, \
+					 node->fldname)
+
 /* Write an unsigned integer field (anything written with UINT64_FORMAT) */
 #define WRITE_UINT64_FIELD(fldname) \
 	appendStringInfo(str, " :" CppAsString(fldname) " " UINT64_FORMAT, \
diff --git a/src/backend/nodes/queryjumblefuncs.c b/src/backend/nodes/queryjumblefuncs.c
index d1e82a63f09a..ac3cb3d9cafe 100644
--- a/src/backend/nodes/queryjumblefuncs.c
+++ b/src/backend/nodes/queryjumblefuncs.c
@@ -56,7 +56,7 @@ int			compute_query_id = COMPUTE_QUERY_ID_AUTO;
 bool		query_id_enabled = false;
 
 static JumbleState *InitJumble(void);
-static uint64 DoJumble(JumbleState *jstate, Node *node);
+static int64 DoJumble(JumbleState *jstate, Node *node);
 static void AppendJumble(JumbleState *jstate,
 						 const unsigned char *value, Size size);
 static void FlushPendingNulls(JumbleState *jstate);
@@ -141,12 +141,12 @@ JumbleQuery(Query *query)
 	 * If we are unlucky enough to get a hash of zero, use 1 instead for
 	 * normal statements and 2 for utility queries.
 	 */
-	if (query->queryId == UINT64CONST(0))
+	if (query->queryId == INT64CONST(0))
 	{
 		if (query->utilityStmt)
-			query->queryId = UINT64CONST(2);
+			query->queryId = INT64CONST(2);
 		else
-			query->queryId = UINT64CONST(1);
+			query->queryId = INT64CONST(1);
 	}
 
 	return jstate;
@@ -197,7 +197,7 @@ InitJumble(void)
  *		Jumble the given Node using the given JumbleState and return the resulting
  *		jumble hash.
  */
-static uint64
+static int64
 DoJumble(JumbleState *jstate, Node *node)
 {
 	/* Jumble the given node */
@@ -208,9 +208,9 @@ DoJumble(JumbleState *jstate, Node *node)
 		FlushPendingNulls(jstate);
 
 	/* Process the jumble buffer and produce the hash value */
-	return DatumGetUInt64(hash_any_extended(jstate->jumble,
-											jstate->jumble_len,
-											0));
+	return DatumGetInt64(hash_any_extended(jstate->jumble,
+										   jstate->jumble_len,
+										   0));
 }
 
 /*
@@ -256,10 +256,10 @@ AppendJumbleInternal(JumbleState *jstate, const unsigned char *item,
 
 		if (unlikely(jumble_len >= JUMBLE_SIZE))
 		{
-			uint64		start_hash;
+			int64		start_hash;
 
-			start_hash = DatumGetUInt64(hash_any_extended(jumble,
-														  JUMBLE_SIZE, 0));
+			start_hash = DatumGetInt64(hash_any_extended(jumble,
+														 JUMBLE_SIZE, 0));
 			memcpy(jumble, &start_hash, sizeof(start_hash));
 			jumble_len = sizeof(start_hash);
 		}
diff --git a/src/backend/nodes/readfuncs.c b/src/backend/nodes/readfuncs.c
index 64d3a09f765b..8c90ab54af81 100644
--- a/src/backend/nodes/readfuncs.c
+++ b/src/backend/nodes/readfuncs.c
@@ -68,6 +68,12 @@
 	token = pg_strtok(&length);		/* get field value */ \
 	local_node->fldname = atoui(token)
 
+/* Read a signed integer field (anything written using INT64_FORMAT) */
+#define READ_INT64_FIELD(fldname) \
+	token = pg_strtok(&length); /* skip :fldname */ \
+	token = pg_strtok(&length); /* get field value */ \
+	local_node->fldname = strtoi64(token, NULL, 10)
+
 /* Read an unsigned integer field (anything written using UINT64_FORMAT) */
 #define READ_UINT64_FIELD(fldname) \
 	token = pg_strtok(&length);		/* skip :fldname */ \
diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c
index f0bce5f9ed95..2ef0e7fbf3a6 100644
--- a/src/backend/rewrite/rewriteHandler.c
+++ b/src/backend/rewrite/rewriteHandler.c
@@ -4544,7 +4544,7 @@ build_generation_expression(Relation rel, int attrno)
 List *
 QueryRewrite(Query *parsetree)
 {
-	uint64		input_query_id = parsetree->queryId;
+	int64		input_query_id = parsetree->queryId;
 	List	   *querylist;
 	List	   *results;
 	ListCell   *l;
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 1ae51b1b3915..7caf740d396c 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -1683,7 +1683,7 @@ exec_bind_message(StringInfo input_message)
 	{
 		Query	   *query = lfirst_node(Query, lc);
 
-		if (query->queryId != UINT64CONST(0))
+		if (query->queryId != INT64CONST(0))
 		{
 			pgstat_report_query_id(query->queryId, false);
 			break;
@@ -2176,7 +2176,7 @@ exec_execute_message(const char *portal_name, long max_rows)
 	{
 		PlannedStmt *stmt = lfirst_node(PlannedStmt, lc);
 
-		if (stmt->queryId != UINT64CONST(0))
+		if (stmt->queryId != INT64CONST(0))
 		{
 			pgstat_report_query_id(stmt->queryId, false);
 			break;
diff --git a/src/backend/utils/activity/backend_status.c b/src/backend/utils/activity/backend_status.c
index e1576e64b6d4..9c2ed2cb9e0b 100644
--- a/src/backend/utils/activity/backend_status.c
+++ b/src/backend/utils/activity/backend_status.c
@@ -320,7 +320,7 @@ pgstat_bestart_initial(void)
 	lbeentry.st_state = STATE_STARTING;
 	lbeentry.st_progress_command = PROGRESS_COMMAND_INVALID;
 	lbeentry.st_progress_command_target = InvalidOid;
-	lbeentry.st_query_id = UINT64CONST(0);
+	lbeentry.st_query_id = INT64CONST(0);
 	lbeentry.st_plan_id = UINT64CONST(0);
 
 	/*
@@ -599,7 +599,7 @@ pgstat_report_activity(BackendState state, const char *cmd_str)
 			beentry->st_activity_start_timestamp = 0;
 			/* st_xact_start_timestamp and wait_event_info are also disabled */
 			beentry->st_xact_start_timestamp = 0;
-			beentry->st_query_id = UINT64CONST(0);
+			beentry->st_query_id = INT64CONST(0);
 			beentry->st_plan_id = UINT64CONST(0);
 			proc->wait_event_info = 0;
 			PGSTAT_END_WRITE_ACTIVITY(beentry);
@@ -662,7 +662,7 @@ pgstat_report_activity(BackendState state, const char *cmd_str)
 	 */
 	if (state == STATE_RUNNING)
 	{
-		beentry->st_query_id = UINT64CONST(0);
+		beentry->st_query_id = INT64CONST(0);
 		beentry->st_plan_id = UINT64CONST(0);
 	}
 
@@ -683,7 +683,7 @@ pgstat_report_activity(BackendState state, const char *cmd_str)
  * --------
  */
 void
-pgstat_report_query_id(uint64 query_id, bool force)
+pgstat_report_query_id(int64 query_id, bool force)
 {
 	volatile PgBackendStatus *beentry = MyBEEntry;
 
@@ -702,7 +702,7 @@ pgstat_report_query_id(uint64 query_id, bool force)
 	 * command, so ignore the one provided unless it's an explicit call to
 	 * reset the identifier.
 	 */
-	if (beentry->st_query_id != 0 && !force)
+	if (beentry->st_query_id != INT64CONST(0) && !force)
 		return;
 
 	/*
@@ -1134,7 +1134,7 @@ pgstat_get_crashed_backend_activity(int pid, char *buffer, int buflen)
  *
  * Return current backend's query identifier.
  */
-uint64
+int64
 pgstat_get_my_query_id(void)
 {
 	if (!MyBEEntry)
diff --git a/src/backend/utils/adt/pgstatfuncs.c b/src/backend/utils/adt/pgstatfuncs.c
index 97af7c6554ff..e980109f2452 100644
--- a/src/backend/utils/adt/pgstatfuncs.c
+++ b/src/backend/utils/adt/pgstatfuncs.c
@@ -640,10 +640,10 @@ pg_stat_get_activity(PG_FUNCTION_ARGS)
 				values[28] = BoolGetDatum(false);	/* GSS credentials not
 													 * delegated */
 			}
-			if (beentry->st_query_id == 0)
+			if (beentry->st_query_id == INT64CONST(0))
 				nulls[30] = true;
 			else
-				values[30] = UInt64GetDatum(beentry->st_query_id);
+				values[30] = Int64GetDatum(beentry->st_query_id);
 		}
 		else
 		{
diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index 9778407cba30..7d7bcc68dfac 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -144,7 +144,7 @@ typedef struct pgssHashKey
 {
 	Oid			userid;			/* user OID */
 	Oid			dbid;			/* database OID */
-	uint64		queryid;		/* query identifier */
+	int64		queryid;		/* query identifier */
 	bool		toplevel;		/* query executed at top level */
 } pgssHashKey;
 
@@ -346,7 +346,7 @@ static void pgss_ProcessUtility(PlannedStmt *pstmt, const char *queryString,
 								ProcessUtilityContext context, ParamListInfo params,
 								QueryEnvironment *queryEnv,
 								DestReceiver *dest, QueryCompletion *qc);
-static void pgss_store(const char *query, uint64 queryId,
+static void pgss_store(const char *query, int64 queryId,
 					   int query_location, int query_len,
 					   pgssStoreKind kind,
 					   double total_time, uint64 rows,
@@ -370,7 +370,7 @@ static char *qtext_fetch(Size query_offset, int query_len,
 						 char *buffer, Size buffer_size);
 static bool need_gc_qtexts(void);
 static void gc_qtexts(void);
-static TimestampTz entry_reset(Oid userid, Oid dbid, uint64 queryid, bool minmax_only);
+static TimestampTz entry_reset(Oid userid, Oid dbid, int64 queryid, bool minmax_only);
 static char *generate_normalized_query(JumbleState *jstate, const char *query,
 									   int query_loc, int *query_len_p);
 static void fill_in_constant_lengths(JumbleState *jstate, const char *query,
@@ -852,7 +852,7 @@ pgss_post_parse_analyze(ParseState *pstate, Query *query, JumbleState *jstate)
 	{
 		if (pgss_track_utility && IsA(query->utilityStmt, ExecuteStmt))
 		{
-			query->queryId = UINT64CONST(0);
+			query->queryId = INT64CONST(0);
 			return;
 		}
 	}
@@ -899,7 +899,7 @@ pgss_planner(Query *parse,
 	 */
 	if (pgss_enabled(nesting_level)
 		&& pgss_track_planning && query_string
-		&& parse->queryId != UINT64CONST(0))
+		&& parse->queryId != INT64CONST(0))
 	{
 		instr_time	start;
 		instr_time	duration;
@@ -1008,7 +1008,7 @@ pgss_ExecutorStart(QueryDesc *queryDesc, int eflags)
 	 * counting of optimizable statements that are directly contained in
 	 * utility statements.
 	 */
-	if (pgss_enabled(nesting_level) && queryDesc->plannedstmt->queryId != UINT64CONST(0))
+	if (pgss_enabled(nesting_level) && queryDesc->plannedstmt->queryId != INT64CONST(0))
 	{
 		/*
 		 * Set up to track total elapsed time in ExecutorRun.  Make sure the
@@ -1076,9 +1076,9 @@ pgss_ExecutorFinish(QueryDesc *queryDesc)
 static void
 pgss_ExecutorEnd(QueryDesc *queryDesc)
 {
-	uint64		queryId = queryDesc->plannedstmt->queryId;
+	int64		queryId = queryDesc->plannedstmt->queryId;
 
-	if (queryId != UINT64CONST(0) && queryDesc->totaltime &&
+	if (queryId != INT64CONST(0) && queryDesc->totaltime &&
 		pgss_enabled(nesting_level))
 	{
 		/*
@@ -1139,7 +1139,7 @@ pgss_ProcessUtility(PlannedStmt *pstmt, const char *queryString,
 	 * only.
 	 */
 	if (enabled)
-		pstmt->queryId = UINT64CONST(0);
+		pstmt->queryId = INT64CONST(0);
 
 	/*
 	 * If it's an EXECUTE statement, we don't track it and don't increment the
@@ -1286,7 +1286,7 @@ pgss_ProcessUtility(PlannedStmt *pstmt, const char *queryString,
  * for the arrays in the Counters field.
  */
 static void
-pgss_store(const char *query, uint64 queryId,
+pgss_store(const char *query, int64 queryId,
 		   int query_location, int query_len,
 		   pgssStoreKind kind,
 		   double total_time, uint64 rows,
@@ -1312,7 +1312,7 @@ pgss_store(const char *query, uint64 queryId,
 	 * Nothing to do if compute_query_id isn't enabled and no other module
 	 * computed a query identifier.
 	 */
-	if (queryId == UINT64CONST(0))
+	if (queryId == INT64CONST(0))
 		return;
 
 	/*
@@ -1522,11 +1522,11 @@ pg_stat_statements_reset_1_7(PG_FUNCTION_ARGS)
 {
 	Oid			userid;
 	Oid			dbid;
-	uint64		queryid;
+	int64		queryid;
 
 	userid = PG_GETARG_OID(0);
 	dbid = PG_GETARG_OID(1);
-	queryid = (uint64) PG_GETARG_INT64(2);
+	queryid = PG_GETARG_INT64(2);
 
 	entry_reset(userid, dbid, queryid, false);
 
@@ -1538,12 +1538,12 @@ pg_stat_statements_reset_1_11(PG_FUNCTION_ARGS)
 {
 	Oid			userid;
 	Oid			dbid;
-	uint64		queryid;
+	int64		queryid;
 	bool		minmax_only;
 
 	userid = PG_GETARG_OID(0);
 	dbid = PG_GETARG_OID(1);
-	queryid = (uint64) PG_GETARG_INT64(2);
+	queryid = PG_GETARG_INT64(2);
 	minmax_only = PG_GETARG_BOOL(3);
 
 	PG_RETURN_TIMESTAMPTZ(entry_reset(userid, dbid, queryid, minmax_only));
@@ -2679,7 +2679,7 @@ if (e) { \
  * Reset entries corresponding to parameters passed.
  */
 static TimestampTz
-entry_reset(Oid userid, Oid dbid, uint64 queryid, bool minmax_only)
+entry_reset(Oid userid, Oid dbid, int64 queryid, bool minmax_only)
 {
 	HASH_SEQ_STATUS hash_seq;
 	pgssEntry  *entry;
@@ -2699,7 +2699,7 @@ entry_reset(Oid userid, Oid dbid, uint64 queryid, bool minmax_only)
 
 	stats_reset = GetCurrentTimestamp();
 
-	if (userid != 0 && dbid != 0 && queryid != UINT64CONST(0))
+	if (userid != 0 && dbid != 0 && queryid != INT64CONST(0))
 	{
 		/* If all the parameters are available, use the fast path. */
 		memset(&key, 0, sizeof(pgssHashKey));
@@ -2722,7 +2722,7 @@ entry_reset(Oid userid, Oid dbid, uint64 queryid, bool minmax_only)
 
 		SINGLE_ENTRY_RESET(entry);
 	}
-	else if (userid != 0 || dbid != 0 || queryid != UINT64CONST(0))
+	else if (userid != 0 || dbid != 0 || queryid != INT64CONST(0))
 	{
 		/* Reset entries corresponding to valid parameters. */
 		hash_seq_init(&hash_seq, pgss_hash);
-- 
2.49.0

0002-Change-type-of-plan-IDs-from-uint64-to-int64.patchtext/x-diff; charset=us-asciiDownload

From 3d3a7cc46fb6358c8fc501a7f35608bb9579c701 Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@paquier.xyz>
Date: Tue, 20 May 2025 15:29:39 +0900
Subject: [PATCH 2/2] Change type of plan IDs from uint64 to int64

---
 src/include/nodes/plannodes.h               |  2 +-
 src/include/utils/backend_status.h          |  6 +++---
 src/backend/tcop/postgres.c                 |  4 ++--
 src/backend/utils/activity/backend_status.c | 10 +++++-----
 4 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/src/include/nodes/plannodes.h b/src/include/nodes/plannodes.h
index 56cfdca074de..fdd3deb4f6d5 100644
--- a/src/include/nodes/plannodes.h
+++ b/src/include/nodes/plannodes.h
@@ -56,7 +56,7 @@ typedef struct PlannedStmt
 	int64		queryId;
 
 	/* plan identifier (can be set by plugins) */
-	uint64		planId;
+	int64		planId;
 
 	/* is it insert|update|delete|merge RETURNING? */
 	bool		hasReturning;
diff --git a/src/include/utils/backend_status.h b/src/include/utils/backend_status.h
index bbebe517501f..3016501ac059 100644
--- a/src/include/utils/backend_status.h
+++ b/src/include/utils/backend_status.h
@@ -173,7 +173,7 @@ typedef struct PgBackendStatus
 	int64		st_query_id;
 
 	/* plan identifier, optionally computed using planner_hook */
-	uint64		st_plan_id;
+	int64		st_plan_id;
 } PgBackendStatus;
 
 
@@ -322,7 +322,7 @@ extern void pgstat_clear_backend_activity_snapshot(void);
 /* Activity reporting functions */
 extern void pgstat_report_activity(BackendState state, const char *cmd_str);
 extern void pgstat_report_query_id(int64 query_id, bool force);
-extern void pgstat_report_plan_id(uint64 plan_id, bool force);
+extern void pgstat_report_plan_id(int64 plan_id, bool force);
 extern void pgstat_report_tempfile(size_t filesize);
 extern void pgstat_report_appname(const char *appname);
 extern void pgstat_report_xact_timestamp(TimestampTz tstamp);
@@ -330,7 +330,7 @@ extern const char *pgstat_get_backend_current_activity(int pid, bool checkUser);
 extern const char *pgstat_get_crashed_backend_activity(int pid, char *buffer,
 													   int buflen);
 extern int64 pgstat_get_my_query_id(void);
-extern uint64 pgstat_get_my_plan_id(void);
+extern int64 pgstat_get_my_plan_id(void);
 extern BackendType pgstat_get_backend_type_by_proc_number(ProcNumber procNumber);
 
 
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 7caf740d396c..2ed6b82c7c8f 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -2036,7 +2036,7 @@ exec_bind_message(StringInfo input_message)
 	{
 		PlannedStmt *plan = lfirst_node(PlannedStmt, lc);
 
-		if (plan->planId != UINT64CONST(0))
+		if (plan->planId != INT64CONST(0))
 		{
 			pgstat_report_plan_id(plan->planId, false);
 			break;
@@ -2187,7 +2187,7 @@ exec_execute_message(const char *portal_name, long max_rows)
 	{
 		PlannedStmt *stmt = lfirst_node(PlannedStmt, lc);
 
-		if (stmt->planId != UINT64CONST(0))
+		if (stmt->planId != INT64CONST(0))
 		{
 			pgstat_report_plan_id(stmt->planId, false);
 			break;
diff --git a/src/backend/utils/activity/backend_status.c b/src/backend/utils/activity/backend_status.c
index 9c2ed2cb9e0b..a290cc4c9750 100644
--- a/src/backend/utils/activity/backend_status.c
+++ b/src/backend/utils/activity/backend_status.c
@@ -321,7 +321,7 @@ pgstat_bestart_initial(void)
 	lbeentry.st_progress_command = PROGRESS_COMMAND_INVALID;
 	lbeentry.st_progress_command_target = InvalidOid;
 	lbeentry.st_query_id = INT64CONST(0);
-	lbeentry.st_plan_id = UINT64CONST(0);
+	lbeentry.st_plan_id = INT64CONST(0);
 
 	/*
 	 * we don't zero st_progress_param here to save cycles; nobody should
@@ -600,7 +600,7 @@ pgstat_report_activity(BackendState state, const char *cmd_str)
 			/* st_xact_start_timestamp and wait_event_info are also disabled */
 			beentry->st_xact_start_timestamp = 0;
 			beentry->st_query_id = INT64CONST(0);
-			beentry->st_plan_id = UINT64CONST(0);
+			beentry->st_plan_id = INT64CONST(0);
 			proc->wait_event_info = 0;
 			PGSTAT_END_WRITE_ACTIVITY(beentry);
 		}
@@ -663,7 +663,7 @@ pgstat_report_activity(BackendState state, const char *cmd_str)
 	if (state == STATE_RUNNING)
 	{
 		beentry->st_query_id = INT64CONST(0);
-		beentry->st_plan_id = UINT64CONST(0);
+		beentry->st_plan_id = INT64CONST(0);
 	}
 
 	if (cmd_str != NULL)
@@ -722,7 +722,7 @@ pgstat_report_query_id(int64 query_id, bool force)
  * --------
  */
 void
-pgstat_report_plan_id(uint64 plan_id, bool force)
+pgstat_report_plan_id(int64 plan_id, bool force)
 {
 	volatile PgBackendStatus *beentry = MyBEEntry;
 
@@ -1154,7 +1154,7 @@ pgstat_get_my_query_id(void)
  *
  * Return current backend's plan identifier.
  */
-uint64
+int64
 pgstat_get_my_plan_id(void)
 {
 	if (!MyBEEntry)
-- 
2.49.0

#17

David Rowley

dgrowleyml@gmail.com

8 months ago

In reply to: Julien Rouhaud (#15)

Re: Add comment explaining why queryid is int64 in pg_stat_statements

On Tue, 20 May 2025 at 18:12, Julien Rouhaud <rjuju123@gmail.com> wrote:

I don't think it was discussed, but why not go the other way, keep everything
as uint64 and expose an uint64 datatype at the SQL level instead? That's
something I actually want pretty often so I would be happy to have that
natively, whether it's called bigoid, oid8 or anything else. That's probably
too late for pg18 though.

Certainly, a bit late, yes. It basically requires implementing
unsigned types on the SQL level. I believe there are a few sunken
ships along that coastline, and plenty of history in the archives if
you want to understand some of the difficulties.

David

#18

Sami Imseih

samimseih@gmail.com

8 months ago

In reply to: Michael Paquier (#16)

Re: Add comment explaining why queryid is int64 in pg_stat_statements

Supporting unsigned INTs will be valuable for more than just this
obviously, and if we do
ever have that capability, we would likely want to go that route. I
know I've been asked about
why queryIds could be negative more than a few times. Until then, I
think the patch being
suggested is reasonable and cleaner.

A few comments on the patches:

1/ this was missed in pg_stat_statements
./contrib/pg_stat_statements/pg_stat_statements.c: uint64
saved_queryId = pstmt->queryId;

2/ Should "DatumGetInt64(hash_any_extended(......" be turned into a
macro since it's used in
several places? Also, we can add a comment in the macro such as
"
Output the queryId as an int64 rather than a uint64, to match the
BIGINT column used to emit
queryId in system views
"

I think this comment is needed to address the confusion that is the
original subject of this thread. Otherwise,
the confusion will be moved from pg_stat_statements.c to queryjumblefuncs.c

DoJumble(JumbleState *jstate, Node *node)
{
/* Jumble the given node */
@@ -208,9 +208,9 @@ DoJumble(JumbleState *jstate, Node *node)
FlushPendingNulls(jstate);
/* Process the jumble buffer and produce the hash value */
- return DatumGetUInt64(hash_any_extended(jstate->jumble,
- jstate->jumble_len,
- 0));
+ return DatumGetInt64(hash_any_extended(jstate->jumble,
+ jstate->jumble_len,
+ 0));
}
/*
@@ -256,10 +256,10 @@ AppendJumbleInternal(JumbleState *jstate, const
unsigned char *item,
if (unlikely(jumble_len >= JUMBLE_SIZE))
{
- uint64 start_hash;
+ int64 start_hash;
- start_hash = DatumGetUInt64(hash_any_extended(jumble,
- JUMBLE_SIZE, 0));
+ start_hash = DatumGetInt64(hash_any_extended(jumble,
+ JUMBLE_SIZE, 0));
memcpy(jumble, &start_hash, sizeof(start_hash));
jumble_len = sizeof(start_hash);

--
Sami Imseih
Amazon Web Services (AWS)

#19

Michael Paquier

michael@paquier.xyz

8 months ago

In reply to: David Rowley (#17)

Re: Add comment explaining why queryid is int64 in pg_stat_statements

On Tue, May 20, 2025 at 11:28:17PM +1200, David Rowley wrote:

Certainly, a bit late, yes. It basically requires implementing
unsigned types on the SQL level. I believe there are a few sunken
ships along that coastline, and plenty of history in the archives if
you want to understand some of the difficulties.

Providing some more context and some more information than this reply,
the latest thread that I can find on the matter is this one, from
December 2024:
/messages/by-id/CAN3gO4sPBKbfWYK10i294u3kzsfDb4WX891FMbjLnKjMS08u7A@mail.gmail.com

It summarizes nicely the situation and it contains some more general
points.

This particular point from Tom about numeric promotion and casting
hierarchy resonates as a very good one:
/messages/by-id/3180774.1733588677@sss.pgh.pa.us
My own bet is that if you don't want to lose any existing
functionality, perhaps we should be just more aggressive with casting
requirements on input to begin with even if it means more work when
writing queries, even if it comes at a loss of usability that should
still be something..

FWIW, I've wanted an unsigned in-core type more than once. It would
be a lot of work, but we have a lot of things that exist in the core
code that map to unsigned 32b or 64b integer values. Just naming one,
WAL LSN difference calculations. XLogRecPtr is a 64b unsigned value,
not its representation at SQL level, meaning that we'd overflow after
reaching the threshold of the bit tracking the signedness. It's true
that a system would need to live long enough to reach such LSNs, but
we have also 32b integers plugged here and there. Another one is
block numbers, or OID which is an in-core type. Having an in-core
unsigned integer type would scale better that creating types mapping
to every single internal core concept.
--
Michael

#20

Michael Paquier

michael@paquier.xyz

8 months ago

In reply to: Sami Imseih (#18)

2 attachment(s)

Re: Add comment explaining why queryid is int64 in pg_stat_statements

On Tue, May 20, 2025 at 10:35:39AM -0500, Sami Imseih wrote:

1/ this was missed in pg_stat_statements
./contrib/pg_stat_statements/pg_stat_statements.c: uint64
saved_queryId = pstmt->queryId;

Right. Some greps based on "queryid" and "query_id" point that this
is the last reference to uint in the tree.

2/ Should "DatumGetInt64(hash_any_extended(......" be turned into a
macro since it's used in
several places? Also, we can add a comment in the macro such as
"
Output the queryId as an int64 rather than a uint64, to match the
BIGINT column used to emit
queryId in system views
"

We are talking about two places in queryjumblefuncs.c. Leaving the
code as in David's original proposal is fine IMO. Adding a comment
about the implied uint64 -> int64 conversion when calling
hash_any_extended() sounds like a good idea to document what we want
from the hash.

I've had a closer look at David's patch, and I did not spot other
places that required similar adjustments. I may have missed
something, of course.

David, how would you like to move on with this patch set? I can take
responsibility for both patches as the plan ID change can qualify as
an open item.
--
Michael

Attachments:

v2-0001-Change-internal-queryid-type-to-int64.patchtext/x-diff; charset=us-asciiDownload

From 64df69093c52a17287aabb8551e8ab2b62d1b16a Mon Sep 17 00:00:00 2001
From: David Rowley <dgrowley@gmail.com>
Date: Tue, 20 May 2025 12:43:18 +1200
Subject: [PATCH v2 1/2] Change internal queryid type to int64

Previously, this was uint64, which perhaps was chosen in cff440d36 as the
previous type was uint32 prior to that widening.

Having this as uint64 does require us to have to cast the value in
various places since the value is output in its signed form in various
places, such as EXPLAIN VERBOSE and in the pg_stat_statements.queryid
column.

We should likely note down this change in the release notes "Source
Code" section as some extensions may wish to adjust their code.

Discussion: https://postgr.es/m/50cb0c8b-994b-48f9-a1c4-13039eb3536b@eisentraut.org
---
 src/include/nodes/parsenodes.h                |  2 +-
 src/include/nodes/plannodes.h                 |  2 +-
 src/include/utils/backend_status.h            |  6 +--
 src/backend/access/brin/brin.c                |  2 +-
 src/backend/access/nbtree/nbtsort.c           |  2 +-
 src/backend/commands/explain.c                |  8 +---
 src/backend/commands/vacuumparallel.c         |  2 +-
 src/backend/nodes/gen_node_support.pl         |  5 +++
 src/backend/nodes/outfuncs.c                  |  6 +++
 src/backend/nodes/queryjumblefuncs.c          | 28 ++++++++------
 src/backend/nodes/readfuncs.c                 |  6 +++
 src/backend/rewrite/rewriteHandler.c          |  2 +-
 src/backend/tcop/postgres.c                   |  4 +-
 src/backend/utils/activity/backend_status.c   | 12 +++---
 src/backend/utils/adt/pgstatfuncs.c           |  4 +-
 .../pg_stat_statements/pg_stat_statements.c   | 38 +++++++++----------
 16 files changed, 73 insertions(+), 56 deletions(-)

diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 4610fc61293b..b976a40a75d6 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -128,7 +128,7 @@ typedef struct Query
 	 * might not be set; also not stored.  This is the result of the query
 	 * jumble, hence ignored.
 	 */
-	uint64		queryId pg_node_attr(equal_ignore, query_jumble_ignore, read_write_ignore, read_as(0));
+	int64		queryId pg_node_attr(equal_ignore, query_jumble_ignore, read_write_ignore, read_as(0));
 
 	/* do I set the command result tag? */
 	bool		canSetTag pg_node_attr(query_jumble_ignore);
diff --git a/src/include/nodes/plannodes.h b/src/include/nodes/plannodes.h
index 658d76225e47..56cfdca074de 100644
--- a/src/include/nodes/plannodes.h
+++ b/src/include/nodes/plannodes.h
@@ -53,7 +53,7 @@ typedef struct PlannedStmt
 	CmdType		commandType;
 
 	/* query identifier (copied from Query) */
-	uint64		queryId;
+	int64		queryId;
 
 	/* plan identifier (can be set by plugins) */
 	uint64		planId;
diff --git a/src/include/utils/backend_status.h b/src/include/utils/backend_status.h
index 430ccd7d78e4..bbebe517501f 100644
--- a/src/include/utils/backend_status.h
+++ b/src/include/utils/backend_status.h
@@ -170,7 +170,7 @@ typedef struct PgBackendStatus
 	int64		st_progress_param[PGSTAT_NUM_PROGRESS_PARAM];
 
 	/* query identifier, optionally computed using post_parse_analyze_hook */
-	uint64		st_query_id;
+	int64		st_query_id;
 
 	/* plan identifier, optionally computed using planner_hook */
 	uint64		st_plan_id;
@@ -321,7 +321,7 @@ extern void pgstat_clear_backend_activity_snapshot(void);
 
 /* Activity reporting functions */
 extern void pgstat_report_activity(BackendState state, const char *cmd_str);
-extern void pgstat_report_query_id(uint64 query_id, bool force);
+extern void pgstat_report_query_id(int64 query_id, bool force);
 extern void pgstat_report_plan_id(uint64 plan_id, bool force);
 extern void pgstat_report_tempfile(size_t filesize);
 extern void pgstat_report_appname(const char *appname);
@@ -329,7 +329,7 @@ extern void pgstat_report_xact_timestamp(TimestampTz tstamp);
 extern const char *pgstat_get_backend_current_activity(int pid, bool checkUser);
 extern const char *pgstat_get_crashed_backend_activity(int pid, char *buffer,
 													   int buflen);
-extern uint64 pgstat_get_my_query_id(void);
+extern int64 pgstat_get_my_query_id(void);
 extern uint64 pgstat_get_my_plan_id(void);
 extern BackendType pgstat_get_backend_type_by_proc_number(ProcNumber procNumber);
 
diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c
index 01e1db7f856b..4204088fa0d7 100644
--- a/src/backend/access/brin/brin.c
+++ b/src/backend/access/brin/brin.c
@@ -68,7 +68,7 @@ typedef struct BrinShared
 	int			scantuplesortstates;
 
 	/* Query ID, for report in worker processes */
-	uint64		queryid;
+	int64		queryid;
 
 	/*
 	 * workersdonecv is used to monitor the progress of workers.  All parallel
diff --git a/src/backend/access/nbtree/nbtsort.c b/src/backend/access/nbtree/nbtsort.c
index 3794cc924ad4..9d70e89c1f3c 100644
--- a/src/backend/access/nbtree/nbtsort.c
+++ b/src/backend/access/nbtree/nbtsort.c
@@ -105,7 +105,7 @@ typedef struct BTShared
 	int			scantuplesortstates;
 
 	/* Query ID, for report in worker processes */
-	uint64		queryid;
+	int64		queryid;
 
 	/*
 	 * workersdonecv is used to monitor the progress of workers.  All parallel
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index 786ee865f147..159a47496743 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -823,14 +823,10 @@ ExplainPrintPlan(ExplainState *es, QueryDesc *queryDesc)
 	 * the queryid in any of the EXPLAIN plans to keep stable the results
 	 * generated by regression test suites.
 	 */
-	if (es->verbose && queryDesc->plannedstmt->queryId != UINT64CONST(0) &&
+	if (es->verbose && queryDesc->plannedstmt->queryId != INT64CONST(0) &&
 		compute_query_id != COMPUTE_QUERY_ID_REGRESS)
 	{
-		/*
-		 * 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.
-		 */
-		ExplainPropertyInteger("Query Identifier", NULL, (int64)
+		ExplainPropertyInteger("Query Identifier", NULL,
 							   queryDesc->plannedstmt->queryId, es);
 	}
 }
diff --git a/src/backend/commands/vacuumparallel.c b/src/backend/commands/vacuumparallel.c
index 2b9d548cdeb1..0feea1d30ec3 100644
--- a/src/backend/commands/vacuumparallel.c
+++ b/src/backend/commands/vacuumparallel.c
@@ -63,7 +63,7 @@ typedef struct PVShared
 	 */
 	Oid			relid;
 	int			elevel;
-	uint64		queryid;
+	int64		queryid;
 
 	/*
 	 * Fields for both index vacuum and cleanup.
diff --git a/src/backend/nodes/gen_node_support.pl b/src/backend/nodes/gen_node_support.pl
index 77659b0f7602..c8595109b0e1 100644
--- a/src/backend/nodes/gen_node_support.pl
+++ b/src/backend/nodes/gen_node_support.pl
@@ -1039,6 +1039,11 @@ _read${n}(void)
 			print $off "\tWRITE_UINT_FIELD($f);\n";
 			print $rff "\tREAD_UINT_FIELD($f);\n" unless $no_read;
 		}
+		elsif ($t eq 'int64')
+		{
+			print $off "\tWRITE_INT64_FIELD($f);\n";
+			print $rff "\tREAD_INT64_FIELD($f);\n" unless $no_read;
+		}
 		elsif ($t eq 'uint64'
 			|| $t eq 'AclMode')
 		{
diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index ceac3fd86201..25e08ba3426b 100644
--- a/src/backend/nodes/outfuncs.c
+++ b/src/backend/nodes/outfuncs.c
@@ -51,6 +51,12 @@ static void outDouble(StringInfo str, double d);
 #define WRITE_UINT_FIELD(fldname) \
 	appendStringInfo(str, " :" CppAsString(fldname) " %u", node->fldname)
 
+/* Write a signed integer field (anything written with INT64_FORMAT) */
+#define WRITE_INT64_FIELD(fldname) \
+	appendStringInfo(str, \
+					 " :" CppAsString(fldname) " " INT64_FORMAT, \
+					 node->fldname)
+
 /* Write an unsigned integer field (anything written with UINT64_FORMAT) */
 #define WRITE_UINT64_FIELD(fldname) \
 	appendStringInfo(str, " :" CppAsString(fldname) " " UINT64_FORMAT, \
diff --git a/src/backend/nodes/queryjumblefuncs.c b/src/backend/nodes/queryjumblefuncs.c
index d1e82a63f09a..5d345dbedd27 100644
--- a/src/backend/nodes/queryjumblefuncs.c
+++ b/src/backend/nodes/queryjumblefuncs.c
@@ -56,7 +56,7 @@ int			compute_query_id = COMPUTE_QUERY_ID_AUTO;
 bool		query_id_enabled = false;
 
 static JumbleState *InitJumble(void);
-static uint64 DoJumble(JumbleState *jstate, Node *node);
+static int64 DoJumble(JumbleState *jstate, Node *node);
 static void AppendJumble(JumbleState *jstate,
 						 const unsigned char *value, Size size);
 static void FlushPendingNulls(JumbleState *jstate);
@@ -141,12 +141,12 @@ JumbleQuery(Query *query)
 	 * If we are unlucky enough to get a hash of zero, use 1 instead for
 	 * normal statements and 2 for utility queries.
 	 */
-	if (query->queryId == UINT64CONST(0))
+	if (query->queryId == INT64CONST(0))
 	{
 		if (query->utilityStmt)
-			query->queryId = UINT64CONST(2);
+			query->queryId = INT64CONST(2);
 		else
-			query->queryId = UINT64CONST(1);
+			query->queryId = INT64CONST(1);
 	}
 
 	return jstate;
@@ -197,7 +197,7 @@ InitJumble(void)
  *		Jumble the given Node using the given JumbleState and return the resulting
  *		jumble hash.
  */
-static uint64
+static int64
 DoJumble(JumbleState *jstate, Node *node)
 {
 	/* Jumble the given node */
@@ -207,10 +207,13 @@ DoJumble(JumbleState *jstate, Node *node)
 	if (jstate->pending_nulls > 0)
 		FlushPendingNulls(jstate);
 
-	/* Process the jumble buffer and produce the hash value */
-	return DatumGetUInt64(hash_any_extended(jstate->jumble,
-											jstate->jumble_len,
-											0));
+	/*
+	 * Process the jumble buffer and produce the hash value, converting to
+	 * int64 to match with the type of the query ID.
+	 */
+	return DatumGetInt64(hash_any_extended(jstate->jumble,
+										   jstate->jumble_len,
+										   0));
 }
 
 /*
@@ -256,10 +259,11 @@ AppendJumbleInternal(JumbleState *jstate, const unsigned char *item,
 
 		if (unlikely(jumble_len >= JUMBLE_SIZE))
 		{
-			uint64		start_hash;
+			int64		start_hash;
 
-			start_hash = DatumGetUInt64(hash_any_extended(jumble,
-														  JUMBLE_SIZE, 0));
+			/* Converted to int64, to match with the type of the query ID */
+			start_hash = DatumGetInt64(hash_any_extended(jumble,
+														 JUMBLE_SIZE, 0));
 			memcpy(jumble, &start_hash, sizeof(start_hash));
 			jumble_len = sizeof(start_hash);
 		}
diff --git a/src/backend/nodes/readfuncs.c b/src/backend/nodes/readfuncs.c
index 64d3a09f765b..8c90ab54af81 100644
--- a/src/backend/nodes/readfuncs.c
+++ b/src/backend/nodes/readfuncs.c
@@ -68,6 +68,12 @@
 	token = pg_strtok(&length);		/* get field value */ \
 	local_node->fldname = atoui(token)
 
+/* Read a signed integer field (anything written using INT64_FORMAT) */
+#define READ_INT64_FIELD(fldname) \
+	token = pg_strtok(&length); /* skip :fldname */ \
+	token = pg_strtok(&length); /* get field value */ \
+	local_node->fldname = strtoi64(token, NULL, 10)
+
 /* Read an unsigned integer field (anything written using UINT64_FORMAT) */
 #define READ_UINT64_FIELD(fldname) \
 	token = pg_strtok(&length);		/* skip :fldname */ \
diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c
index f0bce5f9ed95..2ef0e7fbf3a6 100644
--- a/src/backend/rewrite/rewriteHandler.c
+++ b/src/backend/rewrite/rewriteHandler.c
@@ -4544,7 +4544,7 @@ build_generation_expression(Relation rel, int attrno)
 List *
 QueryRewrite(Query *parsetree)
 {
-	uint64		input_query_id = parsetree->queryId;
+	int64		input_query_id = parsetree->queryId;
 	List	   *querylist;
 	List	   *results;
 	ListCell   *l;
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 1ae51b1b3915..7caf740d396c 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -1683,7 +1683,7 @@ exec_bind_message(StringInfo input_message)
 	{
 		Query	   *query = lfirst_node(Query, lc);
 
-		if (query->queryId != UINT64CONST(0))
+		if (query->queryId != INT64CONST(0))
 		{
 			pgstat_report_query_id(query->queryId, false);
 			break;
@@ -2176,7 +2176,7 @@ exec_execute_message(const char *portal_name, long max_rows)
 	{
 		PlannedStmt *stmt = lfirst_node(PlannedStmt, lc);
 
-		if (stmt->queryId != UINT64CONST(0))
+		if (stmt->queryId != INT64CONST(0))
 		{
 			pgstat_report_query_id(stmt->queryId, false);
 			break;
diff --git a/src/backend/utils/activity/backend_status.c b/src/backend/utils/activity/backend_status.c
index e1576e64b6d4..9c2ed2cb9e0b 100644
--- a/src/backend/utils/activity/backend_status.c
+++ b/src/backend/utils/activity/backend_status.c
@@ -320,7 +320,7 @@ pgstat_bestart_initial(void)
 	lbeentry.st_state = STATE_STARTING;
 	lbeentry.st_progress_command = PROGRESS_COMMAND_INVALID;
 	lbeentry.st_progress_command_target = InvalidOid;
-	lbeentry.st_query_id = UINT64CONST(0);
+	lbeentry.st_query_id = INT64CONST(0);
 	lbeentry.st_plan_id = UINT64CONST(0);
 
 	/*
@@ -599,7 +599,7 @@ pgstat_report_activity(BackendState state, const char *cmd_str)
 			beentry->st_activity_start_timestamp = 0;
 			/* st_xact_start_timestamp and wait_event_info are also disabled */
 			beentry->st_xact_start_timestamp = 0;
-			beentry->st_query_id = UINT64CONST(0);
+			beentry->st_query_id = INT64CONST(0);
 			beentry->st_plan_id = UINT64CONST(0);
 			proc->wait_event_info = 0;
 			PGSTAT_END_WRITE_ACTIVITY(beentry);
@@ -662,7 +662,7 @@ pgstat_report_activity(BackendState state, const char *cmd_str)
 	 */
 	if (state == STATE_RUNNING)
 	{
-		beentry->st_query_id = UINT64CONST(0);
+		beentry->st_query_id = INT64CONST(0);
 		beentry->st_plan_id = UINT64CONST(0);
 	}
 
@@ -683,7 +683,7 @@ pgstat_report_activity(BackendState state, const char *cmd_str)
  * --------
  */
 void
-pgstat_report_query_id(uint64 query_id, bool force)
+pgstat_report_query_id(int64 query_id, bool force)
 {
 	volatile PgBackendStatus *beentry = MyBEEntry;
 
@@ -702,7 +702,7 @@ pgstat_report_query_id(uint64 query_id, bool force)
 	 * command, so ignore the one provided unless it's an explicit call to
 	 * reset the identifier.
 	 */
-	if (beentry->st_query_id != 0 && !force)
+	if (beentry->st_query_id != INT64CONST(0) && !force)
 		return;
 
 	/*
@@ -1134,7 +1134,7 @@ pgstat_get_crashed_backend_activity(int pid, char *buffer, int buflen)
  *
  * Return current backend's query identifier.
  */
-uint64
+int64
 pgstat_get_my_query_id(void)
 {
 	if (!MyBEEntry)
diff --git a/src/backend/utils/adt/pgstatfuncs.c b/src/backend/utils/adt/pgstatfuncs.c
index 97af7c6554ff..e980109f2452 100644
--- a/src/backend/utils/adt/pgstatfuncs.c
+++ b/src/backend/utils/adt/pgstatfuncs.c
@@ -640,10 +640,10 @@ pg_stat_get_activity(PG_FUNCTION_ARGS)
 				values[28] = BoolGetDatum(false);	/* GSS credentials not
 													 * delegated */
 			}
-			if (beentry->st_query_id == 0)
+			if (beentry->st_query_id == INT64CONST(0))
 				nulls[30] = true;
 			else
-				values[30] = UInt64GetDatum(beentry->st_query_id);
+				values[30] = Int64GetDatum(beentry->st_query_id);
 		}
 		else
 		{
diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index 9778407cba30..6d766ceaa508 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -144,7 +144,7 @@ typedef struct pgssHashKey
 {
 	Oid			userid;			/* user OID */
 	Oid			dbid;			/* database OID */
-	uint64		queryid;		/* query identifier */
+	int64		queryid;		/* query identifier */
 	bool		toplevel;		/* query executed at top level */
 } pgssHashKey;
 
@@ -346,7 +346,7 @@ static void pgss_ProcessUtility(PlannedStmt *pstmt, const char *queryString,
 								ProcessUtilityContext context, ParamListInfo params,
 								QueryEnvironment *queryEnv,
 								DestReceiver *dest, QueryCompletion *qc);
-static void pgss_store(const char *query, uint64 queryId,
+static void pgss_store(const char *query, int64 queryId,
 					   int query_location, int query_len,
 					   pgssStoreKind kind,
 					   double total_time, uint64 rows,
@@ -370,7 +370,7 @@ static char *qtext_fetch(Size query_offset, int query_len,
 						 char *buffer, Size buffer_size);
 static bool need_gc_qtexts(void);
 static void gc_qtexts(void);
-static TimestampTz entry_reset(Oid userid, Oid dbid, uint64 queryid, bool minmax_only);
+static TimestampTz entry_reset(Oid userid, Oid dbid, int64 queryid, bool minmax_only);
 static char *generate_normalized_query(JumbleState *jstate, const char *query,
 									   int query_loc, int *query_len_p);
 static void fill_in_constant_lengths(JumbleState *jstate, const char *query,
@@ -852,7 +852,7 @@ pgss_post_parse_analyze(ParseState *pstate, Query *query, JumbleState *jstate)
 	{
 		if (pgss_track_utility && IsA(query->utilityStmt, ExecuteStmt))
 		{
-			query->queryId = UINT64CONST(0);
+			query->queryId = INT64CONST(0);
 			return;
 		}
 	}
@@ -899,7 +899,7 @@ pgss_planner(Query *parse,
 	 */
 	if (pgss_enabled(nesting_level)
 		&& pgss_track_planning && query_string
-		&& parse->queryId != UINT64CONST(0))
+		&& parse->queryId != INT64CONST(0))
 	{
 		instr_time	start;
 		instr_time	duration;
@@ -1008,7 +1008,7 @@ pgss_ExecutorStart(QueryDesc *queryDesc, int eflags)
 	 * counting of optimizable statements that are directly contained in
 	 * utility statements.
 	 */
-	if (pgss_enabled(nesting_level) && queryDesc->plannedstmt->queryId != UINT64CONST(0))
+	if (pgss_enabled(nesting_level) && queryDesc->plannedstmt->queryId != INT64CONST(0))
 	{
 		/*
 		 * Set up to track total elapsed time in ExecutorRun.  Make sure the
@@ -1076,9 +1076,9 @@ pgss_ExecutorFinish(QueryDesc *queryDesc)
 static void
 pgss_ExecutorEnd(QueryDesc *queryDesc)
 {
-	uint64		queryId = queryDesc->plannedstmt->queryId;
+	int64		queryId = queryDesc->plannedstmt->queryId;
 
-	if (queryId != UINT64CONST(0) && queryDesc->totaltime &&
+	if (queryId != INT64CONST(0) && queryDesc->totaltime &&
 		pgss_enabled(nesting_level))
 	{
 		/*
@@ -1119,7 +1119,7 @@ pgss_ProcessUtility(PlannedStmt *pstmt, const char *queryString,
 					DestReceiver *dest, QueryCompletion *qc)
 {
 	Node	   *parsetree = pstmt->utilityStmt;
-	uint64		saved_queryId = pstmt->queryId;
+	int64		saved_queryId = pstmt->queryId;
 	int			saved_stmt_location = pstmt->stmt_location;
 	int			saved_stmt_len = pstmt->stmt_len;
 	bool		enabled = pgss_track_utility && pgss_enabled(nesting_level);
@@ -1139,7 +1139,7 @@ pgss_ProcessUtility(PlannedStmt *pstmt, const char *queryString,
 	 * only.
 	 */
 	if (enabled)
-		pstmt->queryId = UINT64CONST(0);
+		pstmt->queryId = INT64CONST(0);
 
 	/*
 	 * If it's an EXECUTE statement, we don't track it and don't increment the
@@ -1286,7 +1286,7 @@ pgss_ProcessUtility(PlannedStmt *pstmt, const char *queryString,
  * for the arrays in the Counters field.
  */
 static void
-pgss_store(const char *query, uint64 queryId,
+pgss_store(const char *query, int64 queryId,
 		   int query_location, int query_len,
 		   pgssStoreKind kind,
 		   double total_time, uint64 rows,
@@ -1312,7 +1312,7 @@ pgss_store(const char *query, uint64 queryId,
 	 * Nothing to do if compute_query_id isn't enabled and no other module
 	 * computed a query identifier.
 	 */
-	if (queryId == UINT64CONST(0))
+	if (queryId == INT64CONST(0))
 		return;
 
 	/*
@@ -1522,11 +1522,11 @@ pg_stat_statements_reset_1_7(PG_FUNCTION_ARGS)
 {
 	Oid			userid;
 	Oid			dbid;
-	uint64		queryid;
+	int64		queryid;
 
 	userid = PG_GETARG_OID(0);
 	dbid = PG_GETARG_OID(1);
-	queryid = (uint64) PG_GETARG_INT64(2);
+	queryid = PG_GETARG_INT64(2);
 
 	entry_reset(userid, dbid, queryid, false);
 
@@ -1538,12 +1538,12 @@ pg_stat_statements_reset_1_11(PG_FUNCTION_ARGS)
 {
 	Oid			userid;
 	Oid			dbid;
-	uint64		queryid;
+	int64		queryid;
 	bool		minmax_only;
 
 	userid = PG_GETARG_OID(0);
 	dbid = PG_GETARG_OID(1);
-	queryid = (uint64) PG_GETARG_INT64(2);
+	queryid = PG_GETARG_INT64(2);
 	minmax_only = PG_GETARG_BOOL(3);
 
 	PG_RETURN_TIMESTAMPTZ(entry_reset(userid, dbid, queryid, minmax_only));
@@ -2679,7 +2679,7 @@ if (e) { \
  * Reset entries corresponding to parameters passed.
  */
 static TimestampTz
-entry_reset(Oid userid, Oid dbid, uint64 queryid, bool minmax_only)
+entry_reset(Oid userid, Oid dbid, int64 queryid, bool minmax_only)
 {
 	HASH_SEQ_STATUS hash_seq;
 	pgssEntry  *entry;
@@ -2699,7 +2699,7 @@ entry_reset(Oid userid, Oid dbid, uint64 queryid, bool minmax_only)
 
 	stats_reset = GetCurrentTimestamp();
 
-	if (userid != 0 && dbid != 0 && queryid != UINT64CONST(0))
+	if (userid != 0 && dbid != 0 && queryid != INT64CONST(0))
 	{
 		/* If all the parameters are available, use the fast path. */
 		memset(&key, 0, sizeof(pgssHashKey));
@@ -2722,7 +2722,7 @@ entry_reset(Oid userid, Oid dbid, uint64 queryid, bool minmax_only)
 
 		SINGLE_ENTRY_RESET(entry);
 	}
-	else if (userid != 0 || dbid != 0 || queryid != UINT64CONST(0))
+	else if (userid != 0 || dbid != 0 || queryid != INT64CONST(0))
 	{
 		/* Reset entries corresponding to valid parameters. */
 		hash_seq_init(&hash_seq, pgss_hash);
-- 
2.49.0

v2-0002-Change-type-of-plan-IDs-from-uint64-to-int64.patchtext/x-diff; charset=us-asciiDownload

From 5b78e34c5bccc5c8f33406f37e1f7393759570d4 Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@paquier.xyz>
Date: Tue, 20 May 2025 15:29:39 +0900
Subject: [PATCH v2 2/2] Change type of plan IDs from uint64 to int64

---
 src/include/nodes/plannodes.h               |  2 +-
 src/include/utils/backend_status.h          |  6 +++---
 src/backend/tcop/postgres.c                 |  4 ++--
 src/backend/utils/activity/backend_status.c | 10 +++++-----
 4 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/src/include/nodes/plannodes.h b/src/include/nodes/plannodes.h
index 56cfdca074de..fdd3deb4f6d5 100644
--- a/src/include/nodes/plannodes.h
+++ b/src/include/nodes/plannodes.h
@@ -56,7 +56,7 @@ typedef struct PlannedStmt
 	int64		queryId;
 
 	/* plan identifier (can be set by plugins) */
-	uint64		planId;
+	int64		planId;
 
 	/* is it insert|update|delete|merge RETURNING? */
 	bool		hasReturning;
diff --git a/src/include/utils/backend_status.h b/src/include/utils/backend_status.h
index bbebe517501f..3016501ac059 100644
--- a/src/include/utils/backend_status.h
+++ b/src/include/utils/backend_status.h
@@ -173,7 +173,7 @@ typedef struct PgBackendStatus
 	int64		st_query_id;
 
 	/* plan identifier, optionally computed using planner_hook */
-	uint64		st_plan_id;
+	int64		st_plan_id;
 } PgBackendStatus;
 
 
@@ -322,7 +322,7 @@ extern void pgstat_clear_backend_activity_snapshot(void);
 /* Activity reporting functions */
 extern void pgstat_report_activity(BackendState state, const char *cmd_str);
 extern void pgstat_report_query_id(int64 query_id, bool force);
-extern void pgstat_report_plan_id(uint64 plan_id, bool force);
+extern void pgstat_report_plan_id(int64 plan_id, bool force);
 extern void pgstat_report_tempfile(size_t filesize);
 extern void pgstat_report_appname(const char *appname);
 extern void pgstat_report_xact_timestamp(TimestampTz tstamp);
@@ -330,7 +330,7 @@ extern const char *pgstat_get_backend_current_activity(int pid, bool checkUser);
 extern const char *pgstat_get_crashed_backend_activity(int pid, char *buffer,
 													   int buflen);
 extern int64 pgstat_get_my_query_id(void);
-extern uint64 pgstat_get_my_plan_id(void);
+extern int64 pgstat_get_my_plan_id(void);
 extern BackendType pgstat_get_backend_type_by_proc_number(ProcNumber procNumber);
 
 
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 7caf740d396c..2ed6b82c7c8f 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -2036,7 +2036,7 @@ exec_bind_message(StringInfo input_message)
 	{
 		PlannedStmt *plan = lfirst_node(PlannedStmt, lc);
 
-		if (plan->planId != UINT64CONST(0))
+		if (plan->planId != INT64CONST(0))
 		{
 			pgstat_report_plan_id(plan->planId, false);
 			break;
@@ -2187,7 +2187,7 @@ exec_execute_message(const char *portal_name, long max_rows)
 	{
 		PlannedStmt *stmt = lfirst_node(PlannedStmt, lc);
 
-		if (stmt->planId != UINT64CONST(0))
+		if (stmt->planId != INT64CONST(0))
 		{
 			pgstat_report_plan_id(stmt->planId, false);
 			break;
diff --git a/src/backend/utils/activity/backend_status.c b/src/backend/utils/activity/backend_status.c
index 9c2ed2cb9e0b..a290cc4c9750 100644
--- a/src/backend/utils/activity/backend_status.c
+++ b/src/backend/utils/activity/backend_status.c
@@ -321,7 +321,7 @@ pgstat_bestart_initial(void)
 	lbeentry.st_progress_command = PROGRESS_COMMAND_INVALID;
 	lbeentry.st_progress_command_target = InvalidOid;
 	lbeentry.st_query_id = INT64CONST(0);
-	lbeentry.st_plan_id = UINT64CONST(0);
+	lbeentry.st_plan_id = INT64CONST(0);
 
 	/*
 	 * we don't zero st_progress_param here to save cycles; nobody should
@@ -600,7 +600,7 @@ pgstat_report_activity(BackendState state, const char *cmd_str)
 			/* st_xact_start_timestamp and wait_event_info are also disabled */
 			beentry->st_xact_start_timestamp = 0;
 			beentry->st_query_id = INT64CONST(0);
-			beentry->st_plan_id = UINT64CONST(0);
+			beentry->st_plan_id = INT64CONST(0);
 			proc->wait_event_info = 0;
 			PGSTAT_END_WRITE_ACTIVITY(beentry);
 		}
@@ -663,7 +663,7 @@ pgstat_report_activity(BackendState state, const char *cmd_str)
 	if (state == STATE_RUNNING)
 	{
 		beentry->st_query_id = INT64CONST(0);
-		beentry->st_plan_id = UINT64CONST(0);
+		beentry->st_plan_id = INT64CONST(0);
 	}
 
 	if (cmd_str != NULL)
@@ -722,7 +722,7 @@ pgstat_report_query_id(int64 query_id, bool force)
  * --------
  */
 void
-pgstat_report_plan_id(uint64 plan_id, bool force)
+pgstat_report_plan_id(int64 plan_id, bool force)
 {
 	volatile PgBackendStatus *beentry = MyBEEntry;
 
@@ -1154,7 +1154,7 @@ pgstat_get_my_query_id(void)
  *
  * Return current backend's plan identifier.
  */
-uint64
+int64
 pgstat_get_my_plan_id(void)
 {
 	if (!MyBEEntry)
-- 
2.49.0

#21

Julien Rouhaud

rjuju123@gmail.com

8 months ago

In reply to: Michael Paquier (#19)

Re: Add comment explaining why queryid is int64 in pg_stat_statements

On Wed, May 21, 2025 at 09:34:02AM +0900, Michael Paquier wrote:

On Tue, May 20, 2025 at 11:28:17PM +1200, David Rowley wrote:

Certainly, a bit late, yes. It basically requires implementing
unsigned types on the SQL level. I believe there are a few sunken
ships along that coastline, and plenty of history in the archives if
you want to understand some of the difficulties.

Providing some more context and some more information than this reply,
the latest thread that I can find on the matter is this one, from
December 2024:
/messages/by-id/CAN3gO4sPBKbfWYK10i294u3kzsfDb4WX891FMbjLnKjMS08u7A@mail.gmail.com

It summarizes nicely the situation and it contains some more general
points.

This particular point from Tom about numeric promotion and casting
hierarchy resonates as a very good one:
/messages/by-id/3180774.1733588677@sss.pgh.pa.us
My own bet is that if you don't want to lose any existing
functionality, perhaps we should be just more aggressive with casting
requirements on input to begin with even if it means more work when
writing queries, even if it comes at a loss of usability that should
still be something..

Thanks a lot Michael! I actually read that thread at that time but forgot
about it until you sent this link.

FWIW, I've wanted an unsigned in-core type more than once. It would
be a lot of work, but we have a lot of things that exist in the core
code that map to unsigned 32b or 64b integer values. Just naming one,
WAL LSN difference calculations. XLogRecPtr is a 64b unsigned value,
not its representation at SQL level, meaning that we'd overflow after
reaching the threshold of the bit tracking the signedness. It's true
that a system would need to live long enough to reach such LSNs, but
we have also 32b integers plugged here and there. Another one is
block numbers, or OID which is an in-core type. Having an in-core
unsigned integer type would scale better that creating types mapping
to every single internal core concept.

Agreed.

#22

Peter Eisentraut

peter@eisentraut.org

8 months ago

In reply to: Michael Paquier (#16)

Re: Add comment explaining why queryid is int64 in pg_stat_statements

On 20.05.25 08:38, Michael Paquier wrote:

On Tue, May 20, 2025 at 05:51:51PM +1200, David Rowley wrote:

Given the planId stuff is new and has the same issue, I think that
pushes me towards thinking now is better than later for fixing both.

I'm happy to adjust my patch unless you've started working on it already.

Here you go with the attached, to-be-applied on top of your own patch.

Whichever way we're going, surely this whole thing could benefit from a

typedef something QueryId;

#23

David Rowley

dgrowleyml@gmail.com

8 months ago

In reply to: Peter Eisentraut (#22)

Re: Add comment explaining why queryid is int64 in pg_stat_statements

On Thu, 22 May 2025 at 02:58, Peter Eisentraut <peter@eisentraut.org> wrote:

On 20.05.25 08:38, Michael Paquier wrote:

On Tue, May 20, 2025 at 05:51:51PM +1200, David Rowley wrote:

Given the planId stuff is new and has the same issue, I think that
pushes me towards thinking now is better than later for fixing both.

I'm happy to adjust my patch unless you've started working on it already.

Here you go with the attached, to-be-applied on top of your own patch.

Whichever way we're going, surely this whole thing could benefit from a

typedef something QueryId;

This part I'm not so sure about. It's not as if it can be changed to
some other type with a simple definition of the typedef. Looking at
the patch I proposed there are quite a few things that would still
need to be updated after the typedef is changed. PG_GETARG_INT64(),
INT64CONST() and DatumGetInt64() are part of that. There's also the
return type of hash_any_extended(). Changing the typedef definition
might also need an adjustment in gen_node_support.pl, depending on
what you're changing it to.

You could argue that if it reduces the locations that need to be
changed by using a typedef, then it's a win. But there are also
negative aspects to typedefs that need to be considered. For me, those
are the added level of indirection of code reading to actually who
what type I'm working with. I personally dislike typedefs like
"typedef PageHeaderData *PageHeader;" as they hide the fact I'm
working with a pointer.

I'm not outright objecting to the typedef for this. It's just I don't
see it as a clear-cut improvement for this case.

David

#24

Michael Paquier

michael@paquier.xyz

8 months ago

In reply to: David Rowley (#23)

Re: Add comment explaining why queryid is int64 in pg_stat_statements

On Thu, May 22, 2025 at 02:36:38PM +1200, David Rowley wrote:

You could argue that if it reduces the locations that need to be
changed by using a typedef, then it's a win. But there are also
negative aspects to typedefs that need to be considered. For me, those
are the added level of indirection of code reading to actually who
what type I'm working with. I personally dislike typedefs like
"typedef PageHeaderData *PageHeader;" as they hide the fact I'm
working with a pointer.

I'm not outright objecting to the typedef for this. It's just I don't
see it as a clear-cut improvement for this case.

Same opinion here. I am not quite clear what there is to gain in
hiding the query ID behind a typedef, or even apply that to the plan
ID.

I have added an open item about the plan ID part as it applies to v18,
adding the RMT in CC to get an opinion. If we cannot get a consensus
on all that, letting things as they are is still logically correct,
even with the -Wwarnings-format-signedness argument which is not
included by default currently.

Has somebody an opinion to offer?
--
Michael

#25

Michael Paquier

michael@paquier.xyz

8 months ago

In reply to: Michael Paquier (#24)

Re: Add comment explaining why queryid is int64 in pg_stat_statements

On Thu, May 22, 2025 at 01:01:14PM +0900, Michael Paquier wrote:

I have added an open item about the plan ID part as it applies to v18,
adding the RMT in CC to get an opinion. If we cannot get a consensus
on all that, letting things as they are is still logically correct,
even with the -Wwarnings-format-signedness argument which is not
included by default currently.

Has somebody an opinion to offer?

It has been one week since this last update, and there has been
nothing except the sound of cicadas. IMO, I think that we should just
pull the switch and make both of these IDs signed on HEAD, taking case
of the potential signedness warning issues.

Now, I don't really want to take a leap of faith without the RMT being
OK with that now that we are in beta1.
--
Michael

#26

Nathan Bossart

nathandbossart@gmail.com

8 months ago

In reply to: Michael Paquier (#25)

Re: Add comment explaining why queryid is int64 in pg_stat_statements

On Thu, May 29, 2025 at 01:53:07PM +0900, Michael Paquier wrote:

On Thu, May 22, 2025 at 01:01:14PM +0900, Michael Paquier wrote:

I have added an open item about the plan ID part as it applies to v18,
adding the RMT in CC to get an opinion. If we cannot get a consensus
on all that, letting things as they are is still logically correct,
even with the -Wwarnings-format-signedness argument which is not
included by default currently.

Has somebody an opinion to offer?

It has been one week since this last update, and there has been
nothing except the sound of cicadas. IMO, I think that we should just
pull the switch and make both of these IDs signed on HEAD, taking case
of the potential signedness warning issues.

Now, I don't really want to take a leap of faith without the RMT being
OK with that now that we are in beta1.

After reading through this thread and the latest patch set, I don't see any
strong reason for the RMT to object to this change for v18. IIUC some
extensions may need to adapt, but we're still a few months from 18.0, so
that seems okay. I vaguely recall that we've made other small
extension-breaking changes during the beta period for previous major
releases.

--
nathan

#27

Michael Paquier

michael@paquier.xyz

8 months ago

In reply to: Nathan Bossart (#26)

Re: Add comment explaining why queryid is int64 in pg_stat_statements

On Thu, May 29, 2025 at 09:28:35AM -0500, Nathan Bossart wrote:

On Thu, May 29, 2025 at 01:53:07PM +0900, Michael Paquier wrote:

Now, I don't really want to take a leap of faith without the RMT being
OK with that now that we are in beta1.

After reading through this thread and the latest patch set, I don't see any
strong reason for the RMT to object to this change for v18. IIUC some
extensions may need to adapt, but we're still a few months from 18.0, so
that seems okay. I vaguely recall that we've made other small
extension-breaking changes during the beta period for previous major
releases.

Thanks, Nathan. Let's proceed with the change then. David, would you
prefer handling the patch you have written by yourself for the query
ID part?
--
Michael

#28

David Rowley

dgrowleyml@gmail.com

8 months ago

In reply to: Michael Paquier (#27)

Re: Add comment explaining why queryid is int64 in pg_stat_statements

On Fri, 30 May 2025 at 11:35, Michael Paquier <michael@paquier.xyz> wrote:

Thanks, Nathan. Let's proceed with the change then. David, would you
prefer handling the patch you have written by yourself for the query
ID part?

Yes. I'll look at that again shortly.

David

#29

David Rowley

dgrowleyml@gmail.com

8 months ago

In reply to: Sami Imseih (#18)

Re: Add comment explaining why queryid is int64 in pg_stat_statements

Thanks for the review.

On Wed, 21 May 2025 at 03:35, Sami Imseih <samimseih@gmail.com> wrote:

2/ Should "DatumGetInt64(hash_any_extended(......" be turned into a
macro since it's used in
several places? Also, we can add a comment in the macro such as
"
Output the queryId as an int64 rather than a uint64, to match the
BIGINT column used to emit
queryId in system views
"

I think this comment is needed to address the confusion that is the
original subject of this thread. Otherwise,
the confusion will be moved from pg_stat_statements.c to queryjumblefuncs.c

I ended up adding a comment in the Query struct detailing why queryId
is signed. I imagine, as time passes, we might forget why we did that
as hashed values are more naturally unsigned. I think it's wrong to
add comments in DoJumble() to mention details about what the calling
function does. I think the fact that DoJumble() now returns int64
should be a good enough explanation as to why we want to get the hash
value in signed form.

David

#30

David Rowley

dgrowleyml@gmail.com

8 months ago

In reply to: David Rowley (#28)

Re: Add comment explaining why queryid is int64 in pg_stat_statements

On Fri, 30 May 2025 at 11:51, David Rowley <dgrowleyml@gmail.com> wrote:

On Fri, 30 May 2025 at 11:35, Michael Paquier <michael@paquier.xyz> wrote:

Thanks, Nathan. Let's proceed with the change then. David, would you
prefer handling the patch you have written by yourself for the query
ID part?

Yes. I'll look at that again shortly.

Now done.

David

#31

Shaik Mohammad Mujeeb

mujeeb.sk@zohocorp.com

8 months ago

In reply to: David Rowley (#30)

Re: Add comment explaining why queryid is int64 in pg_stat_statements

Thanks for making the appropriate changes, David.

Now everything make sense. :)

---- On Fri, 30 May 2025 16:39:23 +0530 David Rowley <dgrowleyml@gmail.com> wrote ---

On Fri, 30 May 2025 at 11:51, David Rowley < mailto:dgrowleyml@gmail.com > wrote:

On Fri, 30 May 2025 at 11:35, Michael Paquier < mailto:michael@paquier.xyz > wrote:

Thanks, Nathan. Let's proceed with the change then. David, would you
prefer handling the patch you have written by yourself for the query
ID part?

Yes. I'll look at that again shortly.

Now done.

David

#32

Michael Paquier

michael@paquier.xyz

8 months ago

In reply to: David Rowley (#30)

Re: Add comment explaining why queryid is int64 in pg_stat_statements

On Fri, May 30, 2025 at 11:09:23PM +1200, David Rowley wrote:

Now done.

Cool, thanks. Just did the same for the query ID, closing the open
item.
--
Michael