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. Remove Int8GetDatum function

  1. Define DatumGetInt8 function.

    Kirill Reshke <reshkekirill@gmail.com> — 2025-12-29T11:02:34Z

    Hi hackers!

I am currently involved in the Cloudberry kernel rebase process[0]. We
are rebasing [0] which is based on pg-14 on pg-16 kernel. During this
process I noticed rebase conflicts introduced  by c8b2ef0. This commit
defines a number of include functions for datum conversion.

During this rebase resolution, I noticed that there is an Int8GetDatum
function, but there is no DatumGetInt8, which I want to use. All other
inline functions seem to be provided in pairs by postgres.h. So it
looks convenient to define DatumGetInt8 in postgres.h?

FPA doing just that.


[0] https://github.com/apache/cloudberry

-- 
Best regards,
Kirill Reshke

  2. Re: Define DatumGetInt8 function.

    Chao Li <li.evan.chao@gmail.com> — 2025-12-29T11:08:25Z

    
> On Dec 29, 2025, at 19:02, Kirill Reshke <reshkekirill@gmail.com> wrote:
> 
> Hi hackers!
> 
> I am currently involved in the Cloudberry kernel rebase process[0]. We
> are rebasing [0] which is based on pg-14 on pg-16 kernel. During this
> process I noticed rebase conflicts introduced  by c8b2ef0. This commit
> defines a number of include functions for datum conversion.
> 
> During this rebase resolution, I noticed that there is an Int8GetDatum
> function, but there is no DatumGetInt8, which I want to use. All other
> inline functions seem to be provided in pairs by postgres.h. So it
> looks convenient to define DatumGetInt8 in postgres.h?
> 
> FPA doing just that.
> 
> 
> [0] https://github.com/apache/cloudberry
> 
> -- 
> Best regards,
> Kirill Reshke
> <v1-0001-Define-DatumGetInt8-function.patch>

LGTM.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

  3. Re: Define DatumGetInt8 function.

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-12-29T15:52:58Z

    Kirill Reshke <reshkekirill@gmail.com> writes:
> During this rebase resolution, I noticed that there is an Int8GetDatum
> function, but there is no DatumGetInt8, which I want to use. All other
> inline functions seem to be provided in pairs by postgres.h. So it
> looks convenient to define DatumGetInt8 in postgres.h?

I would actually turn this around and ask why we have Int8GetDatum?
We have no SQL types for which that is well-adapted.  I see no
uses of Int8GetDatum in our tree, and only three uses of
UInt8GetDatum, and all three of those look like type puns to me.
(heap_page_items is returning a smallint, and btcharskipsupport
should be using CharGetDatum.)

So from where I sit these look like an attractive nuisance that
we should remove rather than encourage use of.  If you have
some extension data type for which these make sense, that's
fine, but it doesn't mean they should be in core Postgres.

			regards, tom lane

  4. Re: Define DatumGetInt8 function.

    Kirill Reshke <reshkekirill@gmail.com> — 2025-12-29T16:01:10Z

    On Mon, 29 Dec 2025 at 20:52, Tom Lane <tgl@sss.pgh.pa.us> wrote:

> So from where I sit these look like an attractive nuisance that
> we should remove rather than encourage use of.  If you have
> some extension data type for which these make sense, that's
> fine, but it doesn't mean they should be in core Postgres.
>
>                         regards, tom lane

Well, OK. Removal is also fine for me, because it is at least consistent.

-- 
Best regards,
Kirill Reshke

  5. Re: Define DatumGetInt8 function.

    David Rowley <dgrowleyml@gmail.com> — 2026-01-06T11:49:59Z

    On Tue, 30 Dec 2025 at 05:01, Kirill Reshke <reshkekirill@gmail.com> wrote:
> Well, OK. Removal is also fine for me, because it is at least consistent.

Kirill, are you working on this patch?  I've not studied in detail,
but looks like it would require making
char_decrement()/char_increment() and btcharskipsupport() all use
CharGetDatum()/DatumGetChar() and switch to using SCHAR_MIN/SCHAR_MAX
rather than 0/UCHAR_MAX.

If you're not working on it, it would be good if you could withdraw
the patch from the commitfest.

David

  6. Re: Define DatumGetInt8 function.

    Kirill Reshke <reshkekirill@gmail.com> — 2026-01-06T14:12:33Z

    On Tue, 6 Jan 2026 at 16:50, David Rowley <dgrowleyml@gmail.com> wrote:
