public inbox for [email protected]
help / color / mirror / Atom feedRE: Use correct macro for accessing offset numbers.
5+ messages / 4 participants
[nested] [flat]
* RE: Use correct macro for accessing offset numbers.
@ 2026-01-12 05:36 li carol <[email protected]>
0 siblings, 1 reply; 5+ messages in thread
From: li carol @ 2026-01-12 05:36 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; Kirill Reshke <[email protected]>; +Cc: Roman Khapov <[email protected]>; pgsql-hackers
>
> On Sun, Jan 11, 2026 at 04:58:39PM +0500, Kirill Reshke wrote:
> > Maybe, I have stopped some more cases, in v2-0001
>
> Right. It's true that we could be more consistent for all these based on their
> base type, some of them, particularly in the GIN code now, caring about using
> the correct macro. It may be a good occasion to double-check the whole tree
> for similar holes based on unsigned types.
> --
> Michael
Hi Kirill, Roman, and Michael,
While double-checking the tree for similar holes as Michael suggested, I noticed a couple more inconsistencies in contrib/pageinspect/ginfuncs.c where we are using signed macros for unsigned types.
Specifically, in gin_page_opaque_info, we use Int32GetDatum for maxoff:
values[1] = Int32GetDatum(opaq->maxoff);
Since maxoff is of type OffsetNumber (uint16), this seems to be the exact same pattern Kirill is addressing in other parts of the GIN code. Although it is widened to int32 here, for the sake of consistency, it should probably be using a 16-bit or unsigned macro.
Similarly, in gin_metapage_info, tailFreeSize (which is defined as uint32 in GinMetaPageData) is also converted using Int32GetDatum:
values[2] = Int32GetDatum(metadata->tailFreeSize);
It might be better to include these cleanups in the next version of the patch to ensure all pageinspect GIN functions follow the same standard.
Best regards,
Yuan Li(carol)
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Use correct macro for accessing offset numbers.
@ 2026-01-12 08:51 Kirill Reshke <[email protected]>
parent: li carol <[email protected]>
0 siblings, 1 reply; 5+ messages in thread
From: Kirill Reshke @ 2026-01-12 08:51 UTC (permalink / raw)
To: li carol <[email protected]>; +Cc: Michael Paquier <[email protected]>; Roman Khapov <[email protected]>; pgsql-hackers; zengman <[email protected]>
On Mon, 12 Jan 2026 at 04:23, Michael Paquier <[email protected]> wrote:
>
> On Sun, Jan 11, 2026 at 04:58:39PM +0500, Kirill Reshke wrote:
> > Maybe, I have stopped some more cases, in v2-0001
>
> Right. It's true that we could be more consistent for all these based
> on their base type, some of them, particularly in the GIN code now,
> caring about using the correct macro. It may be a good occasion to
> double-check the whole tree for similar holes based on unsigned types.
> --
> Michael
Ok
> Hi Kirill, Roman, and Michael,
> While double-checking the tree for similar holes as Michael suggested, I noticed a couple more inconsistencies in contrib/pageinspect/ginfuncs.c where we are using signed macros for unsigned types.
> Specifically, in gin_page_opaque_info, we use Int32GetDatum for maxoff:
>
> values[1] = Int32GetDatum(opaq->maxoff);
>
> Since maxoff is of type OffsetNumber (uint16), this seems to be the exact same pattern Kirill is addressing in other parts of the GIN code. Although it is widened to int32 here, for the sake of consistency, it should probably be using a 16-bit or unsigned macro.
>
> Similarly, in gin_metapage_info, tailFreeSize (which is defined as uint32 in GinMetaPageData) is also converted using Int32GetDatum:
>
> values[2] = Int32GetDatum(metadata->tailFreeSize);
>
> It might be better to include these cleanups in the next version of the patch to ensure all pageinspect GIN functions follow the same standard.
Thank you, I have included your findings in v3.
On Mon, 12 Jan 2026 at 11:53, zengman <[email protected]> wrote:
>
> Hi,
>
> I’ve also seen such cases in the kernel code, and I’m wondering if this should be added to the patch here?
> ```
> postgres@zxm-VMware-Virtual-Platform:~/code/postgres$ git diff
> diff --git a/src/backend/utils/adt/lockfuncs.c b/src/backend/utils/adt/lockfuncs.c
> index bcbc226125c..9dadd6da672 100644
> --- a/src/backend/utils/adt/lockfuncs.c
> +++ b/src/backend/utils/adt/lockfuncs.c
> @@ -329,7 +329,7 @@ pg_lock_status(PG_FUNCTION_ARGS)
> values[1] = ObjectIdGetDatum(instance->locktag.locktag_field1);
> values[8] = ObjectIdGetDatum(instance->locktag.locktag_field2);
> values[6] = ObjectIdGetDatum(instance->locktag.locktag_field3);
> - values[9] = Int16GetDatum(instance->locktag.locktag_field4);
> + values[9] = UInt16GetDatum(instance->locktag.locktag_field4);
> nulls[2] = true;
> nulls[3] = true;
> nulls[4] = true;
> @@ -343,7 +343,7 @@ pg_lock_status(PG_FUNCTION_ARGS)
> values[1] = ObjectIdGetDatum(instance->locktag.locktag_field1);
> values[7] = ObjectIdGetDatum(instance->locktag.locktag_field2);
> values[8] = ObjectIdGetDatum(instance->locktag.locktag_field3);
> - values[9] = Int16GetDatum(instance->locktag.locktag_field4);
> + values[9] = UInt16GetDatum(instance->locktag.locktag_field4);
> nulls[2] = true;
> nulls[3] = true;
> nulls[4] = true;
Thank you, I have included your findings in v3.
PFA v3 with fixes for signed usage across the tree, with my new
findings and suggestions from thread
--
Best regards,
Kirill Reshke
Attachments:
[application/octet-stream] v3-0001-Use-correct-macro-for-accessing-unsigned-numbers.patch (9.3K, 2-v3-0001-Use-correct-macro-for-accessing-unsigned-numbers.patch)
download | inline diff:
From 9c5b37d9f4b7a355ed57979c3fcac02965318149 Mon Sep 17 00:00:00 2001
From: reshke <[email protected]>
Date: Sun, 11 Jan 2026 11:17:13 +0000
Subject: [PATCH v3] Use correct macro for accessing unsigned numbers.
This patch cleanups usage of Int16GetDatum vs UInt16GetDatum,
enforcing correct marco usage in number of places.
Patch includes using UInt16GetDatum for OffsetNumbers (which are uin16)
and same for strategy numbers. Some parts of commits are suggested
by hackers in thread.
Reviewed-by: Roman Khapov <[email protected]>
Suggested-by: Michael Paquier <[email protected]>
Suggested-by: li carol <[email protected]>
Suggested-by: zengman <[email protected]>
Discussion: https://postgr.es/m/CALdSSPidtC7j3MwhkqRj0K2hyp36ztnnjSt6qzGxQtiePR1dzw@mail.gmail.com
---
contrib/pageinspect/btreefuncs.c | 2 +-
contrib/pageinspect/ginfuncs.c | 4 ++--
contrib/pageinspect/gistfuncs.c | 4 ++--
contrib/pg_buffercache/pg_buffercache_pages.c | 2 +-
src/backend/access/brin/brin_inclusion.c | 2 +-
src/backend/access/brin/brin_minmax.c | 2 +-
src/backend/access/brin/brin_minmax_multi.c | 2 +-
src/backend/access/gist/gistget.c | 4 ++--
src/backend/storage/aio/aio_funcs.c | 2 +-
src/backend/utils/adt/lockfuncs.c | 4 ++--
src/backend/utils/cache/lsyscache.c | 2 +-
11 files changed, 15 insertions(+), 15 deletions(-)
diff --git a/contrib/pageinspect/btreefuncs.c b/contrib/pageinspect/btreefuncs.c
index 62c905c6e7c..0585b7cee40 100644
--- a/contrib/pageinspect/btreefuncs.c
+++ b/contrib/pageinspect/btreefuncs.c
@@ -507,7 +507,7 @@ bt_page_print_tuples(ua_page_items *uargs)
j = 0;
memset(nulls, 0, sizeof(nulls));
- values[j++] = Int16GetDatum(offset);
+ values[j++] = UInt16GetDatum(offset);
values[j++] = ItemPointerGetDatum(&itup->t_tid);
values[j++] = Int32GetDatum((int) IndexTupleSize(itup));
values[j++] = BoolGetDatum(IndexTupleHasNulls(itup));
diff --git a/contrib/pageinspect/ginfuncs.c b/contrib/pageinspect/ginfuncs.c
index ebcc2b3db5c..4825ad3aece 100644
--- a/contrib/pageinspect/ginfuncs.c
+++ b/contrib/pageinspect/ginfuncs.c
@@ -73,7 +73,7 @@ gin_metapage_info(PG_FUNCTION_ARGS)
values[0] = Int64GetDatum(metadata->head);
values[1] = Int64GetDatum(metadata->tail);
- values[2] = Int32GetDatum(metadata->tailFreeSize);
+ values[2] = UInt32GetDatum(metadata->tailFreeSize);
values[3] = Int64GetDatum(metadata->nPendingPages);
values[4] = Int64GetDatum(metadata->nPendingHeapTuples);
@@ -159,7 +159,7 @@ gin_page_opaque_info(PG_FUNCTION_ARGS)
memset(nulls, 0, sizeof(nulls));
values[0] = Int64GetDatum(opaq->rightlink);
- values[1] = Int32GetDatum(opaq->maxoff);
+ values[1] = UInt16GetDatum(opaq->maxoff);
values[2] = PointerGetDatum(construct_array_builtin(flags, nflags, TEXTOID));
/* Build and return the result tuple. */
diff --git a/contrib/pageinspect/gistfuncs.c b/contrib/pageinspect/gistfuncs.c
index 60a4b240302..9b7e3cec882 100644
--- a/contrib/pageinspect/gistfuncs.c
+++ b/contrib/pageinspect/gistfuncs.c
@@ -175,7 +175,7 @@ gist_page_items_bytea(PG_FUNCTION_ARGS)
memset(nulls, 0, sizeof(nulls));
- values[0] = Int16GetDatum(offset);
+ values[0] = UInt16GetDatum(offset);
values[1] = ItemPointerGetDatum(&itup->t_tid);
values[2] = Int32GetDatum((int) IndexTupleSize(itup));
@@ -282,7 +282,7 @@ gist_page_items(PG_FUNCTION_ARGS)
memset(nulls, 0, sizeof(nulls));
- values[0] = Int16GetDatum(offset);
+ values[0] = UInt16GetDatum(offset);
values[1] = ItemPointerGetDatum(&itup->t_tid);
values[2] = Int32GetDatum((int) IndexTupleSize(itup));
values[3] = BoolGetDatum(ItemIdIsDead(id));
diff --git a/contrib/pg_buffercache/pg_buffercache_pages.c b/contrib/pg_buffercache/pg_buffercache_pages.c
index 0c58e4b265c..b682dca658b 100644
--- a/contrib/pg_buffercache/pg_buffercache_pages.c
+++ b/contrib/pg_buffercache/pg_buffercache_pages.c
@@ -276,7 +276,7 @@ pg_buffercache_pages(PG_FUNCTION_ARGS)
nulls[5] = false;
values[6] = BoolGetDatum(fctx->record[i].isdirty);
nulls[6] = false;
- values[7] = Int16GetDatum(fctx->record[i].usagecount);
+ values[7] = UInt16GetDatum(fctx->record[i].usagecount);
nulls[7] = false;
/* unused for v1.0 callers, but the array is always long enough */
values[8] = Int32GetDatum(fctx->record[i].pinning_backends);
diff --git a/src/backend/access/brin/brin_inclusion.c b/src/backend/access/brin/brin_inclusion.c
index 08890a3d009..5a2058d9aad 100644
--- a/src/backend/access/brin/brin_inclusion.c
+++ b/src/backend/access/brin/brin_inclusion.c
@@ -641,7 +641,7 @@ inclusion_get_strategy_procinfo(BrinDesc *bdesc, uint16 attno, Oid subtype,
tuple = SearchSysCache4(AMOPSTRATEGY, ObjectIdGetDatum(opfamily),
ObjectIdGetDatum(attr->atttypid),
ObjectIdGetDatum(subtype),
- Int16GetDatum(strategynum));
+ UInt16GetDatum(strategynum));
if (!HeapTupleIsValid(tuple))
elog(ERROR, "missing operator %d(%u,%u) in opfamily %u",
diff --git a/src/backend/access/brin/brin_minmax.c b/src/backend/access/brin/brin_minmax.c
index 9d4e47b4dc0..73201029371 100644
--- a/src/backend/access/brin/brin_minmax.c
+++ b/src/backend/access/brin/brin_minmax.c
@@ -294,7 +294,7 @@ minmax_get_strategy_procinfo(BrinDesc *bdesc, uint16 attno, Oid subtype,
tuple = SearchSysCache4(AMOPSTRATEGY, ObjectIdGetDatum(opfamily),
ObjectIdGetDatum(attr->atttypid),
ObjectIdGetDatum(subtype),
- Int16GetDatum(strategynum));
+ UInt16GetDatum(strategynum));
if (!HeapTupleIsValid(tuple))
elog(ERROR, "missing operator %d(%u,%u) in opfamily %u",
diff --git a/src/backend/access/brin/brin_minmax_multi.c b/src/backend/access/brin/brin_minmax_multi.c
index 6b86b1fd889..688ca9f2dbb 100644
--- a/src/backend/access/brin/brin_minmax_multi.c
+++ b/src/backend/access/brin/brin_minmax_multi.c
@@ -2932,7 +2932,7 @@ minmax_multi_get_strategy_procinfo(BrinDesc *bdesc, uint16 attno, Oid subtype,
tuple = SearchSysCache4(AMOPSTRATEGY, ObjectIdGetDatum(opfamily),
ObjectIdGetDatum(attr->atttypid),
ObjectIdGetDatum(subtype),
- Int16GetDatum(strategynum));
+ UInt16GetDatum(strategynum));
if (!HeapTupleIsValid(tuple))
elog(ERROR, "missing operator %d(%u,%u) in opfamily %u",
strategynum, attr->atttypid, subtype, opfamily);
diff --git a/src/backend/access/gist/gistget.c b/src/backend/access/gist/gistget.c
index 6d05a5fdc34..d6de8e954e8 100644
--- a/src/backend/access/gist/gistget.c
+++ b/src/backend/access/gist/gistget.c
@@ -222,7 +222,7 @@ gistindex_keytest(IndexScanDesc scan,
key->sk_collation,
PointerGetDatum(&de),
key->sk_argument,
- Int16GetDatum(key->sk_strategy),
+ UInt16GetDatum(key->sk_strategy),
ObjectIdGetDatum(key->sk_subtype),
PointerGetDatum(&recheck));
@@ -286,7 +286,7 @@ gistindex_keytest(IndexScanDesc scan,
key->sk_collation,
PointerGetDatum(&de),
key->sk_argument,
- Int16GetDatum(key->sk_strategy),
+ UInt16GetDatum(key->sk_strategy),
ObjectIdGetDatum(key->sk_subtype),
PointerGetDatum(&recheck));
*recheck_distances_p |= recheck;
diff --git a/src/backend/storage/aio/aio_funcs.c b/src/backend/storage/aio/aio_funcs.c
index 8997c762062..980e96c4582 100644
--- a/src/backend/storage/aio/aio_funcs.c
+++ b/src/backend/storage/aio/aio_funcs.c
@@ -197,7 +197,7 @@ retry:
values[7] = CStringGetTextDatum(pgaio_io_get_target_name(&ioh_copy));
/* column: length of IO's data array */
- values[8] = Int16GetDatum(ioh_copy.handle_data_len);
+ values[8] = UInt16GetDatum(ioh_copy.handle_data_len);
/* column: raw result (i.e. some form of syscall return value) */
if (start_state == PGAIO_HS_COMPLETED_IO
diff --git a/src/backend/utils/adt/lockfuncs.c b/src/backend/utils/adt/lockfuncs.c
index bcbc226125c..9dadd6da672 100644
--- a/src/backend/utils/adt/lockfuncs.c
+++ b/src/backend/utils/adt/lockfuncs.c
@@ -329,7 +329,7 @@ pg_lock_status(PG_FUNCTION_ARGS)
values[1] = ObjectIdGetDatum(instance->locktag.locktag_field1);
values[8] = ObjectIdGetDatum(instance->locktag.locktag_field2);
values[6] = ObjectIdGetDatum(instance->locktag.locktag_field3);
- values[9] = Int16GetDatum(instance->locktag.locktag_field4);
+ values[9] = UInt16GetDatum(instance->locktag.locktag_field4);
nulls[2] = true;
nulls[3] = true;
nulls[4] = true;
@@ -343,7 +343,7 @@ pg_lock_status(PG_FUNCTION_ARGS)
values[1] = ObjectIdGetDatum(instance->locktag.locktag_field1);
values[7] = ObjectIdGetDatum(instance->locktag.locktag_field2);
values[8] = ObjectIdGetDatum(instance->locktag.locktag_field3);
- values[9] = Int16GetDatum(instance->locktag.locktag_field4);
+ values[9] = UInt16GetDatum(instance->locktag.locktag_field4);
nulls[2] = true;
nulls[3] = true;
nulls[4] = true;
diff --git a/src/backend/utils/cache/lsyscache.c b/src/backend/utils/cache/lsyscache.c
index b924a2d900b..81a6bde0af5 100644
--- a/src/backend/utils/cache/lsyscache.c
+++ b/src/backend/utils/cache/lsyscache.c
@@ -176,7 +176,7 @@ get_opfamily_member(Oid opfamily, Oid lefttype, Oid righttype,
ObjectIdGetDatum(opfamily),
ObjectIdGetDatum(lefttype),
ObjectIdGetDatum(righttype),
- Int16GetDatum(strategy));
+ UInt16GetDatum(strategy));
if (!HeapTupleIsValid(tp))
return InvalidOid;
amop_tup = (Form_pg_amop) GETSTRUCT(tp);
--
2.43.0
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Use correct macro for accessing offset numbers.
@ 2026-01-14 08:08 Michael Paquier <[email protected]>
parent: Kirill Reshke <[email protected]>
0 siblings, 1 reply; 5+ messages in thread
From: Michael Paquier @ 2026-01-14 08:08 UTC (permalink / raw)
To: Kirill Reshke <[email protected]>; +Cc: li carol <[email protected]>; Roman Khapov <[email protected]>; pgsql-hackers; zengman <[email protected]>
On Mon, Jan 12, 2026 at 01:51:10PM +0500, Kirill Reshke wrote:
> PFA v3 with fixes for signed usage across the tree, with my new
> findings and suggestions from thread
Note that the change in get_opfamily_member() is not right based on
the type of "strategy". The rest was OK, so done.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, 2-signature.asc)
download
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Use correct macro for accessing offset numbers.
@ 2026-04-24 08:23 Peter Eisentraut <[email protected]>
parent: Michael Paquier <[email protected]>
0 siblings, 1 reply; 5+ messages in thread
From: Peter Eisentraut @ 2026-04-24 08:23 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; Kirill Reshke <[email protected]>; +Cc: li carol <[email protected]>; Roman Khapov <[email protected]>; pgsql-hackers; zengman <[email protected]>
On 14.01.26 09:08, Michael Paquier wrote:
> On Mon, Jan 12, 2026 at 01:51:10PM +0500, Kirill Reshke wrote:
>> PFA v3 with fixes for signed usage across the tree, with my new
>> findings and suggestions from thread
>
> Note that the change in get_opfamily_member() is not right based on
> the type of "strategy". The rest was OK, so done.
The thread [0] is proposing a patch to change these things in the
opposite direction, effectively reverting commit 6dcfac9696c.
I think the premise of the patch in this thread is incorrect. You have
changed
Int16GetDatum(offset)
to
UInt16GetDatum(offset)
because the variable offset is of type OffsetNumber, which is uint16.
But that is not the meaning of the "UInt16" in UInt16GetDatum(), at
least that's the argument being made in the other thread.
These values end up being converted to an output parameter of type
smallint, and the output function int2out uses DatumGetInt16() to
convert its argument. So the *GetDatum() function should match that, so
we should use Int16GetDatum().
The real problem here is that offset values that are uint32 are being
output via the SQL type smallint, which can't handle the whole set of
values, but this is probably not a problem in practice.
[0]:
https://www.postgresql.org/message-id/flat/CALdSSPhFyb9qLSHee73XtZm1CBWJNo9+JzFNf-zUEWCRW5yEiQ@mail....
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Use correct macro for accessing offset numbers.
@ 2026-04-27 23:26 Michael Paquier <[email protected]>
parent: Peter Eisentraut <[email protected]>
0 siblings, 0 replies; 5+ messages in thread
From: Michael Paquier @ 2026-04-27 23:26 UTC (permalink / raw)
To: Peter Eisentraut <[email protected]>; +Cc: Kirill Reshke <[email protected]>; li carol <[email protected]>; Roman Khapov <[email protected]>; pgsql-hackers; zengman <[email protected]>
On Fri, Apr 24, 2026 at 10:23:23AM +0200, Peter Eisentraut wrote:
> The thread [0] is proposing a patch to change these things in the opposite
> direction, effectively reverting commit 6dcfac9696c.
Thanks for the poke. This qualifies as an open item assigned to me, I
guess. I'll double-check the whole and reply on the other thread
where the patch has been posted at [1].
> The real problem here is that offset values that are uint32 are being output
> via the SQL type smallint, which can't handle the whole set of values, but
> this is probably not a problem in practice.
Will check all that.
[1]: https://www.postgresql.org/message-id/CAJ7c6TMcGu8qmRe1gZfJ-gOzVnZq-t%3Dfwn-UuyStx1w6ZyydMw%40mail.g...
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, 2-signature.asc)
download
^ permalink raw reply [nested|flat] 5+ messages in thread
end of thread, other threads:[~2026-04-27 23:26 UTC | newest]
Thread overview: 5+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2026-01-12 05:36 RE: Use correct macro for accessing offset numbers. li carol <[email protected]>
2026-01-12 08:51 ` Kirill Reshke <[email protected]>
2026-01-14 08:08 ` Michael Paquier <[email protected]>
2026-04-24 08:23 ` Peter Eisentraut <[email protected]>
2026-04-27 23:26 ` Michael Paquier <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox