Feedback-ID: ie0a040ee:Fastmail
Message-ID: <7f39480a-4b7a-4a51-a9ec-d1189b44432d@eisentraut.org>
Date: Wed, 11 Mar 2026 11:50:52 +0100
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: Define DatumGetInt8 function.
To: Aleksander Alekseev <aleksander@tigerdata.com>,
 pgsql-hackers <pgsql-hackers@postgresql.org>
Cc: Kirill Reshke <reshkekirill@gmail.com>,
 David Rowley <dgrowleyml@gmail.com>, Tom Lane <tgl@sss.pgh.pa.us>
References: 
 <CALdSSPhFyb9qLSHee73XtZm1CBWJNo9+JzFNf-zUEWCRW5yEiQ@mail.gmail.com>
 <2869012.1767023578@sss.pgh.pa.us>
 <CALdSSPg6UgK+6LJmFQ6G3av4J6dbngN7=QwQEuFZApnpmXgVWQ@mail.gmail.com>
 <CAApHDvoShMiWPSkV8zE3tu7uo9GEmuoe=gTpM0+GfOFh2iDmmw@mail.gmail.com>
 <CALdSSPjmukA1WGyYVqJMPD8080Rm9oXPVLn5T64xFCj6wKAenQ@mail.gmail.com>
 <CAJ7c6TPsaLW7O67vqv8YyV_qUUjF0pncDB7dCQJFDw0so7wrdw@mail.gmail.com>
 <CALdSSPjSaTvQL28csVo-8hNg8xJ5=YFr6qwtF=JAMLb1eJO2gQ@mail.gmail.com>
 <CAJ7c6TM4zecvHj0F9Xk-V1gKqjYQOR4EjerTVkrZ18+w-6JN3Q@mail.gmail.com>
Content-Language: en-US
From: Peter Eisentraut <peter@eisentraut.org>
In-Reply-To: 
 <CAJ7c6TM4zecvHj0F9Xk-V1gKqjYQOR4EjerTVkrZ18+w-6JN3Q@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/7f39480a-4b7a-4a51-a9ec-d1189b44432d%40eisentraut.org>
Precedence: bulk

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.