>
> On Tue, 30 Dec 2025 at 05:01, Kirill Reshke <reshkekirill@gmail.com> wrote:
> > Well, OK. Removal is also fine for me, because it is at least consistent.
>

> Kirill, are you working on this patch?


Hi!
Yes, PFA.

 > I've not studied in detail,
> but looks like it would require making
> char_decrement()/char_increment() and btcharskipsupport() all use
> CharGetDatum()/DatumGetChar() and switch to using SCHAR_MIN/SCHAR_MAX
> rather than 0/UCHAR_MAX.

This is also a possible change, and can be a separate patch. I will
try to also work on this this week.



-- 
Best regards,
Kirill Reshke

  7. Re: Define DatumGetInt8 function.

    Aleksander Alekseev <aleksander@tigerdata.com> — 2026-01-06T14:21:55Z

    Hi,

> > Kirill, are you working on this patch?
>
> Yes, PFA.
>
>  > I've not studied in detail,
> > but looks like it would require making
> > char_decrement()/char_increment() and btcharskipsupport() all use
> > CharGetDatum()/DatumGetChar() and switch to using SCHAR_MIN/SCHAR_MAX
> > rather than 0/UCHAR_MAX.
>
> This is also a possible change, and can be a separate patch. I will
> try to also work on this this week.

v1 looks incomplete to me. As I understand, Tom proposed to get rid of
UInt8 conversions as well. Are you going to implement this idea as
well?

-- 
Best regards,
Aleksander Alekseev

  8. Re: Define DatumGetInt8 function.

    Kirill Reshke <reshkekirill@gmail.com> — 2026-01-06T14:27:47Z

    On Tue, 6 Jan 2026 at 19:22, Aleksander Alekseev
<aleksander@tigerdata.com> wrote:
>
> Hi,
>
> > > Kirill, are you working on this patch?
> >
> > Yes, PFA.
> >
> >  > I've not studied in detail,
> > > but looks like it would require making
> > > char_decrement()/char_increment() and btcharskipsupport() all use
> > > CharGetDatum()/DatumGetChar() and switch to using SCHAR_MIN/SCHAR_MAX
> > > rather than 0/UCHAR_MAX.
> >
> > This is also a possible change, and can be a separate patch. I will
> > try to also work on this this week.
>
> v1 looks incomplete to me. As I understand, Tom proposed to get rid of
> UInt8 conversions as well. Are you going to implement this idea as
> well?
>


Hmm, v1 looks good and self-contained to me. Like, anyway, making two
commits (one for signed Int8 and one for unsigned) here is better for
sake of atomicy?
Anyway, I can see there are users of UInt8GetDatum, which are [0] and
forks of Greenplum. So, I am not super-sure removing UInt8* is
desirable.


[0] https://github.com/duckdb/pg_duckdb/blob/main/src/pgduckdb_types.cpp#L230


-- 
Best regards,
Kirill Reshke

  9. Re: Define DatumGetInt8 function.

    Aleksander Alekseev <aleksander@tigerdata.com> — 2026-01-07T14:03:19Z

    Hi,

> Hmm, v1 looks good and self-contained to me. Like, anyway, making two
> commits (one for signed Int8 and one for unsigned) here is better for
> sake of atomicy?
> Anyway, I can see there are users of UInt8GetDatum, which are [0] and
> forks of Greenplum. So, I am not super-sure removing UInt8* is
> desirable.

Fair enough. Let it be a separate patch then.

--
Best regards,
Aleksander Alekseev

  10. Re: Define DatumGetInt8 function.

    Peter Eisentraut <peter@eisentraut.org> — 2026-03-11T10:50:52Z

    On 07.01.26 15:03, Aleksander Alekseev wrote:
>> Hmm, v1 looks good and self-contained to me. Like, anyway, making two
>> commits (one for signed Int8 and one for unsigned) here is better for
>> sake of atomicy?
>> Anyway, I can see there are users of UInt8GetDatum, which are [0] and
>> forks of Greenplum. So, I am not super-sure removing UInt8* is
>> desirable.
> 
> Fair enough. Let it be a separate patch then.

I have committed the first patch, which removes Int8GetDatum().  (This 
is actually used by my extension pguint, but that already needed to 
provide a replacement for the non-existent DatumGetInt8(), so it's not a 
bother to provide a replacement for Int8GetDatum() for future PG versions.)

About the other patch, two points:

