Re: Add new protocol message to change GUCs for usage with future protocol-only GUCs - Mailing list pgsql-hackers

On Fri, 16 Aug 2024 at 19:44, Jacob Champion
<jacob.champion@enterprisedb.com> wrote:
> Keeping in mind that I'm responding to the change from 3 to 30000:

Let me start with the fact that I agree we **shouldn't** change 3 to
30000. And the proposed patch also specifically doesn't.

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

A **unittest** which is there just to add coverage for a method that
the driver exposes is not **actual usage** IMHO. Afaict Daniele (CCd
for confirmation) only added this code to add coverage for the API it
re-exposes. Considering this as an argument against returning
different values from this function is akin to saying that we should
avoid changing the function if we would have had coverage for this
function ourselves in our own libpq tests by checking for == 3.
Finally, this function in psycopg was only added in 2020. That's a
time when having this function return anything other than 3 when
connecting to a supported Postgres version was already not possible
for 10 years.

>     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!

This is indeed actual usage but, like any sensible production use, it
actually uses >= 3 so nothing would break even if we changed to using
30000. Rewording what you're saying to make it sound much less
terrible: Users of the **new** API who fail to read the docs, and thus
use >= 30000 instead of >=3, would then cause breakage when linking to
older libpq versions. This seems extremely unlikely for anyone to do.
Because if you are a new user of the API, then why on earth would you
check for 3.0 or larger. The last server that used a version lower
than 3.0 is 7.4 of which the final release was in 2010... So the only
reason to use PQprotocolVersion in new code would be to check for
versions higher than 3.0, for which the checks > 30000, and >= 30001
would both work! And finally this would only be the case if we change
the definition of  3.0 to 30000. Which as said above the proposed
patch specifically doesn't, to avoid such confusion.

> > 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.

So, if you cared about the exact wire protocol version for your own
usage. Then why wouldn't you want to know that the minor version
changed?

> 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?

As explained above. Any actual usage that is not a **unittest** of a
driver library.

> 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.

Yes, and assuming they only care about major version upgrades seems
very presumptuous. If they manually parse packets themselves or want
package traces to output the same data, then they'd want to pin to an
exact protocol version, both minor and major. Just because we'd never
had a minor version bump before doesn't mean users of this API don't
care about being notified by them through the existing
PQprotocolVersion API.

> > 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.

Not introducing new APIs definitely is useful to clients and the
community. Before users can use a new API, their libpq wrapper needs
to expose this new function that calls it through FFI. First of all
this requires work from client authors. But the **key point being**:
The new function would not be present in old libpq versions. So for
some clients, the FFI wrapper would also not exist for those old libpq
versions, or at least would fail. So now users before actually being
able to check for a minor protocol version, they first need an up to
date libpq wrapper library for their language that exposes the new
function, and then they'd still have to check their actual libpq
version, before they could finally check for the minor protocol
version...

Also as explained above, the few users that check for == 3 probably
**actually care** about the exact version. And would want to have that
check fail when the minor version changes.

> > 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.

Only when connecting to PG7.4 which ended support in 2010. So in
practice it has been returning only 3 for quite a while.

> > 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.

I feel like we're agreeing here, but somehow you end with a different
conclusion: People that care to check for the exact protocol version
also care about minor version bumps. In this Go client where someone
was doing weird packet level magic they cared about the **exact packet
layout**. Which is possible to change in minor version bumps. So by
not changing the return value of PQprotocolVersion, we'd be doing
exactly what you said you don't want: We'd upgrade them silently!