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

On Fri, Aug 16, 2024 at 12:05 AM Jelte Fennema-Nio <postgres(at)jeltef(dot)nl> wrote:
>
> On Fri, 16 Aug 2024 at 00:39, Jacob Champion
> <jacob(dot)champion(at)enterprisedb(dot)com> wrote:
> >
> > On Thu, Aug 15, 2024 at 3:04 PM Heikki Linnakangas <hlinnaka(at)iki(dot)fi> wrote:
> > > Perhaps we should even change it to return
> > > 300000 for protocol version 3.0, and just leave a note in the docs like
> > > "in older versions of libpq, this returned 3 for protocol version 3.0".
> >
> > I think that would absolutely break current code. It's not uncommon
> > (IME) for hand-built clients wrapping libpq to make sure they're not
> > talking v2 before turning on some feature, and they're allowed to do
> > that with a PQprotocolVersion() == 3 check. A GitHub code search
> > brings up examples.
>
> Can you give a link for that code search and/or an example where
> someone used it like that in a real setting?

Keeping in mind that I'm responding to the change from 3 to 30000:

https://github.com/search?q=PQprotocolVersion&type=code
https://github.com/psycopg/psycopg2/blob/658afe4cd90d3e167d7c98d22824a8d6ec895b1c/tests/test_async.py#L89

Bindings re-export this symbol in ways that basically just expand the
web of things to talk about. And there's hazards like

https://github.com/infusion/PHP/blob/7ebefb6426bb4b4820a30cca5c3a10bfd757b6ea/ext/pgsql/pgsql.c#L864

where the old client is fine, but new clients could be tricked into
writing similar checks as `>= 30000` -- which is wrong because older
libpqs use 3, haha, surprise, have fun with that!

> The only example I could
> find where someone used it at all was psycopg having a unittest for
> their python wrapper around this API, and they indeed used == 3.

I've written code that uses exact equality as well, because I cared
about the wire protocol version. Even if I hadn't, isn't the first
public example enough, since a GitHub search is going to be an
undercount? What is your acceptable level of breakage?

People who are testing against this have some reason to care about the
underlying protocol compatibility. I don't need to understand (or even
agree with!) why they care; we've exported an API that allows them to
do something they find useful.

> The advantage is not introducing yet another API when we already have
> one with a great name

Sorry to move the goalposts, but when I say "value" I'm specifically
talking about value for clients/community, not value for patch writers
(or even maintainers). A change here doesn't add any value for
existing clients when compared to a new API, since they've already
written the version check code and are presumably happy with it. New
clients that have reason to care about the minor version, whatever
that happens to mean for a protocol, can use new code.

I'm not opposed to compatibility breaks when a client can see some
value in what we've done -- but the dev being broken should at least
be able to say something like "oh yeah, it was clearly broken before
and I'm glad it works now" or "wow, what a security hole, I'm glad
they patched it". That's not true here.

libpq is close to the base of a gigantic software stack and ecosystem.
We have an API, we have an SONAME, we have ways to introduce better
APIs while not breaking past clients. (And we can collect the list of
cruft to discard in a future SONAME bump, so we don't have to carry it
forever... but is the cost of this particularly large?)

> that no-one is currently using.

This is demonstrably false. You just cited the example you found in
psycopg, and all those bindings on GitHub have clients of their own,
not all of which are going to be easily searchable.

> The current API
> is in practice just a very convoluted way of writing 3.

There are versions of libpq still in support that can return 2, and
clients above us have to write code against the whole spectrum.

> Also doing an
> == 3 check is obviously problematic, if people use this function they
> should be using > 3 to be compatible with future versions.

Depends on why they're checking. I regularly write test clients that
drop down beneath the libpq layer. I don't want to be silently
upgraded.

I think I remember some production Go-based libpq clients several
years ago that did similar things, dropping down to the packet level
to do some sort of magic, but I can't remember exactly what now.
That's terrifying in the abstract, but it's not my decision or code to
maintain. The community is allowed to do things like that; we publish
a protocol definition in addition to an API that exposes the socket.

> So if we
> ever introduce protocol version 4, then these (afaict theoretical)
> users would break anyway.

Yes -- not theoretical, since I am one of them! -- that's the point.
Since we've already demonstrated that protocol details can leak up
above the API for the 2->3 change, a dev with reason to be paranoid
(such as myself) can write a canary for the 3->4 change. "Protocol 4.0
not yet supported" can be a feature.

--Jacob