1) The changes in nbtcompare.c look a little messy with respect to 
signedness and unsignedness of char.  I don't know what this code 
actually does at a higher level, but the low-level details look weird. 
Like, why does char_decrement() get its input value using 
DatumGetUInt8() but makes the return value using CharGetDatum()?  And 
your patch changes the UCHAR_MAX to SCHAR_MAX but changes the variable 
from uint8 to char.  First, there is no explanation why this change from 
unsigned to unclear-sign is correct, and second, if you are using the 
char type you should then also use the matching symbol CHAR_MAX.

I'm tempted to think the correct direction here would be to use uint8 
and its associated macros and functions more rigorously, not less.

2) The change in pageinspect looks correct, but then when you look 
nearby and further around, you will find that there are also a bunch of 
(if not most) UInt16GetDatum and UInt32GetDatum that are wrong.  I think 
there is some confusion about what the "UIntNN" or "IntNN" in these 
functions refers to.  Some code appears to think that this refers to the 
input type of that function call, but it's actually more like what SQL 
data type the value will be used with.  (Some maybe it would be more 
helpful to think of it as "GetDatumForInt32" etc.)

There are a lot of opportunities to clean this up, and I suspect that 
while many of these will work either way in practice because the actual 
values don't go far enough to hit the signed/unsigned boundary, I think 
there could a number of little bugs here to be fixed.

I don't think it's worth making an isolated fix here for the one use of 
UInt8GetDatum(), especially if you believe my point 1) and we are not 
going to be removing this function anyway.

  11. Re: Define DatumGetInt8 function.

    Aleksander Alekseev <aleksander@tigerdata.com> — 2026-03-13T10:55:44Z

    Hi Peter,

Thanks for your feedback.

> I'm tempted to think the correct direction here would be to use uint8
> and its associated macros and functions more rigorously, not less.

OK, if my understanding is correct this leaves us char_increment() and
char_decrement() which currently use DatumGetUInt8 inconsistently with
CharGetDatum for the argument and return value correspondingly.

> I don't think it's worth making an isolated fix here for the one use of
> UInt8GetDatum()

