Re: Make query cancellation keys longer

On 22/02/2025 16:58, Jelte Fennema-Nio wrote:
> On Mon, 9 Sept 2024 at 17:58, Robert Haas  wrote:
>>
>> On Fri, Aug 16, 2024 at 11:29 AM Robert Haas  wrote:
>>>> I'll split this patch like that, to make it easier to compare and merge
>>>> with Jelte's corresponding patches.
>>>
>>> That sounds great. IMHO, comparing and merging the patches is the next
>>> step here and would be great to see.
>>
>> Heikki, do you have any update on this work?
> 
> My patchset in the other protocol thread needed a rebase. So I took
> that as an opportunity to rebase this patchset on top of it, because
> this seems to be the protocol change that we can most easily push over
> the finish line.
> 
> 1. I changed the last patch from Heikki to only contain the changes
> for the cancel lengthening. The general protocol change related things
> I merged with mine (I only kept some error handling and docs).
> 2. I also removed the length field in the new BackendKey definition,
> eventhough I asked for that addition previously. I agree with Heikki
> that it's actually easier to parse and create without, because you can
> use the same code for both versions.
> 3. I made our timingsafe_bcmp implementation call into OpenSSL's CRYPTO_memcmp.

Great, thanks!

I spent some time going over this again, making tiny fixes here and 
there. A couple of somewhat notable fixes:

- fixed the "server only supports protocol version %d.%d, but 
min_protocol_version was set to %d.%d" message to print the server's 
protocol version correctly
- error out if the server sends two NegotiateProtocolVersion messages 
for some reason
- added a new "protocol versions" subsection to the docs, moving the 
existing paragraphs related to version negotiation there. I also added a 
table to list the different protocol versions. See the last patch in the 
series.

I'm a bit disappointed that we're not changing the default protocol 
version to 3.2 yet. It will receive very little usage in practice until 
we do. But that seems to be the least objectionable path forward.

I think this is pretty much ready for commit. I will go over it one more 
time, and plan to push tomorrow.

> One open question on the last patch is: Document what the maximum size
> of the cancel key is that the client can expect? I think Jacob might
> have some ideas on that.

It's documented as 256 bytes now. But that still leaves one remaining 
question: the idea is that the maximum size is long enough that 
middleware can add their data to it, and still include the original 
server's key. As the patch stands, the server creates 32 byte keys, 
leaving 224 for middleware. But that's not documented anywhere. I'm 
inclined to document that the server only uses up to 32 bytes, and 
middleware should likewise try to "add" only 32 bytes or so to the key, 
so that you can stack multiple such middleware without exceeding the 
total length. And perhaps 256 bytes is still too small, if you consider 
such stacking.

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