Обсуждение: Define DatumGetInt8 function.
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
Вложения
> 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/
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
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
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
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
Вложения
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
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
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
Вложения
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.