Re: pgsql: Make cancel request keys longer

(moving to pgsql-hackers)

On 09/04/2025 12:39, Peter Eisentraut wrote:
> On 09.04.25 10:53, Heikki Linnakangas wrote:
>> On 08/04/2025 22:41, Heikki Linnakangas wrote:
>>> On 08/04/2025 20:06, Peter Eisentraut wrote:
>>>> While I was looking at this, I suggest to make the first argument 
>>>> void *.  This is consistent for passing binary data.
>>>
>>> Ok, sure.
>>
>> On second thoughts, -1 on that. 'void *' is appropriate for functions 
>> like libc's read() or pq_sendbytes(), where the buffer can point to 
>> anything. In other words, the caller is expected to have a pointer 
>> like 'foobar *', and it gets cast to 'void *' when you call the 
>> function. That's not the case with the cancellation key. The 
>> cancellation key is just an array of bytes, the caller is expected to 
>> pass an array of bytes, not a struct.
>>
>> The right precedent for that are e.g. SCRAM functions in scram- 
>> common.h, for example. They use "const uint8 *" for the hashes.
>>
>> I'll switch to "const uint *" everywhere that deals with cancel keys. 
>> There are a few more variables elsewhere in the backend and in libpq.
> 
> I was having the same second thoughts overnight.  I agree with your 
> conclusion.

Here's a patch to change cancellation keys to "uint8 *". I did the same 
for a few other places, namely the new scram_client_key_binary and 
scram_server_key_binary fields in pg_conn, and a few libpq functions 
that started to give compiler warnings after that. There probably would 
be more code that could be changed to follow this convention, but I 
didn't look hard. What do you think?

I'm on the edge with the pg_b64_encode/decode functions, whether they 
should work on "uint8 *" or "void *". On one hand, you do base64 
encoding on a byte array, which would support "uint8 *". But on the 
other hand, you might use it for encoding things with more structure, 
which would support "void *". I went with "void *", mostly out of 
convenience as many of the SCRAM functions that currently use 
pg_b64_encode/decode, use "char *" to represent byte arrays. But 
arguably those should be changed to use "uint8 *" too.

I committed the other parts of your original patch, thanks!

-- 
Heikki Linnakangas
Neon (https://neon.tech)