Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs

On Mon, 9 Sept 2024 at 17:57, Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Mon, Aug 26, 2024 at 8:40 AM Robert Haas <robertmhaas@gmail.com> wrote:
> > On Fri, Aug 23, 2024 at 3:40 PM Jacob Champion
> > <jacob.champion@enterprisedb.com> wrote:
> > > Agreed on the name. I've attached a reconfigured version of v15-0003,
> > > with an extension that should hopefully not throw off the cfbot, and a
> > > commit message that should hopefully not misrepresent the discussion
> > > so far?
> >
> > LGTM. Objections?
>
> Hearing none, committed.

Sorry for the radio silence here, but I thought I'd pick this thread
up again. Attached is an updated version of the patchset.

1. Fixes a small, but important oversight in cdb6b0fdb0b
2. The fix for tracing logic, which I originally tried to get
committed in another thread[1], but Alvaro suggested I move it back
here.
3. Making pqGetNegotiateProtocolVersion3 less strict
4. Adds min_protocol_version and max_protocol_version. This patchset
did not have min_protocol_version before, but given that some new
protocol features might improve security it makes sense to allow
people to configure a lower limit.
5. Unchanged from previous patchset versions
6. Bumps the version and was changed to also introduce tests, which
add coverage for PQfullProtocolVersion and max_protocol_version
(min_protocol_version isn't testable unless connecting against an
older PG server)
7 & 8. Unchanged from previous versions

I think 1 & 2 should hopefully be no-brainers.

3 doesn't do anything too special imo, but maybe requires some
bikeshedding on the error messages or something.

4 is a user facing API, so probably some discussion is needed. In
another thread it was suggested to use a single connection option that
allowed version ranges[2]. I don't feel super strong either way, but I
prefer these two separate options. It matches well with the
ssl_min_protocol_version/ssl_max_protocol_version options that we
already have. It also makes sure we don't need to introduce the
complexity to allow for the parsing, documenting and handling of such
version ranges. E.g. if we would to support both > and >= in such
version ranges then, we'd need to store both a version number and an
operation. And finally having separate min and max options nudges
people to use them sensibly, because we don't want people to
accidentally disable support for protocol versions, and by having
protocol_version=3.2 mean ONLY 3.2 that's probably what will happen
because that looks much more natural than protocol_version=>=3.2

5 might be debatable if we want to do that.

6 It only makes sense to bump the protocol version in a PG release if
there's also some changes to the protocol. But I agree with Heikki
that it's problematic that not all poolers support sending the
ProtocolVersionNegotiation message. Having libpq request a new
protocol version by default is probably the best way of nudging those
maintainers to implement support for it. So I think we could even
consider merging this without any protocol changes. This all with the
intent of reverting the commit before the actual release, if we don't
also merge a protocol change. If we do this we'd also want to
temporarily set the default max_protocol_version to "latest", to
actually request the new protocol version.

7 & 8 should probably be ignored for now, because I think I haven't
addressed all previous feedback on those yet.

[1]: https://www.postgresql.org/message-id/202408160005.k6pafpzuz5dl%40alvherre.pgsql
[2]: https://www.postgresql.org/message-id/CA+Tgmoa2ScSSVoa1qzpN9rPFmWsb=SstgFR0eBFPy1udDFm5DQ@mail.gmail.com