I think you meant not this particular change so I included it to the
patch. We can keep nbtcompare.c as if you believe this change is not
that important (it arguably isn't).

> The change in pageinspect looks correct, but then when you look
> nearby and further around, you will find that there are also a bunch of
> (if not most) UInt16GetDatum and UInt32GetDatum that are wrong
> [...]

Agree. I did my best to fix all the inconsistencies in pageinsect.

> There are a lot of opportunities to clean this up, and I suspect that
> while many of these will work either way in practice because the actual
> values don't go far enough to hit the signed/unsigned boundary, I think
> there could a number of little bugs here to be fixed.

I believe you were referring to places other than pageinspect. I will
investigate and create separate threads with corresponding patches
later.

-- 
Best regards,
Aleksander Alekseev

  12. Re: Define DatumGetInt8 function.

    Peter Eisentraut <peter@eisentraut.org> — 2026-03-27T12:17:15Z

    On 13.03.26 11:55, Aleksander Alekseev wrote:
> Hi Peter,
> 
> Thanks for your feedback.
> 
>> I'm tempted to think the correct direction here would be to use uint8
>> and its associated macros and functions more rigorously, not less.
> 
> OK, if my understanding is correct this leaves us char_increment() and
> char_decrement() which currently use DatumGetUInt8 inconsistently with
> CharGetDatum for the argument and return value correspondingly.
> 
>> I don't think it's worth making an isolated fix here for the one use of
>> UInt8GetDatum()
> 
> I think you meant not this particular change so I included it to the
> patch. We can keep nbtcompare.c as if you believe this change is not
> that important (it arguably isn't).
> 
>> The change in pageinspect looks correct, but then when you look
>> nearby and further around, you will find that there are also a bunch of
>> (if not most) UInt16GetDatum and UInt32GetDatum that are wrong
>> [...]
> 
> Agree. I did my best to fix all the inconsistencies in pageinsect.
> 
>> There are a lot of opportunities to clean this up, and I suspect that
>> while many of these will work either way in practice because the actual
>> values don't go far enough to hit the signed/unsigned boundary, I think
>> there could a number of little bugs here to be fixed.
> 
> I believe you were referring to places other than pageinspect. I will
> investigate and create separate threads with corresponding patches
> later.

I'm moving this patch to the next commitfest.  It's worth continuing to 
work on making this more correct, but AFAICT no bug has been claimed 
here, so it's not worth rushing this now.

  13. Re: Define DatumGetInt8 function.

    Michael Paquier <michael@paquier.xyz> — 2026-04-28T05:13:31Z

    On Fri, Mar 27, 2026 at 01:17:15PM +0100, Peter Eisentraut wrote:
> I'm moving this patch to the next commitfest.  It's worth continuing to work
> on making this more correct, but AFAICT no bug has been claimed here, so
> it's not worth rushing this now.

Well, as you have pointed out at [1], this is undoing a portion of
6dcfac9696cb that has changed a set of *GetDatum() functions to match
with the C type of the variables they work on, and not the output of
the function, which is incorrect.  This means an open item standing on
top of my head for this release, that needs to be acted on.

The patch proposed at [2] handles the problem partially,
unfortunately.  I have spotted a lot more places where we use a
*GetDatum() that does not match with the data type of the SQL function
related to.  In terms of locations spotted, some notes:
- Of course, the..  cough..  places that 6dcfac9696cb has touched,
reverted now to their original form to match the SQL data type (BRIN,
pageinspect, GiST, lockfuncs.c, one in pg_buffercache).
- The IndexTupleSize() business in pageinspect is a good catch.  All
these related to smallint, used a int32 GetDatum().
- Not completely sure about the ones in nbtcompare.c, TBH.
- lockfuncs.c is much more inconsistent than I thought, even after
reverting the bits of 6dcfac9696cb.  pg_lock_status() was a mixed bag
of incorrectness (field3 for LOCKTAG_PAGE, field3 and field4 for
LOCKTAG_TUPLE, two more for PREDLOCKTAG_TUPLE).
- ginlogic.c, for triConsistentFn and consistentFmgrInfo.
nuserentries is a uint32, but the functions require Int32GetDatum() as
use int4 for the data type.
- xlogfuncs.c, pg_walfile_name_offset() should use Int32GetDatum() for
xrecoff.
- pg_proc.c, for pronargs and pronargdefaults.  Quite an old one.
- pg_logicalinspect, 4 spots for the snapshot functions.
- pg_walinspect, for various record data.
- pgstatindex, pending pages.

In short, it took me some time to put some order into all that,
finishing with the attached patch set.  0001 is the minimum for v19,
that reverts 6dcfac9696cb so as we have more *GetDatum() matching with
the types in the SQL functions.  That would take care of the open item
on top of my head.

0002 is a set of fixes that I have spotted while investigating this
set of issues in depth.  These spots are actually wrong, some of them
for a long time.  I would be slightly tempted to do something about
these in v19 rather than wait for v20, as these are somewhat latent
bugs, to have more consistency across the board.  Has anybody from the
RMT an opinion to offer?  There is not much urgency in it, still..
Added the RMT in CC for opinions.

[1]: https://www.postgresql.org/message-id/97f9375a-be61-4272-a44d-408337fe8fa6%40eisentraut.org
[2]: https://www.postgresql.org/message-id/CAJ7c6TMcGu8qmRe1gZfJ-gOzVnZq-t%3Dfwn-UuyStx1w6ZyydMw%40mail.gmail.com
--
Michael

  14. Re: Define DatumGetInt8 function.

    Michael Paquier <michael@paquier.xyz> — 2026-04-30T05:28:04Z

    On Tue, Apr 28, 2026 at 02:13:31PM +0900, Michael Paquier wrote:
> In short, it took me some time to put some order into all that,
> finishing with the attached patch set.  0001 is the minimum for v19,
> that reverts 6dcfac9696cb so as we have more *GetDatum() matching with
> the types in the SQL functions.  That would take care of the open item
> on top of my head.

This was not completely right after a second look.  6dcfac9696cb did
not get things completely wrong, either, the brin and gist parts of
the changes were right.  I have undone the incorrect bits for now to
address the open item.

> 0002 is a set of fixes that I have spotted while investigating this
> set of issues in depth.  These spots are actually wrong, some of them
> for a long time.  I would be slightly tempted to do something about
> these in v19 rather than wait for v20, as these are somewhat latent
> bugs, to have more consistency across the board.  Has anybody from the
> RMT an opinion to offer?  There is not much urgency in it, still..
> Added the RMT in CC for opinions.

This deserves a different discussion, unrelated to DatumGetInt8().
I'll post that on a separate thread, for v20.
--
Michael