On 16/08/2024 15:31, Robert Haas wrote:
On Thu, Aug 15, 2024 at 6:07 PM Heikki Linnakangas <hlinn...@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)
