Re: Make query cancellation keys longer

On 16/08/2024 15:31, Robert Haas wrote:
> On Thu, Aug 15, 2024 at 6:07 PM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>> Ok, I've read through that thread now, and opined there too. One
>> difference is with libpq option name: My patch adds "protocol_version",
>> while Jelte proposes "max_protocol_version". I don't have strong
>> opinions on that. I hope the ecosystem catches up to support
>> NegotiateProtocolVersion quickly, so that only few people will need to
>> set this option. In particular, I hope that there will never be need to
>> use "max_protocol_version=3.2", because by the time we introduce version
>> 3.3, all the connection poolers that support 3.2 will also implement
>> NegotiateProtocolVersion.
> 
> In Jelte's design, there end up being two connection parameters. We
> tell the server we want max_protocol_version, but we accept that it
> might give us something older. If, however, it tries to degrade us to
> something lower than min_protocol_version, we bail out. I see you've
> gone for a simpler design: you ask the server for protocol_version and
> you get that or you die. To be honest, right up until exactly now, I
> was assuming we wanted a two-parameter system like that, just because
> being able to tolerate a range of protocol versions seems useful.
> However, maybe we don't need it. Alternatively, we could do this for
> now, and then later we could adjust the parameter so that you can say
> protocol_version=3.2-3.7 and the client will ask for 3.7 but tolerate
> anything >= 3.2. Hmm, I kind of like that idea.

Works for me.

If we envision accepting ranges like that in the future, it would be 
good to do now rather than later. Otherwise, if someone wants to require 
features from protocol 3.2 today, they will have to put 
"protocol_version=3.2" in the connection string, and later when 3.3 
version is released, their connection string will continue to force the 
then-old 3.2 version.

> I think it's likely that the ecosystem will catch up with
> NegotiateProtocolVersion once things start breaking. However, I feel
> pretty confident that there are going to be glitches. Clients are
> going to want to force newer protocol versions to make sure they get
> new features, or to make sure that security features that they want to
> have (like this one) are enabled. Some users are going to be running
> old poolers that can't handle 3.2, or there will be weirder things
> where the pooler says it supports it but it doesn't actually work
> properly in all cases. There are also non-PG servers that reimplement
> the PG wire protocol. I can't really enumerate all the things that go
> wrong, but I think there are a number of wire protocol changes that
> various people have been wanting for a long while now, and when we
> start to get the infrastructure in place to make that practical,
> people are going to take advantage of it. So I think we can expect a
> number of protocol enhancements and changes -- Peter's transparent
> column encryption stuff is another example -- and there will be
> mistakes by us and mistakes by others along the way. Allowing users to
> specify what protocol version they want is probably an important part
> of coping with that.

Yes, it's a good escape hatch to have.

> The documentation in the patch you attached still seems to think
> there's an explicit length field for the cancel key.

ok thanks

> Also, I think it
> would be good to split this into two patches, one to bump the protocol
> version and a second to change the cancel key stuff. It would
> facilitate review, and I also think that bumping the protocol version
> is a big enough deal that it should have its own entry in the commit
> log.

Right. That's what Jelte's first patches did too. Those changes are more 
or less the same between this patch and his. These clearly need to be 
merged into one "introduce protocol version 3.2" patch.

I'll split this patch like that, to make it easier to compare and merge 
with Jelte's corresponding patches.

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