Horton
Menu

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Adjust some incorrect *GetDatum() macros

  2. Use more consistent *GetDatum() macros for some unsigned numbers

  1. Use correct macro for accessing offset numbers.

    Kirill Reshke <reshkekirill@gmail.com> — 2026-01-11T11:21:43Z

    Hi hackers!

While working on pageinspect support for GIN and SpGiST (welcome to
review them [0] & [1]), I spotted $subi.

PFA trivial patch that uses UInt16GetDatum for OffsetNumber rather
than Int16GetDatum


[0]https://www.postgresql.org/message-id/CALdSSPiN13n7feQcY0WCmq8jzxjwqhNrt1E%3Dg%3Dg6aZANyE_OoQ%40mail.gmail.com
[1] https://www.postgresql.org/message-id/CALdSSPhbAQbFtjK0nT8_G5GsXmsSEVx8J735Ga%2BZxLp9osHcRA%40mail.gmail.com

-- 
Best regards,
Kirill Reshke

  2. Re: Use correct macro for accessing offset numbers.

    Roman Khapov <rkhapov@yandex-team.ru> — 2026-01-11T11:41:19Z

    > On 11 Jan 2026, at 16:21, Kirill Reshke <reshkekirill@gmail.com> wrote:
> 
> Hi hackers!
> 
> PFA trivial patch that uses UInt16GetDatum for OffsetNumber rather
> than Int16GetDatum

Hi!

LGTM, should we check another places of offset number conversations to Datum
as part of this thread?

--
Best regards,
Roman Khapov

  3. Re: Use correct macro for accessing offset numbers.

    Kirill Reshke <reshkekirill@gmail.com> — 2026-01-11T11:58:39Z

    On Sun, 11 Jan 2026 at 16:41, Roman Khapov <rkhapov@yandex-team.ru> wrote:
>
> should we check another places of offset number conversations to Datum
> as part of this thread?

Maybe, I have stopped some more cases, in v2-0001


-- 
Best regards,
Kirill Reshke

  4. Re: Use correct macro for accessing offset numbers.

    Michael Paquier <michael@paquier.xyz> — 2026-01-11T23:23:18Z

    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

  5. RE: Use correct macro for accessing offset numbers.

    li carol <carol.li2025@outlook.com> — 2026-01-12T05:36:15Z

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

  6. Re: Use correct macro for accessing offset numbers.

    zengman <zengman@halodbtech.com> — 2026-01-12T06:53:24Z

    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;
```

--
Regards,
Man Zeng
www.openhalo.org

  7. Re: Use correct macro for accessing offset numbers.

    Kirill Reshke <reshkekirill@gmail.com> — 2026-01-12T08:51:10Z

    On Mon, 12 Jan 2026 at 04:23, Michael Paquier <michael@paquier.xyz> 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 <zengman@halodbtech.com> 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

  8. Re: Use correct macro for accessing offset numbers.

    Michael Paquier <michael@paquier.xyz> — 2026-01-14T08:08:08Z

    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

  9. Re: Use correct macro for accessing offset numbers.

    Peter Eisentraut <peter@eisentraut.org> — 2026-04-24T08:23:23Z

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

  10. Re: Use correct macro for accessing offset numbers.

    Michael Paquier <michael@paquier.xyz> — 2026-04-27T23:26:44Z

    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.gmail.com
--
Michael