Re: Make query cancellation keys longer

On Fri, 8 Mar 2024 at 23:20, Heikki Linnakangas <hlinnaka(at)iki(dot)fi> wrote:
> Added some documentation. There's more work to be done there, but at
> least the message type descriptions are now up-to-date.

Thanks, that's very helpful.

> The nice thing about relying on the message length was that we could
> just redefine the CancelRequest message to have a variable length
> secret, and old CancelRequest with 4-byte secret was compatible with the
> new definition too. But it doesn't matter much, so I added an explicit
> length field.
>
> FWIW I don't think that restriction makes sense. Any code that parses
> the messages needs to have the message length available, and I don't
> think it helps with sanity checking that much. I think the documentation
> is a little anachronistic. The real reason that all the message types
> include enough information to find the message end is that in protocol
> version 2, there was no message length field. The only exception that
> doesn't have that property is CopyData, and it's no coincidence that it
> was added in protocol version 3.

Hmm, looking at the current code, I do agree that not introducing a
new message would simplify both client and server implementation. Now
clients need to do different things depending on if the server
supports 3.1 or 3.0 (sending CancelRequestExtended instead of
CancelRequest and having to parse BackendKeyData differently). And I
also agree that the extra length field doesn't add much in regards to
sanity checking (for the CancelRequest and BackendKeyData message
types at least). So, sorry for the back and forth on this, but I now
agree with you that we should not add the length field. I think one
reason I didn't see the benefit before was because the initial patch
0002 was still introducing a CancelRequestExtended message type. If we
can get rid of this message type by not adding a length, then I think
that's worth losing the sanity checking.

> Unfortunately 1234,5679 already in use by NEGOTIATE_SSL_CODE. That's why
> I went the other direction. If we want to add this to "the end", it
> needs to be 1234,5681. But I wanted to keep the cancel requests together.

Fair enough, I didn't realise that. This whole point is moot anyway if
we're not introducing CancelRequestExtended

> We could teach
> libpq to disconnect and reconnect with minor protocol version 3.0, if
> connecting with 3.1 fails, but IMHO it's better to not complicate this
> and accept the break in backwards-compatibility.

Yeah, implementing automatic reconnection seems a bit overkill to me
too. But it might be nice to add a connection option that causes libpq
to use protocol 3.0. Having to install an old libpq to connect to an
old server seems quite annoying. Especially since I think that many
other types of servers that implement the postgres protocol have not
implemented the minor version negotiation.

I at least know PgBouncer[1] and pgcat[2] have not, but probably other
server implementations like CockroachDB and Google Spanner have this
problem too.

I'll try to add such a fallback connection option to my patchset soon.

[1]: https://github.com/pgbouncer/pgbouncer/pull/1007
[2]: https://github.com/postgresml/pgcat/issues/706