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

[Robert Haas]

On Mon, Aug 26, 2024 at 8:40 AM Robert Haas  wrote:
> On Fri, Aug 23, 2024 at 3:40 PM Jacob Champion
>  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.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

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

[Robert Haas]

On Fri, Aug 23, 2024 at 3:40 PM Jacob Champion
 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?

-- 
Robert Haas
EDB: http://www.enterprisedb.com

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

[Jacob Champion]

On Fri, Aug 23, 2024 at 7:42 AM Jelte Fennema-Nio  wrote:
>
> On Fri, 23 Aug 2024 at 16:02, Robert Haas  wrote:
> > Personally, I'm 100% convinced at this point that we're arguing about
> > the wrong problem. Before, I didn't know for sure whether anyone would
> > be mad if we redefined PQprotocolVersion(), but now I know that there
> > is at least one person who will be, and that's Jacob.
>
> I could be interpreting Jacob his response incorrectly, but my
> understanding is that the type of protocol changes we would actually
> do in this version bump, would determine if he's either mad or happy
> that we redefined PQprotocolVersion.

Yes, but my conclusion is pretty much the same: let's talk about the
protocol changes. If we get to the end and revert the new API because
it's no longer adding anything -- e.g. because we've decided that
minor versions no longer have any compatibility guarantees at all --
so be it.

> > If there's one
> > among regular -hackers posters, there are probably more. Since Jelte
> > doesn't seem to want to produce the patch to add
> > PQminorProtocolVersion(), I suggest that somebody else does that --
> > Jacob, do you want to? -- and we commit that and move on.
>
> Let's call it PQfullProtocolVersion and make it return 30002. I'm fine
> with updating the patch. But I'll be unavailable for the next ~3
> weeks.

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?

Thanks,
--Jacob
From d4e4ccd2d08e2a6159521fa31d929b796c56383d Mon Sep 17 00:00:00 2001
From: Jacob Champion 
Date: Fri, 23 Aug 2024 11:48:55 -0700
Subject: [PATCH] libpq: surface the full protocol version to clients

PQprotocolVersion() does not include the minor version of the protocol.
In preparation for proposals that bump that number for the first time,
add PQfullProtocolVersion() to provide it to clients that may care,
using the (major * 1 + minor) construction already in use by
PQserverVersion().

The core of this patch and its documentation is based on work by Jelte
Fennema-Nio, but that doesn't mean he endorses how I've used it here.
There is still significant disagreement on the semantics of major vs
minor versions when it comes to the wire protocol.
---
 doc/src/sgml/libpq.sgml   | 38 ---
 src/include/libpq/pqcomm.h|  1 +
 src/interfaces/libpq/fe-connect.c | 10 
 src/interfaces/libpq/libpq-fe.h   |  5 
 4 files changed, 46 insertions(+), 8 deletions(-)

diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index f916fce414..eb8c232869 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -2678,22 +2678,44 @@ const char *PQparameterStatus(const PGconn *conn, const 
char *paramName);
  
 
 
+
+ 
PQfullProtocolVersionPQfullProtocolVersion
+
+ 
+  
+   Interrogates the frontend/backend protocol being used.
+
+int PQfullProtocolVersion(const PGconn *conn);
+
+   Applications might wish to use this function to determine whether 
certain
+   features are supported. The result is formed by multiplying the server's
+   major version number by 1 and adding the minor version number. For
+   example, version 3.2 would be returned as 30002, and version 4.0 would
+   be returned as 4. Zero is returned if the connection is bad. The 3.0
+   protocol is supported by PostgreSQL server
+   versions 7.4 and above.
+  
+  
+   The protocol version will not change after connection startup is
+   complete, but it could theoretically change during a connection reset.
+  
+ 
+
+
 
  
PQprotocolVersionPQprotocolVersion
 
  
   
-   Interrogates the frontend/backend protocol being used.
+   Interrogates the frontend/backend protocol major version.
 
 int PQprotocolVersion(const PGconn *conn);
 
-   Applications might wish to use this function to determine whether 
certain
-   features are supported.  Currently, the possible values are 3
-   (3.0 protocol), or zero (connection bad).  The protocol version will
-   not change after connection startup is complete, but it could
-   theoretically change during a connection reset.  The 3.0 protocol is
-   supported by PostgreSQL server versions 7.4
-   and above.
+   Unlike , this returns only
+   the major protocol version in use, but it is supported by a wider range
+   of libpq releases back to version 7.4. Currently, the possible values 
are
+   3 (3.0 protocol), or zero (connection bad). Prior to release version
+   14.0, libpq could additionally return 2 (2.0 protocol).
   
  
 
diff --git a/src/include/libpq/pqcomm.h b/src/include/libpq/pqcomm.h
index 527735e3db..6925f10602 100644
--- a/src/include/libpq/pqcomm.h
+++ b/src/include/libpq/pqcomm.h
@@ -86,6 +86,7 @@ is_unix

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

[Jelte Fennema-Nio]

On Fri, 23 Aug 2024 at 16:02, Robert Haas  wrote:
> Personally, I'm 100% convinced at this point that we're arguing about
> the wrong problem. Before, I didn't know for sure whether anyone would
> be mad if we redefined PQprotocolVersion(), but now I know that there
> is at least one person who will be, and that's Jacob.

I could be interpreting Jacob his response incorrectly, but my
understanding is that the type of protocol changes we would actually
do in this version bump, would determine if he's either mad or happy
that we redefined PQprotocolVersion.

> If there's one
> among regular -hackers posters, there are probably more. Since Jelte
> doesn't seem to want to produce the patch to add
> PQminorProtocolVersion(), I suggest that somebody else does that --
> Jacob, do you want to? -- and we commit that and move on.

Let's call it PQfullProtocolVersion and make it return 30002. I'm fine
with updating the patch. But I'll be unavailable for the next ~3
weeks.

> Then we can get down to the business of actually changing some stuff
> at the protocol level. IMHO, that's what should be scary and/or
> controversial here, and it's also imperative that if we're going to do
> it, we do it soon.

Agreed, but I don't think doing so is blocked on merging a
PQfullProtocolVersion libpq function.

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

[Robert Haas]

On Wed, Aug 21, 2024 at 3:14 PM Jacob Champion
 wrote:
> Put another way: we've seen that our protocol-version joint has rusted
> [1, 2]. I agree that needs to be fixed. But I also believe that we
> shouldn't try to smash the joint open with a hammer, and that belief
> seems philosophically at odds with the approach being taken upthread.

+1. We are inevitably going to break some things, especially if we
introduce changes that affect everyone, such as longer cancel keys,
rather than just optional features. But we owe it not only to our
rather large user base but also to ourselves to minimize the extent of
the breakage as much as possible. I have yet to experience the
situation where I commit something that angers users and my life
nevertheless improves afterward.

Personally, I'm 100% convinced at this point that we're arguing about
the wrong problem. Before, I didn't know for sure whether anyone would
be mad if we redefined PQprotocolVersion(), but now I know that there
is at least one person who will be, and that's Jacob. If there's one
among regular -hackers posters, there are probably more. Since Jelte
doesn't seem to want to produce the patch to add
PQminorProtocolVersion(), I suggest that somebody else does that --
Jacob, do you want to? -- and we commit that and move on.

Then we can get down to the business of actually changing some stuff
at the protocol level. IMHO, that's what should be scary and/or
controversial here, and it's also imperative that if we're going to do
it, we do it soon. If we make the mistake of dumping a bunch of
changes that break half of the ecosystem into the tree just before
feature freeze, there's no time for us to fix anything more than
trivial problems. If more serious problems turn up, it's a revert. If
we start to get some of these changes made now, there's a lot more
room for error. Let's take advantage of the time available while we
still have it.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

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

[Jacob Champion]

On Tue, Aug 20, 2024 at 3:17 PM Jelte Fennema-Nio  wrote:
>
> On Tue, 20 Aug 2024 at 23:48, Jacob Champion
>  wrote:
> > That guarantee (if adopted) would also make it possible for my use
> > case to proceed correctly, since a libpq client can still speak 3.0
> > packets on the socket safely.
>
> Not necessarily (at least not how I defined it). If a protocol
> parameter has been configured (possibly done by default by libpq),
> then that might not be the case anymore. So, you'd also need to
> compare the current values of the protocol parameters to their
> expected value.

With your definition, I agree. But I was about to sneakily suggest
(muahaha) that if you want to go that route, maybe protocol extensions
need to provide their own forward compatibility statements. Whether
via the same mechanism, or with something like criticality.

> > But in that case, PQprotocolVersion
> > should keep returning 3, because there's an explicit reason to care
> > about the major version by itself.
>
> I agree that there's a reason to care about the major version then,
> but it might still be better to fail hard for people that care about
> protocol details.

Maybe? In the span of a couple of days we've gone from "minor versions
are actually major versions and we will break all intermediaries all
the time" to "maybe not, actually". It's difficult for me to reason
through things that quickly. And on some level, that's fine and
expected, if we're still at the debate-and-design stage.

But personally I'd hoped that most of the conversation around
something this disruptive would be about what's going to break and
what's not, with the goal of making the broken set as small as
possible in exchange for specific benefits. Instead it seems like use
cases are having to justify themselves to avoid being broken, which is
not really the stance I want to see from a protocol maintainer.
Especially not if your stated goal is to bump versions whenever we
want (which, just for the record, I do not agree with).

Put another way: we've seen that our protocol-version joint has rusted
[1, 2]. I agree that needs to be fixed. But I also believe that we
shouldn't try to smash the joint open with a hammer, and that belief
seems philosophically at odds with the approach being taken upthread.
So if I'm the only one who feels this way, please someone let me know
so I can bow out instead of throwing up roadblocks... I don't want to
be a distraction from incremental protocol improvements.

--Jacob

[1] https://en.wikipedia.org/wiki/Protocol_ossification
[2] https://www.imperialviolet.org/2016/05/16/agility.html

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

[Jelte Fennema-Nio]

On Tue, 20 Aug 2024 at 23:48, Jacob Champion
 wrote:
> That guarantee (if adopted) would also make it possible for my use
> case to proceed correctly, since a libpq client can still speak 3.0
> packets on the socket safely.

Not necessarily (at least not how I defined it). If a protocol
parameter has been configured (possibly done by default by libpq),
then that might not be the case anymore. So, you'd also need to
compare the current values of the protocol parameters to their
expected value.

> But in that case, PQprotocolVersion
> should keep returning 3, because there's an explicit reason to care
> about the major version by itself.

I agree that there's a reason to care about the major version then,
but it might still be better to fail hard for people that care about
protocol details. Because when writing the code that checked
PQprotocolVersion there was no definition at all of what could change
during a minor version bump. So there was no possibility to reason for
that person if they should be notified of a minor version bump or not.
So notifying the author of the code by failing the check would be
erring on the safe side, because maybe they would need to update their
code that depends on the protocol details. If not, and they then
realize that our guarantee is strong enough for their use case, then
they could replace their check with something like:

PQprotocolVersion() >= 3 && PQprotocolVersion() < 4

To be clear, the argument for changing PQprotocolVersion() is
definitely less strong if we'd provide such a guarantee. But I don't
think the problem is completely gone either.

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

[Jacob Champion]

On Tue, Aug 20, 2024 at 12:55 PM Jelte Fennema-Nio  wrote:
> Another way of describing this guarantee: If a client connects using
> 3.8 and configures no protocol parameters, the client needs to handle
> anything 3.8 specific that the handshake requires (such as longer
> cancel token). But then after that handshake it can choose to send
> only 3.0 packets and expect to receive only 3.0 packets back.

That guarantee (if adopted) would also make it possible for my use
case to proceed correctly, since a libpq client can still speak 3.0
packets on the socket safely. But in that case, PQprotocolVersion
should keep returning 3, because there's an explicit reason to care
about the major version by itself.

--Jacob

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

[Jelte Fennema-Nio]

On Tue, 20 Aug 2024 at 19:31, Jacob Champion
 wrote:
> It's applicable to the use case I was talking about with Jelte. A
> libpq client dropping down to the socket level is relying on
> (implicit, currently undocumented/undecided, possibly incorrect!)
> intermediary guarantees that the protocol provides for a major
> version. I'm hoping we can provide some, since we haven't broken
> anything yet. If we decide we can't, then so be it -- things will
> break either way -- but it's still strange to me that we'd be okay
> with literally zero forward compatibility and still call that a "minor
> version".

I think one compatibility guarantee that we might want to uphold is
something like the following: After completing the initial connection
setup, a server should only send new message types or new fields on
existing message types when the client has specifically advertised
support for that message type in one of two ways:
1. By configuring a specific protocol parameter
2. By sending a new message type or using a new field that implicitly
advertises support for the new message type/fields. In this case the
message should be of a request-response style, the server cannot
assume that after the request-response communication happened this new
message type is still supported by the client.

The reasoning for this was discussed a while back upthread: This would
be to allow a connection pooler (like PgBouncer) to have a pool of the
highest minor version that the pooler supports e.g 3.8, but then it
could still hand out these connections to clients that connected to
the pooler using a lower version. Without having these clients receive
messages that they do not support.

Another way of describing this guarantee: If a client connects using
3.8 and configures no protocol parameters, the client needs to handle
anything 3.8 specific that the handshake requires (such as longer
cancel token). But then after that handshake it can choose to send
only 3.0 packets and expect to receive only 3.0 packets back.

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

[Jelte Fennema-Nio]

On Tue, 20 Aug 2024 at 19:02, David G. Johnston
 wrote:
> So basically my proposal amounted to making every update a "major version 
> update" and changing the behavior surrounding NegotiateProtocolVersion so it 
> applies to major version differences.  I'll stand by that change in 
> definition.  The current one doesn't seem all that useful anyway, and as we 
> only have a single version, definitely hasn't been materially implemented.  
> Otherwise, at some point a client that knows both v3 and v4 will exist and 
> its connection will be rejected instead of downgraded by a v3-only server 
> even though such a downgrade would be possible.  I suspect we'd go ahead and 
> change the rule then - so why not just do so now, while getting rid of the 
> idea that minor versions are a thing.

If we decide to never change the format of the StartupMessage again
(which may be an okay thing to decide). Then I agree it would make
sense to update the existing supported servers ASAP to be able to send
back a NegotiateProtocolVersion message if they receive a 4.x
StartupMessage, and the server only supports up to 3.x.

However, even if we do that, I don't think it makes sense to start
using the 4.0 version straight away. Because many older postgres
servers would still throw an error when receiving the 4.x request. By
using a 3.x version we are able to avoid those errors in the existing
ecosystem. Basically, I think we should probably wait ~5 years again
until we actually use a 4.0 version.

i.e. I don't see serious benefits to using 4.0.  The main benefit you
seem to describe is: "it's theoretically cleaner to use major version
bumps". And there is a serious downside: "seriously breaking the
existing ecosystem".

> I suppose we could leave minor versions for patch releases of the main server 
> version - which still leaves the first new feature of a release incrementing 
> the major version.  That would be incidental to changing how we handle major 
> versions.

Having a Postgres server patch update change the protocol version that
the server supports sounds pretty scary to me.

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

[David G. Johnston]

On Tue, Aug 20, 2024 at 10:46 AM Jacob Champion <
jacob.champ...@enterprisedb.com> wrote:

> On Tue, Aug 20, 2024 at 10:42 AM David G. Johnston
>  wrote:
> > Semantic versioning guidelines are not something we are following,
> especially here.
>
> I understand; the protocol is ours, and we'll do whatever we do in the
> end. I'm claiming that we can choose to provide semantics, and if we
> do, those semantics will help people who are not here on the list to
> defend their use cases.
>
>
I was mostly just responding to your surprise given that we have a
track-record here.  I agree that our existing effective policy isn't all
that well documented, namely as to when the major component might change,
and the fact that the minor component does not represent a "bug fix
release".

David J.

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

[Jacob Champion]

On Tue, Aug 20, 2024 at 10:42 AM David G. Johnston
 wrote:
> Semantic versioning guidelines are not something we are following, especially 
> here.

I understand; the protocol is ours, and we'll do whatever we do in the
end. I'm claiming that we can choose to provide semantics, and if we
do, those semantics will help people who are not here on the list to
defend their use cases.

--Jacob

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

[David G. Johnston]

On Tue, Aug 20, 2024 at 10:31 AM Jacob Champion <
jacob.champ...@enterprisedb.com> wrote:

>  If we decide we can't, then so be it -- things will
> break either way -- but it's still strange to me that we'd be okay
> with literally zero forward compatibility and still call that a "minor
> version".
>

Semantic versioning guidelines are not something we are following,
especially here.

Our protocol version is really just two-part; just like our server major
version used to be.  We just happen to have named both parts here, unlike
with the historical server major version.

We never have implemented a protocol change during a minor server version
update, it doesn't have (though maybe it needs?) a patch version part.

David J.

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

[Jacob Champion]

On Tue, Aug 20, 2024 at 8:24 AM Heikki Linnakangas  wrote:
> > Put another way: for a middlebox on the connection (which may be
> > passively observing, but also maybe actively adding new messages to
> > the stream), what is guaranteed to remain the same in the protocol
> > across a minor version bump? Hopefully the answer isn't "nothing"?
>
> I don't think we can give any future guarantees like that. If you have a
> middlebox on the connection, it needs to fully understand all the
> protocol versions it supports.

(GMail has catastrophically unthreaded this conversation for me, so
apologies if I start responding out of order)

Many protocols provide the list of assumptions that intermediates are
allowed to make within a single group of compatible versions, even as
the protocol gets extended. If we choose to provide those, then our
"major version" gains really useful semantics. See also the brief
"criticality" tangent upthread.

> That seems a bit tangential to the PQprotocolVersion() function though.
> A middlebox like that would probably not use libpq.

It's applicable to the use case I was talking about with Jelte. A
libpq client dropping down to the socket level is relying on
(implicit, currently undocumented/undecided, possibly incorrect!)
intermediary guarantees that the protocol provides for a major
version. I'm hoping we can provide some, since we haven't broken
anything yet. If we decide we can't, then so be it -- things will
break either way -- but it's still strange to me that we'd be okay
with literally zero forward compatibility and still call that a "minor
version".

--Jacob

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

[David G. Johnston]

On Tue, Aug 20, 2024 at 10:03 AM Jacob Champion <
jacob.champ...@enterprisedb.com> wrote:

> On Tue, Aug 20, 2024 at 7:26 AM Jelte Fennema-Nio 
> wrote:
> > In practical terms I think that means for a minor version bump the
> > format of the StartupMessage cannot be changed. Changing anything else
> > is fair game for a minor protocol version bump.
>
> I may be in a tiny minority here, but when I combine that statement
> with your opinion from way upthread that
>
> > IMHO, we
> > should get to a state where protocol minor version bumps are so
> > low-risk that we can do them whenever we add message types
>
> To me it seems that what you're proposing is indistinguishable from
> what most other protocols would consider a major version bump; it's
> just that you (reasonably) want existing clients to be able to
> negotiate multiple major versions in one round trip.
>
>
This makes more sense to me - a major version change is one where the
server fails to understand the incoming message(s) to the point that it
cannot make decisions based upon contents.

Framed up this way the two-part versioning works just fine and I concur
that PQversionNumber should go ahead and report 1+minor (starting at 2)
with 3.0 remaining as-is since apparently negotiation down to 3.0 is
possible here if the intermediate and/or final server have such ability.

Still, instead of just failing immediately if 30002 is specified and
rejected, falling back to trying 3.0 - unless configured to either not do
that or to only do 3.0 - is advised to help with the transition.

David J.

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

[Robert Haas]

On Tue, Aug 20, 2024 at 1:02 PM David G. Johnston
 wrote:
> So basically my proposal amounted to making every update a "major version 
> update" and changing the behavior surrounding NegotiateProtocolVersion so it 
> applies to major version differences.  I'll stand by that change in 
> definition.  The current one doesn't seem all that useful anyway, and as we 
> only have a single version, definitely hasn't been materially implemented.  
> Otherwise, at some point a client that knows both v3 and v4 will exist and 
> its connection will be rejected instead of downgraded by a v3-only server 
> even though such a downgrade would be possible.  I suspect we'd go ahead and 
> change the rule then - so why not just do so now, while getting rid of the 
> idea that minor versions are a thing.
>
> I suppose we could leave minor versions for patch releases of the main server 
> version - which still leaves the first new feature of a release incrementing 
> the major version.  That would be incidental to changing how we handle major 
> versions.

I don't see how this makes life any better for anyone. At some point
in the future we may decide to make a protocol change that is big and
breaks a lot of stuff, but the current goals are all to make minor
changes that break as little stuff as possible. I think it's
appropriate to call the latter a "minor" change and the former a
"major" change. If we adopted this proposal, then we could end up in a
situation where versions 3 through 17 are all mostly compatible and
then version 18 is something totally different. It sounds much better
to me to have versions 3.0 through 3.14 and then eventually 4.0. This
is also what the person who designed the current protocol version
numbering scheme seems to have had in mind, even if the implementation
to make it a reality has been a bit lacking.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

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

[Jacob Champion]

On Tue, Aug 20, 2024 at 7:26 AM Jelte Fennema-Nio  wrote:
> In practical terms I think that means for a minor version bump the
> format of the StartupMessage cannot be changed. Changing anything else
> is fair game for a minor protocol version bump.

I may be in a tiny minority here, but when I combine that statement
with your opinion from way upthread that

> IMHO, we
> should get to a state where protocol minor version bumps are so
> low-risk that we can do them whenever we add message types

then I don't see this effort ending up in a healthy place or with a
happy ecosystem. Pick any IETF-managed protocol, add on the statement
"we get to change anything we want in a minor version, and we reserve
the right to do it every single year", and imagine the chaos for
anyone who doesn't have power over both servers and clients.

To me it seems that what you're proposing is indistinguishable from
what most other protocols would consider a major version bump; it's
just that you (reasonably) want existing clients to be able to
negotiate multiple major versions in one round trip.

--Jacob

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

[David G. Johnston]

On Tue, Aug 20, 2024 at 9:44 AM Robert Haas  wrote:

> On Tue, Aug 20, 2024 at 12:42 PM David G. Johnston
>  wrote:
> > I'm wondering why we are indicating that minor versions of the protocol
> are even a real thing.
>
> Because that concept is already a part of the existing wire protocol.
>
>
Right...

"
If the major version requested by the client is not supported by the
server, the connection will be rejected ... If the minor version requested
by the client is not supported by the server ... the server may either
reject the connection or may respond with a NegotiateProtocolVersion
message containing the highest minor protocol version which it supports.
The client may then choose either to continue with the connection using the
specified protocol version or to abort the connection.
"

So basically my proposal amounted to making every update a "major version
update" and changing the behavior surrounding NegotiateProtocolVersion so
it applies to major version differences.  I'll stand by that change in
definition.  The current one doesn't seem all that useful anyway, and as we
only have a single version, definitely hasn't been materially implemented.
Otherwise, at some point a client that knows both v3 and v4 will exist and
its connection will be rejected instead of downgraded by a v3-only server
even though such a downgrade would be possible.  I suspect we'd go ahead
and change the rule then - so why not just do so now, while getting rid of
the idea that minor versions are a thing.

I suppose we could leave minor versions for patch releases of the main
server version - which still leaves the first new feature of a release
incrementing the major version.  That would be incidental to changing how
we handle major versions.

David J.

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

[Jelte Fennema-Nio]

On Tue, 20 Aug 2024 at 18:42, David G. Johnston
 wrote:
> v18 libpq-based clients, if they attempt to connect using v4 and fail, will 
> try again using the v3 connection.  That will retain status quo behavior when 
> something like a connection pooler doesn't understand the new reality.

Having connection latency double when connecting to an older Postgres
is something I'd very much like to avoid. Reconnecting functionally
retains the status quo, but it doesn't retain the expected perf
characteristics. By using a minor protocol version we can easily avoid
this connection latency issue.

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

[Robert Haas]

On Tue, Aug 20, 2024 at 12:42 PM David G. Johnston
 wrote:
> I'm wondering why we are indicating that minor versions of the protocol are 
> even a real thing.

Because that concept is already a part of the existing wire protocol.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

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

[David G. Johnston]

On Tue, Aug 20, 2024 at 9:02 AM Robert Haas  wrote:

> Yes. And the major * 1 + minor convention is used in other places
> already, for PG versions, so it might already be familiar to some
> people.
>

I'm wondering why we are indicating that minor versions of the protocol are
even a real thing.  We should just use integer version numbers.  We are on
3. The next one is 4 (the trailing .0 is just historical cruft just like
with our 3-digit PostgreSQL version number).

v18 libpq-based clients, if they attempt to connect using v4 and fail, will
try again using the v3 connection.  That will retain status quo behavior
when something like a connection pooler doesn't understand the new
reality.  We can add a libpq option to prevent this auto-downgrade behavior.

At some point users will want to use something other than the v3 current
tooling supports and will put pressure on those tools to change.  In the
mean-time our out-of-the-box behavior continues to work using the v3
protocol.

Feature detection sounds great, and maybe we want to go there eventually,
but everyone understands progressive enhancement represented by version
numbering.  A given major server version would only support a fixed and
unchanging set of protocol versions between 3 and N.  On the client, if N =
7 then libpq would be able to choose both 7 and 3 as the version it tries
out-of-the-box.  We can use a libpq parameter to allow the client to
specify something between 4 and 6 (which may fail depending on poolers and
what-not).  If the chain of servers supports protocol version negotiation
then the attempt to connect using 7 can be auto-downgraded to anything
between 3 and 6 (saving effort of a failed attempt and establishing a new
one.)  Leaving the client the option to specify a minimum version of the
protocol it can accept.

David J.

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

[Robert Haas]

On Tue, Aug 20, 2024 at 11:53 AM Jelte Fennema-Nio  wrote:
> On Tue, 20 Aug 2024 at 17:46, Robert Haas  wrote:
> > I personally like this less than both (a) adding a new function and
> > (b) redefining the existing function as Jelte proposes. It just seems
> > too clever to me.
>
> Agreed, I'm not really seeing a benefit of returning 4 instead of
> 30004. Both are new numbers that are higher than 3, so on existing
> code they would have the same impact. But any new code would be more
> readable when using version >= 30004 imho.

Yes. And the major * 1 + minor convention is used in other places
already, for PG versions, so it might already be familiar to some
people. I think if we're going to redefine an existing function, we
might as well just redefine it as you propose -- or perhaps even
redefine it to return major * 1 + minor always, instead of having
the strange exception for 3.0. I think I'm still on the side of not
redefining it, but if we're going to redefine it, I think we should do
what seems most elegant/logical and just accept that some code may
break.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

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

[Jelte Fennema-Nio]

On Tue, 20 Aug 2024 at 17:46, Robert Haas  wrote:
> I personally like this less than both (a) adding a new function and
> (b) redefining the existing function as Jelte proposes. It just seems
> too clever to me.

Agreed, I'm not really seeing a benefit of returning 4 instead of
30004. Both are new numbers that are higher than 3, so on existing
code they would have the same impact. But any new code would be more
readable when using version >= 30004 imho.

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

[Robert Haas]

On Tue, Aug 20, 2024 at 11:24 AM Heikki Linnakangas  wrote:
> That's not a completely crazy idea, it crossed my mind too. And since we
> already decided to skip protocol number 3.1, how about we jump directly
> to 3.4. That way:
>
> protocol |
>   version | PQProtocolVersion()
>
> 2 | 2   (in old unsupported library versions)
>   3.0 | 3
>   3.4 | 4
>   3.5 | 5
>
> and so forth.
>
> This kind of assumes we'll never bump the major protocol version again.
> But if we do, we could jump to 4 at that point.

I personally like this less than both (a) adding a new function and
(b) redefining the existing function as Jelte proposes. It just seems
too clever to me.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

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

[Heikki Linnakangas]

On 20/08/2024 16:48, Jacob Champion wrote:

On Mon, Aug 19, 2024 at 1:54 PM Jelte Fennema-Nio  wrote:

My point is that the code that breaks, actually wants to be broken in this case.


I'll turn this around then and assume for a moment that this is true:
no matter what the use cases are, they all want to be broken for
correctness. If this version change is allowed to break both the
endpoints and any intermediaries on the connection, why have we chosen
30001 as the new reported version as opposed to, say, 4?


That's not a completely crazy idea, it crossed my mind too. And since we 
already decided to skip protocol number 3.1, how about we jump directly 
to 3.4. That way:


protocol |
 version | PQProtocolVersion()

   2 | 2   (in old unsupported library versions)
 3.0 | 3
 3.4 | 4
 3.5 | 5

and so forth.

This kind of assumes we'll never bump the major protocol version again. 
But if we do, we could jump to 4 at that point.



Put another way: for a middlebox on the connection (which may be
passively observing, but also maybe actively adding new messages to
the stream), what is guaranteed to remain the same in the protocol
across a minor version bump? Hopefully the answer isn't "nothing"?


I don't think we can give any future guarantees like that. If you have a 
middlebox on the connection, it needs to fully understand all the 
protocol versions it supports. It cannot safely pass through protocol 
version 3.5 without knowing what changed between 3.4 and 3.5. If the 
middlebox only knows about protocol version 3.4, it should respond with 
a NegotiateProtocolVersion packet to downgrade to 3.4, even if both ends 
of the connection could speak 3.5.


That seems a bit tangential to the PQprotocolVersion() function though. 
A middlebox like that would probably not use libpq.


I'm actually not sure exactly what an application would use 
PQprotocolVersion() for. To check if a feature exists or not? None of 
the features discussed so far really need an application to check that, 
but if we introduce one, I think we'd want to add a better feature-check 
function for that purpose. Something like "bool PQsupportsFeature(conn, 
const char *feature_name)" perhaps. If we introduce optional protocol 
features rather than bump protocol version in the future, we'll need a 
different mechanism than PQprotocolVersion() anyway.


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

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

[Jelte Fennema-Nio]

On Tue, 20 Aug 2024 at 15:48, Jacob Champion
 wrote:
> Put another way: for a middlebox on the connection (which may be
> passively observing, but also maybe actively adding new messages to
> the stream), what is guaranteed to remain the same in the protocol
> across a minor version bump? Hopefully the answer isn't "nothing"?

I think primarily we do a minor version bump because a major version
bump would cause existing Postgres servers to throw an error for the
connection attempt (and we don't want that). While for a minor version
bump they will instead send a NegotiateProtocolVersion message.

In practical terms I think that means for a minor version bump the
format of the StartupMessage cannot be changed. Changing anything else
is fair game for a minor protocol version bump. I think we probably
would not want to change the format of ErrorResponse and
NoticeResponse, since those can be sent by the server before the
NegotiateProtocolVersion message. But I don't even think that is
strictly necessary, as long as clients would be able to parse both the
old and new versions.

Note that this definition arises from code and behaviour introduced in
ae65f6066dc3 in 2017. And PQprotocolVersion was introduced in
efc3a25bb0 in 2003. So anyone starting to use the PQprotocolVersion
function in between 2003 and 2017 had no way of knowing that there
would ever be a thing called a "minor" version, in which anything
about the protocol could be changed except for the StartupMessage.

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

[Jacob Champion]

On Mon, Aug 19, 2024 at 1:54 PM Jelte Fennema-Nio  wrote:
> My point is that the code that breaks, actually wants to be broken in this 
> case.

I'll turn this around then and assume for a moment that this is true:
no matter what the use cases are, they all want to be broken for
correctness. If this version change is allowed to break both the
endpoints and any intermediaries on the connection, why have we chosen
30001 as the new reported version as opposed to, say, 4?

Put another way: for a middlebox on the connection (which may be
passively observing, but also maybe actively adding new messages to
the stream), what is guaranteed to remain the same in the protocol
across a minor version bump? Hopefully the answer isn't "nothing"?

--Jacob

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

[Jelte Fennema-Nio]

On Mon, 19 Aug 2024 at 16:16, Robert Haas  wrote:
> If somebody is using PQprotocolVersion() to detect the arrival of a
> new protocol version, it stands to reason that they only care about
> new major protocol versions, because that's what the function is
> defined to tell you about. Anyone who has done a moderate amount of
> looking into this area will understand that the protocol has a major
> version number and a minor version number and that this function only
> returns the former. Therefore, they should expect that the arrival of
> a new minor protocol version won't change the return value of this
> function.

What I'm trying to say is: I don't think there's any usecase where
people would care about a major bump, but not a minor bump. Especially
keeping in mind that a minor bump had never occurred when originally
creating this function. And because we never did it, there has so far
been no definition of what is the actual difference between a major
and a minor bump.

> I really don't understand why we're still arguing about this. It seems
> to me that we've established that there is some usage of the existing
> function, and that changing the return value will break something.
> Sure, so far as we know that something is "only" regression tests, but
> there's no guarantee that there couldn't be other code that we don't
> know about that breaks worse

My point is that the code that breaks, actually wants to be broken in this case.

> and even there isn't, who wants to break
> regression tests when there's nothing actually wrong?

Updating the regression test would be less work than adding support
for a new API. So if the main problem is

> Now we could
> decide we're going to do it anyway because of whatever reason we might
> have, but it doesn't seem like that's what most people want to do.
>
> I feel like we're finally in a position to get some things done here
> and this doesn't seem like the point to get stuck on. YMMV, of course.

I'd love to hear a response from Jacob and Heikki on my arguments
after their last response. But if after reading those arguments they
still think we should add a new function, I'll update the patchset to
include a new function.

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

[Robert Haas]

On Mon, Aug 19, 2024 at 3:30 AM Jelte Fennema-Nio  wrote:
> But **now I actually feel much more strongly about reusing the same
> function**. Because by introducing a new function we actually break
> the users of the second use-case.
>
> P.S. The docs for PQprotocolVersion[1] have never said that this
> function only returns the major protocol version. And by using the
> word "Currently" it has always suggested that new return values could
> be introduced later, and thus for feature detection you should use >=
> 3

If somebody is using PQprotocolVersion() to detect the arrival of a
new protocol version, it stands to reason that they only care about
new major protocol versions, because that's what the function is
defined to tell you about. Anyone who has done a moderate amount of
looking into this area will understand that the protocol has a major
version number and a minor version number and that this function only
returns the former. Therefore, they should expect that the arrival of
a new minor protocol version won't change the return value of this
function.

I really don't understand why we're still arguing about this. It seems
to me that we've established that there is some usage of the existing
function, and that changing the return value will break something.
Sure, so far as we know that something is "only" regression tests, but
there's no guarantee that there couldn't be other code that we don't
know about that breaks worse, and even there isn't, who wants to break
regression tests when there's nothing actually wrong? Now we could
decide we're going to do it anyway because of whatever reason we might
have, but it doesn't seem like that's what most people want to do.

I feel like we're finally in a position to get some things done here
and this doesn't seem like the point to get stuck on. YMMV, of course.

--
Robert Haas
EDB: http://www.enterprisedb.com

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

[Jelte Fennema-Nio]

On Mon, 19 Aug 2024 at 05:44, Robert Haas  wrote:
> I feel like what you're really complaining about here is that libpq is
> not properly versioned. We've just been calling it libpq.so.5 forever
> instead of bumping the version number when we change stuff.

There is PQlibVersion() that can be used for this. Which has existed
since 2010, so people can assume it exists.

> I just don't see why this particular change is special.

I didn't mean to say that it was, and I don't think the problem is
enormous either. I mainly meant to say that there is not just a cost
to Postgres maintainers when we introduce a new API. There's
definitely a cost to users and client authors too.

> Also, I kind of wish you had brought this argument up earlier. Maybe
> you did and I missed it, but I was under the impression that you were
> just arguing that "nobody will notice or care," which is a quite
> different argument than what you make here.

"nobody will notice or care" was definitely my argument before Jacob
responded. Since Jacob his response I realize there are two valid use
cases for PQprotocolVersion():

1. Feature detection. For this my argument still is: people won't
notice. Many people won't have bothered to use the function and
everyone else will have used >= 3 here.
2. Pinning the protocol version, because they care that the exact
protocol details are the same. Here people will have used == 3, and
thus their check will fail when we start to return a different version
from PQprotocolVersion(). But that's actually what this usecase
desires. By creating a new function, we actually break the expectation
of these people: i.e. they want the PQprotocolVersion() to return a
different version when the protocol changes.

Before Jacob responded I only considered the first case. So my
argument was indeed basically: Let's reuse this currently useless
function with the nice name, because no-one will care. But if people
thought that the risk was too high, I didn't see huge downsides to
introducing a new API either.

But **now I actually feel much more strongly about reusing the same
function**. Because by introducing a new function we actually break
the users of the second use-case.

P.S. The docs for PQprotocolVersion[1] have never said that this
function only returns the major protocol version. And by using the
word "Currently" it has always suggested that new return values could
be introduced later, and thus for feature detection you should use >=
3

[1]: 
https://www.postgresql.org/docs/13/libpq-status.html#LIBPQ-PQPROTOCOLVERSION

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

[Tom Lane]

Robert Haas  writes:
> I feel like what you're really complaining about here is that libpq is
> not properly versioned. We've just been calling it libpq.so.5 forever
> instead of bumping the version number when we change stuff. Maybe we
> should start doing that, because that's exactly what version numbers
> are for. Alternatively or in addition, maybe we should have a function
> in libpq that returns its own PostgreSQL version, because that would
> solve this problem for all cases, whereas what you're proposing here
> only solves it for this particular case (and at the risk of breaking
> things for somebody).

Not really.  *No* runtime test is adequate for discovery of a new
library API, because if you try to call a function that doesn't exist
in the version you have, you will get a compile or link failure long
before you can call any inquiry function.

Bumping the .so's major version just creates another way to fail
at link time, so I'm not seeing how that would make this better.

> I just don't see why this particular change is special. We add new
> libpq interfaces all the time and we don't do anything to make that
> easy for libpq clients to discover.

Indeed.  But we have actually paid a little bit of attention to that,
in the form of inventing #define symbols that can be tested at compile
time.  (There's an open item for 17 concerning failure to do that for
some new-in-17 APIs.)  Yeah, it's grotty, but runtime checks aren't
especially relevant here.

In any case, please let us not abuse the wire protocol version number
as an indicator of the libpq-to-application API version.  They are
fundamentally different things.

regards, tom lane

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

[Robert Haas]

On Sat, Aug 17, 2024 at 5:32 AM Jelte Fennema-Nio  wrote:
> 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.

Sure, just like they do every other new libpq function.

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

I feel like what you're really complaining about here is that libpq is
not properly versioned. We've just been calling it libpq.so.5 forever
instead of bumping the version number when we change stuff. Maybe we
should start doing that, because that's exactly what version numbers
are for. Alternatively or in addition, maybe we should have a function
in libpq that returns its own PostgreSQL version, because that would
solve this problem for all cases, whereas what you're proposing here
only solves it for this particular case (and at the risk of breaking
things for somebody).

I just don't see why this particular change is special. We add new
libpq interfaces all the time and we don't do anything to make that
easy for libpq clients to discover. If we implemented longer cancel
keys or protocol parameters or transparent column encryption without a
protocol version bump, clients would still need to figure out that
those features were present in the libpq they are linked against, just
like they presumably already need to worry about whether they're
linked a new-enough libpq to have any other feature that's been added
since forever ago. Sure, that's not great, but it doesn't seem any
more not great in this case than any other, and I'd rather see us come
up with a nice general solution to that problem than hack this
specific case by redefining an existing function.

Also, I kind of wish you had brought this argument up earlier. Maybe
you did and I missed it, but I was under the impression that you were
just arguing that "nobody will notice or care," which is a quite
different argument than what you make here.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

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

[Jelte Fennema-Nio]

On Fri, 16 Aug 2024 at 19:44, Jacob Champion
 wrote:
> Keeping in mind that I'm responding to the change from 3 to 3:

Let me start with the fact that I agree we **shouldn't** change 3 to
3. 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 `>= 3` -- 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
3. 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 >= 3 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 > 3, and >= 30001
would both work! And finally this would only be the case if we change
the definition of  3.0 to 3. 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 final

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

[Robert Haas]

On Fri, Aug 16, 2024 at 4:03 PM Dave Cramer  wrote:
> Admittedly I'm a bit late into this discussion so I may be off base.
> Ultimately we need to negotiate the protocol. From what I can tell for libpq 
> we are providing a function that returns a number, currently 3.
>
> The proposal is to change it to something like 3.
>
> Ultimately this has to go over the wire so that clients that are implementing 
> the protocol themselves can respond to the new behaviour.
>
> Wouldn't we have to send this number in the protocol negotiation ?

See the discussion of the NegotiateProtocolVersion message which has
been around for a long time but is still not supported by all clients.

https://www.postgresql.org/docs/current/protocol.html
https://www.postgresql.org/docs/current/protocol-message-formats.html

No changes to the format of that message are proposed. The startup
message also contains a version number, and changes the format of that
message are also not proposed.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

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

[Dave Cramer]

On Fri, 16 Aug 2024 at 15:54, Heikki Linnakangas  wrote:

> On 16/08/2024 22:45, Dave Cramer wrote:
> > On Fri, 16 Aug 2024 at 15:26, Heikki Linnakangas  > > wrote:
> >
> > On 16/08/2024 21:01, Robert Haas wrote:
> >  > On Fri, Aug 16, 2024 at 1:44 PM Jacob Champion
> >  >  > > wrote:
> >  >>
> >
> https://github.com/psycopg/psycopg2/blob/658afe4cd90d3e167d7c98d22824a8d6ec895b1c/tests/test_async.py#L89
> <
> https://github.com/psycopg/psycopg2/blob/658afe4cd90d3e167d7c98d22824a8d6ec895b1c/tests/test_async.py#L89
> >
> >  >>
> >
> https://github.com/infusion/PHP/blob/7ebefb6426bb4b4820a30cca5c3a10bfd757b6ea/ext/pgsql/pgsql.c#L864
> <
> https://github.com/infusion/PHP/blob/7ebefb6426bb4b4820a30cca5c3a10bfd757b6ea/ext/pgsql/pgsql.c#L864
> >
> >  >
> >  > IMHO these examples establish beyond doubt that the existing
> function
> >  > really is being used in ways that would break if we committed the
> >  > proposed patch. To be honest, I'm slightly surprised, because
> > protocol
> >  > version 2 has been so dead for so long that I would not have
> >  > anticipated people would even bother checking for it. But these
> >  > examples show that some people do. If Jacob found these examples
> this
> >  > easily, there are probably a bunch of others.
> >  >
> >  > It's not worth breaking existing code to avoid adding one new
> libpq
> >  > entrypoint. Let's just add the new function and move on.
> >
> > +1. Jacob is right.
> >
> >
> > For those of us who don't use a function. How will this work ?
>
> Sorry, I don't understand the question. This sub-thread is all about the
> libpq PQprotocolVersion() function.
>

Admittedly I'm a bit late into this discussion so I may be off base.
Ultimately we need to negotiate the protocol. From what I can tell for
libpq we are providing a function that returns a number, currently 3.

The proposal is to change it to something like 3.

Ultimately this has to go over the wire so that clients that are
implementing the protocol themselves can respond to the new behaviour.

Wouldn't we have to send this number in the protocol negotiation ?

Dave

>
>

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

[Heikki Linnakangas]

On 16/08/2024 22:45, Dave Cramer wrote:
On Fri, 16 Aug 2024 at 15:26, Heikki Linnakangas > wrote:


On 16/08/2024 21:01, Robert Haas wrote:
 > On Fri, Aug 16, 2024 at 1:44 PM Jacob Champion
 > mailto:jacob.champ...@enterprisedb.com>> wrote:
 >>

https://github.com/psycopg/psycopg2/blob/658afe4cd90d3e167d7c98d22824a8d6ec895b1c/tests/test_async.py#L89
 

 >>

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

 >
 > IMHO these examples establish beyond doubt that the existing function
 > really is being used in ways that would break if we committed the
 > proposed patch. To be honest, I'm slightly surprised, because
protocol
 > version 2 has been so dead for so long that I would not have
 > anticipated people would even bother checking for it. But these
 > examples show that some people do. If Jacob found these examples this
 > easily, there are probably a bunch of others.
 >
 > It's not worth breaking existing code to avoid adding one new libpq
 > entrypoint. Let's just add the new function and move on.

+1. Jacob is right.


For those of us who don't use a function. How will this work ?


Sorry, I don't understand the question. This sub-thread is all about the 
libpq PQprotocolVersion() function.


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

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

[Dave Cramer]

On Fri, 16 Aug 2024 at 15:26, Heikki Linnakangas  wrote:

> On 16/08/2024 21:01, Robert Haas wrote:
> > On Fri, Aug 16, 2024 at 1:44 PM Jacob Champion
> >  wrote:
> >>
> https://github.com/psycopg/psycopg2/blob/658afe4cd90d3e167d7c98d22824a8d6ec895b1c/tests/test_async.py#L89
> >>
> https://github.com/infusion/PHP/blob/7ebefb6426bb4b4820a30cca5c3a10bfd757b6ea/ext/pgsql/pgsql.c#L864
> >
> > IMHO these examples establish beyond doubt that the existing function
> > really is being used in ways that would break if we committed the
> > proposed patch. To be honest, I'm slightly surprised, because protocol
> > version 2 has been so dead for so long that I would not have
> > anticipated people would even bother checking for it. But these
> > examples show that some people do. If Jacob found these examples this
> > easily, there are probably a bunch of others.
> >
> > It's not worth breaking existing code to avoid adding one new libpq
> > entrypoint. Let's just add the new function and move on.
>
> +1. Jacob is right.
>

For those of us who don't use a function. How will this work ?

Dave

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

[Heikki Linnakangas]

On 16/08/2024 21:01, Robert Haas wrote:

On Fri, Aug 16, 2024 at 1:44 PM Jacob Champion
 wrote:

 
https://github.com/psycopg/psycopg2/blob/658afe4cd90d3e167d7c98d22824a8d6ec895b1c/tests/test_async.py#L89
 
https://github.com/infusion/PHP/blob/7ebefb6426bb4b4820a30cca5c3a10bfd757b6ea/ext/pgsql/pgsql.c#L864


IMHO these examples establish beyond doubt that the existing function
really is being used in ways that would break if we committed the
proposed patch. To be honest, I'm slightly surprised, because protocol
version 2 has been so dead for so long that I would not have
anticipated people would even bother checking for it. But these
examples show that some people do. If Jacob found these examples this
easily, there are probably a bunch of others.

It's not worth breaking existing code to avoid adding one new libpq
entrypoint. Let's just add the new function and move on.


+1. Jacob is right.

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

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

[Robert Haas]

On Fri, Aug 16, 2024 at 1:44 PM Jacob Champion
 wrote:
> 
> https://github.com/psycopg/psycopg2/blob/658afe4cd90d3e167d7c98d22824a8d6ec895b1c/tests/test_async.py#L89
> 
> https://github.com/infusion/PHP/blob/7ebefb6426bb4b4820a30cca5c3a10bfd757b6ea/ext/pgsql/pgsql.c#L864

IMHO these examples establish beyond doubt that the existing function
really is being used in ways that would break if we committed the
proposed patch. To be honest, I'm slightly surprised, because protocol
version 2 has been so dead for so long that I would not have
anticipated people would even bother checking for it. But these
examples show that some people do. If Jacob found these examples this
easily, there are probably a bunch of others.

It's not worth breaking existing code to avoid adding one new libpq
entrypoint. Let's just add the new function and move on.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

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

[Jacob Champion]

On Fri, Aug 16, 2024 at 12:05 AM Jelte Fennema-Nio  wrote:
>
> On Fri, 16 Aug 2024 at 00:39, Jacob Champion
>  wrote:
> >
> > On Thu, Aug 15, 2024 at 3:04 PM Heikki Linnakangas  wrote:
> > > Perhaps we should even change it to return
> > > 30 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 3:

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 `>= 3` -- 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

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

[Robert Haas]

On Fri, Aug 16, 2024 at 10:51 AM Heikki Linnakangas  wrote:
> Now, I think we should still do it, but it might not warrant changing
> the default. Unfortunately that means that it will get very little
> adoption. It will only be adopted as a side-effect of some other changes
> that make people change the protocol_version setting. Or after a very
> long transition period.
>
> That said, I think we *should* change the default for the time being, so
> that developers working on the bleeding edge and building from git get
> some exposure to it. Hopefully that will nudge some of the poolers to
> adopt NegotiateProtocolVersion sooner. But we revert the default to 3.0
> before the v18 release.

I'm fine with changing the default for the time being. I'm not really
sure what I think about changing it back before release. Maybe six
months from now it will be clearer what the right thing to do is; or
maybe other people will be more certain of what The Right Thing To Do
is than I am myself; but there's no rush to finalize a decision right
this minute.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

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

[Jelte Fennema-Nio]

On Fri, 16 Aug 2024 at 16:51, Heikki Linnakangas  wrote:
> That said, I think we *should* change the default for the time being, so
> that developers working on the bleeding edge and building from git get
> some exposure to it. Hopefully that will nudge some of the poolers to
> adopt NegotiateProtocolVersion sooner. But we revert the default to 3.0
> before the v18 release.

That seems reasonable to me. To be clear, the latest PgBouncer release
now supports NegotiateProtocolVersion.

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

[Heikki Linnakangas]

On 16/08/2024 15:55, Robert Haas wrote:

On Thu, Aug 15, 2024 at 6:03 PM Heikki Linnakangas  wrote:

On the default for "max_protocol_version": I'm pretty disappointed if we
cannot change the default to "latest". I realize that that won't work
with poolers that don't implement NegotiateProtocolVersion. But I'm
afraid if we make the new protocol version opt-in, no one will use it,
and the poolers etc still won't bother to implement
NegotiateProtocolVersion for years to come, if ever. We can revisit this
decision later in the release cycle though. But I'd suggest changing the
default to "latest" for now, so that more hackers working with
connection poolers will notice, and we get more testing of the new
protocol and the negotiation.


In this regard, I think your proposed protocol change (bumping the
cancel-key length) is different from all of the other protocol
enhancement proposals that I can think of. Most people seem to be
interested in adding an optional feature that some clients might want
and other clients might not care about. Peter Eisentraut's transparent
column encryption stuff is an example of that. What Jelte wants to do
here is too, really, because while these facilities seem like they
could be generally useful for poolers -- at least if we could agree on
what to do and work out all the problems -- and could potentially be
used by applications as well, there would no requirement that any
particular application use any of the new facilities and many of them
wouldn't. So in that kind of world, it makes more sense to me to
default to 3.0 unless the user indicates a desire to use a newer
feature. That way, we minimize breakage at very little cost. Desire to
use the new features can be expected to spur some development in
ecosystem projects, and until that work gets done, many setups are
unaffected.

But the cancel key is a whole different kind of thing. I don't expect
people to be motivated to add support for newer protocol versions just
to get a longer cancel key. If we want people to adopt longer cancel
keys, we need to change the client default and accept that there's
going to be a bunch of breakage until everybody fixes their code.


Agreed.


But is that actually a good idea?

I have some misgivings about that. When somebody's stuff stops working
because of some problem that boils down to "we made the cancel key
longer," I think we're going to have some pretty irate users. I
believe everybody would agree in a vacuum that making cancel keys
longer is probably a good idea, but it seems like a relatively minor
benefit for the amount of inconvenience we're going to be imposing on
everyone.


Agreed.


On the other hand, the logical conclusion of that argument
is that we should never do it, which I don't really believe either.
I'm actually kind of surprised that nobody else (that I've seen,
anyway) has expressed concern about the fact that that proposal
involves a protocol version bump. Have people not noticed? Does nobody
care? To me, that thread reads like "I'm going to make a fire in the
fireplace" but then there's a footnote that reads "by setting off a
nuclear bomb" but we're only talking about how warm and cozy the fire
will be. :-)

I'm sure you're going to say "it's worth it" -- you wouldn't have
written the patch otherwise -- but I wonder if it's going to feel
worth it to everybody who has to deal with the downstream effects.


I didn't realize the issue with poolers is so widespread when I started 
working on that patch this spring, and I gave up hoping to get it into 
v17 when that was brought up.


Now, I think we should still do it, but it might not warrant changing 
the default. Unfortunately that means that it will get very little 
adoption. It will only be adopted as a side-effect of some other changes 
that make people change the protocol_version setting. Or after a very 
long transition period.


That said, I think we *should* change the default for the time being, so 
that developers working on the bleeding edge and building from git get 
some exposure to it. Hopefully that will nudge some of the poolers to 
adopt NegotiateProtocolVersion sooner. But we revert the default to 3.0 
before the v18 release.


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

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

[Robert Haas]

On Thu, Aug 15, 2024 at 6:03 PM Heikki Linnakangas  wrote:
> On the default for "max_protocol_version": I'm pretty disappointed if we
> cannot change the default to "latest". I realize that that won't work
> with poolers that don't implement NegotiateProtocolVersion. But I'm
> afraid if we make the new protocol version opt-in, no one will use it,
> and the poolers etc still won't bother to implement
> NegotiateProtocolVersion for years to come, if ever. We can revisit this
> decision later in the release cycle though. But I'd suggest changing the
> default to "latest" for now, so that more hackers working with
> connection poolers will notice, and we get more testing of the new
> protocol and the negotiation.

In this regard, I think your proposed protocol change (bumping the
cancel-key length) is different from all of the other protocol
enhancement proposals that I can think of. Most people seem to be
interested in adding an optional feature that some clients might want
and other clients might not care about. Peter Eisentraut's transparent
column encryption stuff is an example of that. What Jelte wants to do
here is too, really, because while these facilities seem like they
could be generally useful for poolers -- at least if we could agree on
what to do and work out all the problems -- and could potentially be
used by applications as well, there would no requirement that any
particular application use any of the new facilities and many of them
wouldn't. So in that kind of world, it makes more sense to me to
default to 3.0 unless the user indicates a desire to use a newer
feature. That way, we minimize breakage at very little cost. Desire to
use the new features can be expected to spur some development in
ecosystem projects, and until that work gets done, many setups are
unaffected.

But the cancel key is a whole different kind of thing. I don't expect
people to be motivated to add support for newer protocol versions just
to get a longer cancel key. If we want people to adopt longer cancel
keys, we need to change the client default and accept that there's
going to be a bunch of breakage until everybody fixes their code.

But is that actually a good idea?

I have some misgivings about that. When somebody's stuff stops working
because of some problem that boils down to "we made the cancel key
longer," I think we're going to have some pretty irate users. I
believe everybody would agree in a vacuum that making cancel keys
longer is probably a good idea, but it seems like a relatively minor
benefit for the amount of inconvenience we're going to be imposing on
everyone. On the other hand, the logical conclusion of that argument
is that we should never do it, which I don't really believe either.
I'm actually kind of surprised that nobody else (that I've seen,
anyway) has expressed concern about the fact that that proposal
involves a protocol version bump. Have people not noticed? Does nobody
care? To me, that thread reads like "I'm going to make a fire in the
fireplace" but then there's a footnote that reads "by setting off a
nuclear bomb" but we're only talking about how warm and cozy the fire
will be. :-)

I'm sure you're going to say "it's worth it" -- you wouldn't have
written the patch otherwise -- but I wonder if it's going to feel
worth it to everybody who has to deal with the downstream effects.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

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

[Jelte Fennema-Nio]

On Fri, 16 Aug 2024 at 00:39, Jacob Champion
 wrote:
>
> On Thu, Aug 15, 2024 at 3:04 PM Heikki Linnakangas  wrote:
> > Perhaps we should even change it to return
> > 30 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? 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.

> As for 30001: I don't see the value in modifying an exported API in
> this way. Especially since we can add a new entry point that will be
> guaranteed not to break anyone, as Robert suggested. I think it's a
> POLA violation at minimum; my understanding was that up until this
> point, the value was incremented during major (incompatible) version
> bumps. And I think other users will have had the same understanding.

The advantage is not introducing yet another API when we already have
one with a great name that no-one is currently using. The current API
is in practice just a very convoluted way of writing 3. Also doing an
== 3 check is obviously problematic, if people use this function they
should be using > 3 to be compatible with future versions. So if we
ever introduce protocol version 4, then these (afaict theoretical)
users would break anyway.

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

[Jacob Champion]

On Thu, Aug 15, 2024 at 3:04 PM Heikki Linnakangas  wrote:
> Perhaps we should even change it to return
> 30 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.

As for 30001: I don't see the value in modifying an exported API in
this way. Especially since we can add a new entry point that will be
guaranteed not to break anyone, as Robert suggested. I think it's a
POLA violation at minimum; my understanding was that up until this
point, the value was incremented during major (incompatible) version
bumps. And I think other users will have had the same understanding.

--Jacob

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

[Heikki Linnakangas]

On 14/08/2024 21:04, Jelte Fennema-Nio wrote:

On Wed, 7 Aug 2024 at 22:10, Robert Haas  wrote:

I respect that, but I don't want to get flamed for doing something
that might be controversial without anybody else endorsing it. I'll
commit this if it gets some support, but not otherwise. I'm willing to
commit a patch adding a new function even if nobody else votes, but
not this.


Makes sense. I'm not in too much of a hurry with this specific one. So
I'll leave it like this for now and hopefully someone else responds.
If this becomes close to being the final unmerged patch of this
patchset, I'll probably cut my losses and create a patch that adds a
function instead.


I think Jelte's proposal on PQprotocolVersion() is OK. As he pointed 
out, the function is pretty useless as it is, so I doubt anyone is doing 
anything interesting with it. Perhaps we should even change it to return 
30 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".


On Wed, 7 Aug 2024 at 22:10, Robert Haas  wrote:

> > Patch 7: Bump the protocol version to 3.2 (see commit message for why
> > not bumping to 3.1)
>
> Good commit message. The point is arguable, so putting forth your best
> argument is important.

Just to clarify: do you agree with the point now, after that argument?


Well, here again, I would like to know what other people think. It
doesn't intrinsically matter to me that much what we do here, but it
matters to me a lot that extensive recriminations don't ensue
afterwards.


Makes sense to me. It's sad that pgbouncer had such a bug, but it makes 
sense to accommodate it. We're not going to run out of integers. This 
deserves some more commentary in the docs I think. If I understand the 
plan correctly, if the client requests version 3.1, the server accepts 
it, but behaves exactly the same as 3.0. Or should 3.1 be forbidden 
altogether?


On the default for "max_protocol_version": I'm pretty disappointed if we 
cannot change the default to "latest". I realize that that won't work 
with poolers that don't implement NegotiateProtocolVersion. But I'm 
afraid if we make the new protocol version opt-in, no one will use it, 
and the poolers etc still won't bother to implement 
NegotiateProtocolVersion for years to come, if ever. We can revisit this 
decision later in the release cycle though. But I'd suggest changing the 
default to "latest" for now, so that more hackers working with 
connection poolers will notice, and we get more testing of the new 
protocol and the negotiation.


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

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

[Robert Haas]

On Wed, Aug 14, 2024 at 2:04 PM Jelte Fennema-Nio  wrote:
> Applied all 0002 feedback. Although I used Min(proto,
> PG_PROTOCOL_LATEST) because Max results in the wrong value.

Picky, picky. :-)

Committed.

> Makes sense. I'm not in too much of a hurry with this specific one. So
> I'll leave it like this for now and hopefully someone else responds.
> If this becomes close to being the final unmerged patch of this
> patchset, I'll probably cut my losses and create a patch that adds a
> function instead.

Maybe reorder the series to put that one later then.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

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

[Robert Haas]

On Mon, Jun 24, 2024 at 9:19 AM Jelte Fennema-Nio  wrote:
> > I agree with 0002 except for the change from PG_PROTOCOL_MINOR(proto)
> > > PG_PROTOCOL_MINOR(PG_PROTOCOL_LATEST) to proto > PG_PROTOCOL_LATEST.
> > I prefer that test the way it is; I think the intent is clearer with
> > the existing code.
>
> Great to hear! I reverted that change to the check back to what it was now.

I took another look at 0002 today:

+if (proto > PG_PROTOCOL_LATEST)
+FrontendProtocol = PG_PROTOCOL_LATEST;

A few lines before this, we set FrontendProto = proto. Then we have
one error check, then this. How about instead changing the earlier
assignment to FrontendProto = Max(proto, PG_PROTOCOL_LATEST), or an
if-statement with similar effect? I don't think it practically matters
because send_message_to_frontend() looks well-equipped to handle a
garbage value in FrontendProto, but it seems cleaner to set the value
once and not change it than to change it and then almost immediately
change it again. Also, if we do that, we could fix the comment e.g.
"Set FrontendProtocol now so that ereport() knows what format to send
if we fail during startup, but limit the value to the newest protocol
version we actually support." And if we don't do that, then this new
if-statement needs a comment of its own, and it should explain why we
didn't choose to merge this with the earlier assignment.

-   if (PG_PROTOCOL_MINOR(proto) >
PG_PROTOCOL_MINOR(PG_PROTOCOL_LATEST) ||
-   unrecognized_protocol_options != NIL)
+   if (PG_PROTOCOL_MINOR(proto) >
PG_PROTOCOL_MINOR(PG_PROTOCOL_LATEST)
+   || unrecognized_protocol_options != NIL)

This hunk can be dropped.

-   pq_sendint32(&buf, PG_PROTOCOL_LATEST);
+   pq_sendint32(&buf, FrontendProtocol);

This looks good, but the function header comment needs to be
corrected, e.g. "This lets the client know that they have either
requested a newer minor protocol version than we are able to speak, or
at least one protocol option that we don't understand, or possibly
both. FrontendProtocol has already been set to the version requested
by the client or the highest version we know how to speak, whichever
is older. If the highest version that we know how to speak is too old
for the client, it can abandon the connection."

> > > Patch 3: Similar to 1 & 2 in that it has no actual effect yet. But
> > > after bumping the version this would be a user visible API change, so
> > > I expect it requires a bit more discussion.
> >
> > I don't know if this is the right idea or not. An alternative would be
> > to leave this alone and add PQprotocolMinorVersion().
>
> I considered that, but that makes the API a lot more annoying to use
> for end users when they want to compare to a version, especially when
> they want to include the major version too.
>
> PQprotocolVersion() >= 30001
>
> vs
>
> PQprotocolVersion() > 3 || (PQprotocolVersion() == 3 &&
> PQprotocolVersionMinor() >= 1)
>
> Given that PQprotocolVersion currently has no practical use, because
> it always returns 3 in practice. I personally think that changing the
> behaviour of API in the way I suggested is the best option here.

I respect that, but I don't want to get flamed for doing something
that might be controversial without anybody else endorsing it. I'll
commit this if it gets some support, but not otherwise. I'm willing to
commit a patch adding a new function even if nobody else votes, but
not this.

> > > Patch 4: Adds a new connection option, but initially all parameters
> > > that it takes have the same effect.
> >
> > Generally seems OK, but:
>
> Fixed

Not entirely. The documentation of the environment variable gets the
name of the environment variable wrong.

> > I wonder whether we could define 3.2 to report on all supported
> > protocol parameters even if they weren't in the startup message, to
> > avoid having to jam a lot of stuff we don't really care about into the
> > startup message.
>
> I think that's a good idea. I'll try to look into doing that soonish.

Is this something that you still intend to do? It looks to me like
this would change some things as early as 0006.

Other things about 0006:

+* Since some old servers and poolers don't support the
+* NegotiateProtocolVersion message. So we don't want to send a

You could say "Some old servers ... message, so we don't" or "Some old
servers ... message. So, we don't" or "Since some old servers ...
message, we don't" but it's not quite grammatical the way it is.

+* ask for the latest protocol version we support by
default too.

How about "default to the latest protocol version we support"?

+   for (const pg_protocol_parameter *param =
KnownProtocolParameters; param->name; param++)
+   {
+   const char *value = *(char **) ((char *) conn
+ param->conn_connection_string_value_offset);
+
+   if (value && v

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

[Robert Haas]

On Mon, Jun 24, 2024 at 9:19 AM Jelte Fennema-Nio  wrote:
> > > Patch 3: Similar to 1 & 2 in that it has no actual effect yet. But
> > > after bumping the version this would be a user visible API change, so
> > > I expect it requires a bit more discussion.
> >
> > I don't know if this is the right idea or not. An alternative would be
> > to leave this alone and add PQprotocolMinorVersion().
>
> I considered that, but that makes the API a lot more annoying to use
> for end users when they want to compare to a version, especially when
> they want to include the major version too.
>
> PQprotocolVersion() >= 30001
>
> vs
>
> PQprotocolVersion() > 3 || (PQprotocolVersion() == 3 &&
> PQprotocolVersionMinor() >= 1)
>
> Given that PQprotocolVersion currently has no practical use, because
> it always returns 3 in practice. I personally think that changing the
> behaviour of API in the way I suggested is the best option here.

Mmm, I was thinking of defining the new function to return the major
and minor version in one number, while the existing function could
continue to work as now. I see your point too, but I'd like to hear
some other opinions before we decide, because I think it's unclear
what is actually best here. And also: surely this is not the hill to
die on. If it makes others a lot happier to do it one way or the
other, I'm severely disinclined to spend energy arguing with those
people. And, on the basis of previous experience, this is exactly the
sort of question about which opinions sometimes run quite strongly. I
would much rather swim with the current than get what I would myself
prefer.

> > > Patch 5: Starts using the new connection option from Patch 4
> >
> > Not sure yet whether I like this approach.
>
> Could you explain a bit more what you don't like about it?

I don't dislike it; I'm just not sure whether it is the best approach
to testing the remainder of the patch series. Perhaps the commit
message could explain more why this approach was chosen and what value
we get from it.

> Fair enough. I changed them a bit now, do you think they are good now?

I'll try to re-review the patch set when I have a bit more time than I
do at this exact moment.

> > > Patch 7: Bump the protocol version to 3.2 (see commit message for why
> > > not bumping to 3.1)
> >
> > Good commit message. The point is arguable, so putting forth your best
> > argument is important.
>
> Just to clarify: do you agree with the point now, after that argument?

Well, here again, I would like to know what other people think. It
doesn't intrinsically matter to me that much what we do here, but it
matters to me a lot that extensive recriminations don't ensue
afterwards.

> I did consider an error message (and I think that is what we discussed
> in-person too). But during implementation a WARNING together with a
> simple error indicator seemed nicer since that could hook into the
> existing infrastructure for reporting warnings (both server and client
> side). e.g. you can now provide detail/context/errorcode in the
> warning, without having to add all those to the
> SetProtocolParameterComplete message. I don't feel super strongly
> either way though, so If you prefer the error message to be part of
> the SetProtocolParameterComplete message then I'm happy to change
> that.

My general programming experience has been that any time I decide to
include an error flag rather than an error message, I end up
regretting it. It's possible that this case is, for some reason, an
exception to that principle, but I feel like we need to nail down
exactly what the protocol flow *and* the libpq API for these new
messages is going to be before I can really have an intelligent
opinion about that.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

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

[Robert Haas]

On Wed, Jun 5, 2024 at 10:06 AM Jelte Fennema-Nio  wrote:
> Patch 1 & 2: Minor code changes with zero effect until we actually
> bump the protocol version or add protocol parameters. I hope these can
> be merged rather soon to reduce the number of patches in the patchset.

0001 looks like a bug fix that can (and probably should) be committed
and back-patched. What would reduce work for me is if the commit
message explained why these changes are necessary and to which stable
branches they should be applied and, if that's not all of them, the
reason why back-patching should stop at some particular release. The
change to pqTraceOutput_NegotiateProtocolVersion is easy to
understand: the current code doesn't dump the message format as
documented. It probably doesn't help that the documentation is missing
a word -- it should say "Then, for each protocol option...". It's less
obvious why the change to fe-connect.c is needed. Since most cases
seem to be handled in a few common places, it would be good to mention
in the commit message (or maybe a comment) why this one needs separate
handling.

I agree with 0002 except for the change from PG_PROTOCOL_MINOR(proto)
> PG_PROTOCOL_MINOR(PG_PROTOCOL_LATEST) to proto > PG_PROTOCOL_LATEST.
I prefer that test the way it is; I think the intent is clearer with
the existing code.

> Patch 3: Similar to 1 & 2 in that it has no actual effect yet. But
> after bumping the version this would be a user visible API change, so
> I expect it requires a bit more discussion.

I don't know if this is the right idea or not. An alternative would be
to leave this alone and add PQprotocolMinorVersion().

> Patch 4: Adds a new connection option, but initially all parameters
> that it takes have the same effect.

Generally seems OK, but:
- The commit message needs spelling and grammar checking.
- dispsize 3 isn't long enough for 3.10, or more immediately, "latest".
- This is storing the major protocol version in a variable called
"minor" and the minor protocol version in a variable called "major".
- I think PGMAXPROTOCOLVERSION needs to be added to the docs.

> Patch 5: Starts using the new connection option from Patch 4

Not sure yet whether I like this approach.

> Patch 6: Libpq changes to start handling NegotiateProtocolVersion in a
> more complex way. (nothing changes in practice yet)

+ * NegotiateProtocolVersion message. So we only want to send a

only->don't

+ * protocol version by default. Since either of those would cause a

"default. Since" => "default, since"

+ char*conn_string_value = *(char **) ((char *) conn +
param->conn_connection_string_value_offset);
+ char*server_value = *(char **) ((char *) conn +
param->conn_connection_string_value_offset);
+ char*supported_value = *(char **) ((char *) conn +
param->conn_connection_string_value_offset);

I have some difficulty understanding how these calculations would
produce different answers.

+ libpq_append_conn_error(conn, "invalid NegotiateProtocolVersion
message, server version is newer than client version");
+ libpq_append_conn_error(conn, "invalid NegotiateProtocolVersion
message, negative protocol parameter count");
+ libpq_append_conn_error(conn, "unexpected NegotiateProtocolVersion
message, server supports requested features");

These messages don't seem good. First, I don't think that telling the
user that there's a problem with a specific wire protocol message is
very user-friendly. Second, the use of a comma to glue two
semi-related thoughts together is not a great practice in English in
general and is certainly something we should try to avoid in our error
messages. Third, the first and last of these aren't very clear about
what the problem actually is. I can only understand it from reading
the code.

Maybe these messages could be rephrased as "unable to negotiate
protocol version: blah". Like "unable to negotiate protocol version:
server requests downgrade to higher-numbered version" or "unable to
negotiate protocol version: server negotiates but asks for no
changes".

I don't think I completely understand what's going on in this patch
yet. I'm not sure that it can be committed on its own, and I think it
might need more polishing, including on comments and the commit
message.

> Patch 7: Bump the protocol version to 3.2 (see commit message for why
> not bumping to 3.1)

Good commit message. The point is arguable, so putting forth your best
argument is important.

> Patch 8: The main change: Infrastructure and protocol support to be
> able to add protocol parameters
> Patch 9: Adds a report_parameters protocol extension as a POC for the
> changes in the previous patch.

My general impression on first looking at these patches is that a lot
of the ideas make sense but that they don't seem very close to being
committable.

It's not very clear how these new messages integrate into the overall
protocol flow. The documentation makes the negative statement that
they can't be used as part of the extended query protoco

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

[Robert Haas]

On Thu, Jun 6, 2024 at 3:27 PM Jelte Fennema-Nio  wrote:
> Of course there's always the possibility to review more. But I don't
> really agree with this summary of my review activity.

Nonetheless, I need to take a break from this to work on some of my
own stuff. I'll circle back around to it.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

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

[Jelte Fennema-Nio]

On Thu, 6 Jun 2024 at 18:01, Robert Haas  wrote:
> As I see it, the issue here is whether the default value would ever be
> different from the latest value. If not, then using blank to mean the
> latest seems fine, but if so, then I'd expect blank to mean the
> default version and latest to mean the latest version.

Alright, that's fair. And we already seem to follow that pattern:
There's currently no connection option that has a default that's not
the empty string, but still accepts the empty string as an argument.

> > I'll look into adding min_protocol_version to the patchset soonish.
> > Some review of the existing code in the first few patches would
> > definitely be appreciated.
>
> Yeah, I understand, and I do want to do that, but keep in mind I've
> already spent considerable time on this patch set, way more than most
> others, and if I want to get it committed I'm nowhere close to being
> done. It's probably multiple weeks of additional work for me, and I
> think I've probably already spent close to a week on this, and I only
> work ~48 weeks a year, and there are ~300 patches in the CommitFest.

I very much appreciate the time you spent on this patchset so far. I
mainly thought that instead of only discussing the more complex parts
of the patchset, it would be nice to also actually move forward a
little bit too. And the first 3 patches in this patchset are very
small and imho straightforward improvements.

To be clear, I'm not saying that should be all on you. I think those
first three patches can be reviewed by pretty much anyone.

> Plus, right now there is no possibility of actually committing
> anything until after we branch.

Totally fair, but even a LGTM on one of the patches would be quite nice.

> And, respectfully, I feel like there
> has to be some give and take here. I've been trying to give this patch
> set higher priority because it's in an area that I know something
> about and have opinions about and also because I can tell that you're
> kind of frustrated and I don't want you to leave the development
> community.

Thank you for giving it a higher priority, it's definitely appreciated
and noticed.

> But, at the same time, I don't think you've done much to
> help me get my patches committed, and while you have done some review
> of other people's patches, it doesn't seem to often be the kind of
> detailed, line-by-line review that is needed to get most patches
> committed. So I'm curious how you expect this system to scale.

Of course there's always the possibility to review more. But I don't
really agree with this summary of my review activity. I did see your
patches related to the incremental backup stuff. They looked
interesting, but at the time from an outside perspective it didn't
seem like those threads needed my reviews to progress (a bunch of
people more knowledgable on the topic were already responding). So I
spent my time mainly on threads where I felt I could add something
useful, and often that was more on the design front than the exact
code. Honestly that's what triggered this whole patchset in the first
place: Adding infrastructure for protocol changes so that the several
other threads that try to introduce protocol changes can actually move
forward, instead of being in limbo forever.

Regarding line-by-line reviews, imho I definitely do that for the
smaller patches I tend to review (even if they are part of a bigger
patchset). But the bigger ones I don't think line-by-line reviews are
super helpful at the start, so I generally comment more on the design
in those cases.

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

[Robert Haas]

On Thu, Jun 6, 2024 at 5:12 AM Jelte Fennema-Nio  wrote:
> Looking at ssl_max_protocol_version closer though, to stay really
> consistent I'd have to change "latest" to be renamed to empty string
> (i.e. there is no max_protocol_version). I think I might prefer
> staying consistent over introducing an imho slightly clearer name.
> Another way to stay consistent would of course be also adding "latest"
> as an option to ssl_max_protocol_version? What do you think?

As I see it, the issue here is whether the default value would ever be
different from the latest value. If not, then using blank to mean the
latest seems fine, but if so, then I'd expect blank to mean the
default version and latest to mean the latest version.

> I'll look into adding min_protocol_version to the patchset soonish.
> Some review of the existing code in the first few patches would
> definitely be appreciated.

Yeah, I understand, and I do want to do that, but keep in mind I've
already spent considerable time on this patch set, way more than most
others, and if I want to get it committed I'm nowhere close to being
done. It's probably multiple weeks of additional work for me, and I
think I've probably already spent close to a week on this, and I only
work ~48 weeks a year, and there are ~300 patches in the CommitFest.
Plus, right now there is no possibility of actually committing
anything until after we branch. And, respectfully, I feel like there
has to be some give and take here. I've been trying to give this patch
set higher priority because it's in an area that I know something
about and have opinions about and also because I can tell that you're
kind of frustrated and I don't want you to leave the development
community. But, at the same time, I don't think you've done much to
help me get my patches committed, and while you have done some review
of other people's patches, it doesn't seem to often be the kind of
detailed, line-by-line review that is needed to get most patches
committed. So I'm curious how you expect this system to scale.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

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

[Jelte Fennema-Nio]

On Thu, 6 Jun 2024 at 03:03, Robert Haas  wrote:
> This makes some sense to me. I don't think that I believe
> max_protocol_version=latest is future-proof: just because I want to
> opt into this round of new features doesn't mean I feel the same way
> about the next round. But it may still be a good idea.

I think for most people the only reason not to opt-in to improvements
(even if they are small) is if those improvements break something
else. Once the NegotiateProtocolVersion message is implemented
everywhere in the ecosystem, nothing should break when going from e.g.
3.2 to 3.4. So for the majority of the people I think
max_protocol_version=latest is what they'll want to use once the
ecosystem has caught up. Of course there will be people that want
tight control, but they can set max_protocol_version=3.2 instead.

> I suppose the semantics are that we try to connect with the version
> specified by max_protocol_version and, if we get downgraded by the
> server, we abort if we end up below min_protocol_version.

Correct

> I like those
> semantics, and I think I also like having both parameters, but I'm not
> 100% sure of the naming. It's a funny use of "max" and "min", because
> the max is really what we're trying to do and the min is what we end
> up with, and those terms don't necessarily bring those ideas to mind.
> I don't have a better idea off-hand, though.

I borrowed this terminology from the the ssl_min_protocol_version and
ssl_max_protocol_version connection options that we already have.
Those basically have the same semantics as what I'm proposing here,
but for the TLS protocol version instead of the Postgres protocol
version. I'm also not a huge fan of the min_protocol_version and
max_protocol_version names, but staying consistent with existing
options seems quite nice.

Looking at ssl_max_protocol_version closer though, to stay really
consistent I'd have to change "latest" to be renamed to empty string
(i.e. there is no max_protocol_version). I think I might prefer
staying consistent over introducing an imho slightly clearer name.
Another way to stay consistent would of course be also adding "latest"
as an option to ssl_max_protocol_version? What do you think?

I'll look into adding min_protocol_version to the patchset soonish.
Some review of the existing code in the first few patches would
definitely be appreciated.

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

[Greg Sabino Mullane]

On Wed, Jun 5, 2024 at 9:03 PM Robert Haas  wrote:

>  It's a funny use of "max" and "min", because the max is really what we're
> trying to do and the min is what we end
> up with, and those terms don't necessarily bring those ideas to mind.


requested_protocol_version and minimum_protocol_version?

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

[Robert Haas]

On Wed, Jun 5, 2024 at 5:16 PM Jelte Fennema-Nio  wrote:
> I agree longcancelkey=true is not what we want. In my patch 0004, you
> can specify max_protocol_version=latest to use the latest protocol
> version as opt-in. This is a future proof version of
> protocolversion=3.1 that you're proposing, because it will
> automatically start using 3.2 when it comes out. So I think that
> solves your concern here. (although maybe it should be called
> latest-3.x or something, in case we ever want to add a 4.0 protocol,
> naming is hard)
>
> I personally quite like the max_protocol_version connection parameter.
> I think even for testing it is pretty useful to tell libpq what
> protocol version to try to connect as. It could even be accompanied
> with a min_protocol_version, e.g. in case you only want the connection
> attempt to fail when the server does not support this more secure
> cancel key.

This makes some sense to me. I don't think that I believe
max_protocol_version=latest is future-proof: just because I want to
opt into this round of new features doesn't mean I feel the same way
about the next round. But it may still be a good idea.

I suppose the semantics are that we try to connect with the version
specified by max_protocol_version and, if we get downgraded by the
server, we abort if we end up below min_protocol_version. I like those
semantics, and I think I also like having both parameters, but I'm not
100% sure of the naming. It's a funny use of "max" and "min", because
the max is really what we're trying to do and the min is what we end
up with, and those terms don't necessarily bring those ideas to mind.
I don't have a better idea off-hand, though.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

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

[Jelte Fennema-Nio]

On Wed, 5 Jun 2024 at 22:48, Robert Haas  wrote:
> I agree that we need such a mechanism, but if the driving feature is
> cancel-key length, I expect that opt-in isn't going to work very well.
> Opt-in seems like it would work well with compression or transparent
> column encryption, but few users will specify a connection string
> option just to get a longer cancel key length, so the feature won't
> get much use if we do it that way.

I know Neon wants to make use of this for their proxy (to encode some
tenant_id into the key). So they might want to require people to
opt-in when using their proxy.

> I won't be crushed if we decide to
> somehow make it opt-in, but I kind of doubt that will happen.

> Would we
> make everyone add longcancelkey=true to all their connection strings
> for one release and then carry that connection parameter until the
> heat death of the universe even though after the 1-release transition
> period there would be no reason to ever use it? Would we rip the
> parameter back out after the transition period and break existing
> connection strings? Would we have people write protocolversion=3.1 to
> opt in and then they could just keep that in the connection string
> without harm, or at least without harm until 3.2 comes out? I don't
> really like any of these options that well.

I agree longcancelkey=true is not what we want. In my patch 0004, you
can specify max_protocol_version=latest to use the latest protocol
version as opt-in. This is a future proof version of
protocolversion=3.1 that you're proposing, because it will
automatically start using 3.2 when it comes out. So I think that
solves your concern here. (although maybe it should be called
latest-3.x or something, in case we ever want to add a 4.0 protocol,
naming is hard)

I personally quite like the max_protocol_version connection parameter.
I think even for testing it is pretty useful to tell libpq what
protocol version to try to connect as. It could even be accompanied
with a min_protocol_version, e.g. in case you only want the connection
attempt to fail when the server does not support this more secure
cancel key.

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

[Robert Haas]

On Wed, Jun 5, 2024 at 1:50 PM Jelte Fennema-Nio  wrote:
> Totally agreed, and that's easily achievable because of the
> NegotiateProtocolVersion message. We're all good on v18 to v17
> connections and protocol changes, as long as we make any new behaviour
> depend on either a protocol parameter or a protocol version bump.

Cool.

> I think at minimum we'll need a mechanism for people to connect using
> a StartupMessage that doesn't break existing poolers. If users have no
> way to connect at all to their semi-recently installed connection
> pooler with the newest libpq, then I expect we'll have many unhappy
> users. I think it's debatable whether the compatible StartupMessage
> should be opt-in or opt-out. I'd personally at minimum want to wait
> one PG release before using the incompatible StartupMessage by
> default, just to give pooler installs some time to catch up.

I agree that we need such a mechanism, but if the driving feature is
cancel-key length, I expect that opt-in isn't going to work very well.
Opt-in seems like it would work well with compression or transparent
column encryption, but few users will specify a connection string
option just to get a longer cancel key length, so the feature won't
get much use if we do it that way. I won't be crushed if we decide to
somehow make it opt-in, but I kind of doubt that will happen. Would we
make everyone add longcancelkey=true to all their connection strings
for one release and then carry that connection parameter until the
heat death of the universe even though after the 1-release transition
period there would be no reason to ever use it? Would we rip the
parameter back out after the transition period and break existing
connection strings? Would we have people write protocolversion=3.1 to
opt in and then they could just keep that in the connection string
without harm, or at least without harm until 3.2 comes out? I don't
really like any of these options that well.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

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

[Jelte Fennema-Nio]

On Wed, 5 Jun 2024 at 17:12, Robert Haas  wrote:
> Well, hang on. The discussion on that thread suggests that this is
> only going to break connections to servers that don't have
> NegotiateProtocolVersion.

Yes, correct.

> What
> I really don't want is for v18 to break connections to v17 servers.
> That would be exponentially more disruptive.

Totally agreed, and that's easily achievable because of the
NegotiateProtocolVersion message. We're all good on v18 to v17
connections and protocol changes, as long as we make any new behaviour
depend on either a protocol parameter or a protocol version bump.

> I do take your point that poolers haven't added support for
> NegotiateProtocolVersion yet, but I bet that will change pretty
> quickly once there's a benefit to doing so.

I think at minimum we'll need a mechanism for people to connect using
a StartupMessage that doesn't break existing poolers. If users have no
way to connect at all to their semi-recently installed connection
pooler with the newest libpq, then I expect we'll have many unhappy
users. I think it's debatable whether the compatible StartupMessage
should be opt-in or opt-out. I'd personally at minimum want to wait
one PG release before using the incompatible StartupMessage by
default, just to give pooler installs some time to catch up.

> I think it's a lot easier
> for people to install a new pooler version than a new server version,
> because the server has the data in it. Also, it's not like they
> haven't had time.

I agree that it's a lot easier to upgrade poolers than it is to
upgrade PG, but still people are generally hesitant to upgrade stuff
in their infrastructure. And I totally agree that poolers have had the
time to implement NegotiateProtocolVersion support, but I'm pretty
sure many users will assign blame to libpq anyway.

> But the real question here is whether we think the longer cancel key
> is really important enough to justify the partial compatibility break.
> I'm not deathly opposed to it, but I think it's debatable and I'm sure
> some people are going to be unhappy.

I think if it's an opt-in compatibility break, users won't care how
important it is. It's either important enough to opt-in to this
compatibility break for them, or it's not and nothing changes for
them.

My feeling is that we're unlikely to find a feature that's worth
breaking compatibility (with poolers and pre-9.3 PGs) by default on
its own. But after adding a few new protocol features, at some point
together these features become worth this break. Especially, because
by then 9.3 will be even older and poolers will have started to
support NegotiateProtocolVersion (due to people complaining that they
cannot connect with the new opt-in libpq break-compatibility flag).
But if we're going to wait until we have the one super important
protocol feature, then I don't see us moving forward.

To summarize: I'd like to make some relatively small opt-in change(s)
in PG18 that breaks compatibility (with poolers and pre-9.3 PGs). So
that when we have an important enough reason to break compatibility by
default, that break will be much less painful to do.

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

[Robert Haas]

On Wed, Jun 5, 2024 at 10:06 AM Jelte Fennema-Nio  wrote:
> FYI Heikki his patchset is here:
> https://www.postgresql.org/message-id/flat/508d0505-8b7a-4864-a681-e7e5edfe32aa%40iki.fi
>
> Afaict there's no way to implement this with old clients supporting
> the new message. So it would need to be opt-in from the client
> perspective, either using a version bump or a protocol parameter (e.g.
> large_cancel=true). IMHO a version bump would make more sense for this
> one.

Well, hang on. The discussion on that thread suggests that this is
only going to break connections to servers that don't have
NegotiateProtocolVersion. Personally, I think that's fairly OK. It's
true that connections to, I guess, pre-9.3 servers will break, but
there shouldn't be tons of those left around. It's not great to break
connectivity to such servers, of course, but it seems kind of OK. What
I really don't want is for v18 to break connections to v17 servers.
That would be exponentially more disruptive.

I do take your point that poolers haven't added support for
NegotiateProtocolVersion yet, but I bet that will change pretty
quickly once there's a benefit to doing so. I think it's a lot easier
for people to install a new pooler version than a new server version,
because the server has the data in it. Also, it's not like they
haven't had time.

But the real question here is whether we think the longer cancel key
is really important enough to justify the partial compatibility break.
I'm not deathly opposed to it, but I think it's debatable and I'm sure
some people are going to be unhappy.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

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

[Robert Haas]

On Sat, May 25, 2024 at 6:40 AM Jelte Fennema-Nio  wrote:
> Of the proposed changes so far on the mailing list the only 2 that
> would fall under 1 imho are:
> 1. The ParameterSet message
> 2. Longer than 32bit secret in BackendKeyData

Yeah. I wonder how Heikki thinks he can do (2) without breaking
everything. Maybe just adding an extra, optional, longer field onto
the existing message and hoping that client implementations ignore the
extra field? If that's not good enough, then I don't understand how
that can be done without breaking compatibility in a fundamental and
relatively serious way -- at which point maybe bumping the protocol
version is the right thing to do.

Really, what I'm strongly opposed to is not bumping the version, but
rather doing things that break compatibility such that we need to bump
the version. *If* we have a problem that's sufficiently serious to
justify breaking compatibility anyway, then we don't really lose
anything by bumping the version, and indeed we gain something. I just
want us to be searching for ways to avoid breaking interoperability,
rather than seeking them out. If it becomes impossible for a PG 18 (or
whatever version) server to communicate with earlier servers without
specifying special options, or worse yet at all, then a lot of people
are going to be very sad about that. We will get a bunch of complaints
and a bunch of frustrated users, and they will not be impressed by
vague claims of necessity or desirability. They'll just be mad.

The question for me here is not "what is the theoretically right thing
to do?" but rather "what am I going to tell angry users when they
demand to know why I committed the patch that broke this?". "The old
way was insecure so we had to change it" might be a good enough reason
for people to calm down and stop yelling at me, but "it's no use
having a protocol version if we never bump it" definitely won't be.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

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

[Robert Haas]

On Fri, May 24, 2024 at 6:09 PM Jelte Fennema-Nio  wrote:
> Separating it from the GUC infrastructure will mean we need to
> duplicate a lot of the infrastructure. Assuming we don't care about
> SHOW or pg_settings (which I agree are not super important), the
> things that we would want for protocol parameters to have that guc.c
> gives us for free are:
> 1. Reporting the value of the parameter to the client (done using
> ParameterStatus)
> 2. Parsing and validating of the input, bool, int, enum, etc, but also
> check_hook and assign_hook.
> 3. Logic in all connection poolers to change GUC values to the
> client's expected values whenever a server connection is handed off to
> a client
> 4. Permission checking, if we want some protocol extensions to only be
> configurable by a highly privileged user
>
> All of those things would have to be duplicated/re-implemented if we
> make protocol parameters their own dedicated thing. Doing that work
> seems like a waste of time to me, and would imho add much more
> complexity than the proposed 65 lines of code in 0011.

I think that separating (1) and (3) is going to make us happy rather
than sad. For example, if a client speaks to an intermediate pooler
which speaks to a server, the client-pooler connection could have
different values from the pooler-server connection, and then if
everything is done via ParameterStatus messages it's actually more
complicated for the pooler, which will have to edit ParameterStatus
messages as they pass through, and possibly also create new ones out
of nothing. If we separate things, the pooler can always pass
ParameterStatus messages unchanged, and only has to worry about the
separate infrastructure for handling these new things.

Said differently, I'd agree with you if (a) ParameterStatus weren't so
dubiously designed and (b) all poolers were going to want every
protocol parameter to match on the input side and the output side. And
maybe in practice (b) will happen, but I want to leave the door open
to cases where it doesn't, and as soon as that happens, I think this
becomes a hassle, whereas separate mechanisms don't really hurt much.
As you and I discussed a bit in person last week, two for loops rather
than one in the pooler isn't really that big of a deal. IMHO, that's a
small price to pay for an increased chance of not boxing ourselves
into a corner depending on how these parameters end up getting used in
practice.

As for (2) and (4), I agree there's some duplication, but I think it's
quite minor. We have existing facilities for parsing integers and
booleans that are reused by a lot of code already, and this is just
one more place that can use them. That infrastructure is not
GUC-specific. The permissions-checking stuff isn't either. The vast
bulk of the GUC infrastructure is concerned with (1) allowing for GUCs
to be set from many different sources and (2) handling their
transactional nature. We don't need or want either of those
characteristics here, so a lot of that complexity just isn't needed.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

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

[Dave Cramer]

On Sat, 25 May 2024 at 06:40, Jelte Fennema-Nio  wrote:

> On Thu, 23 May 2024 at 20:12, Tom Lane  wrote:
> >
> > Jelte Fennema-Nio  writes:
> > > On Fri, 17 May 2024 at 21:24, Robert Haas 
> wrote:
> > >> Perhaps these are all minority positions, but I can't tell you what
> > >> everyone thinks, only what I think.
> >
> > > I'd love to hear some opinions from others on these design choices. So
> > > far it seems like we're the only two that have opinions on this (which
> > > seems hard to believe) and our opinions are clearly conflicting. And
> > > above all I'd like to move forward with this, be it my way or yours
> > > (although I'd prefer my way of course ;) )
> >
> > I got around to looking through this thread in preparation for next
> > week's patch review session.  I have a couple of opinions to offer:
> >
> > 1. Protocol versions suck.  Bumping them is seldom a good answer,
> > and should never be done if you have any finer-grained negotiation
> > mechanism available.  My aversion to this is over thirty years old:
> > I learned that lesson from watching the GIF87-to-GIF89 transition mess.
> > Authors of GIF-writing tools tended to take the easy way out and write
> > "GIF89" in the header whether they were actually using any of the new
> > version's features or not.  This led to an awful lot of pictures that
> > couldn't be read by available GIF-displaying tools, for no good reason
> > whatsoever.  The PNG committee, a couple years later, reacted to that
> > mess by designing PNG to have no version number whatsoever, and yet
> > be extensible in a fine-grained way.  (Basically, a PNG file is made
> > up of labeled chunks.  If a reader doesn't recognize a particular
> > chunk code, it can still tell whether the chunk is "critical" or not,
> > and thereby decide if it must give up or can proceed while ignoring
> > that chunk.)
> >
> > So overall, I have a strong preference for using the _pq_.xxx
> > mechanism instead of a protocol version bump.  I do not believe
> > the latter has any advantage.
>
> I'm not necessarily super opposed to only using the _pq_.xxx
> mechanism.


I find it interesting that up to now nobody has ever used this mechanism.


> I mainly think it's silly to have a protocol version number
> and then never use it. And I feel some of the proposed changes don't
> really benefit from being able to be turned on-and-off by themselves.
> My rule of thumb would be:
> 1. Things that a modern client/pooler would always request: version bump
> 2. Everything else: _pq_.xxx
>

Have to agree, why have a protocol version and then just not use it ?

>
> Of the proposed changes so far on the mailing list the only 2 that
> would fall under 1 imho are:
> 1. The ParameterSet message
> 2. Longer than 32bit secret in BackendKeyData
>
> I also don't think the GIF situation you describe translates fully to
> this discussion. We have active protocol version negotiation, so if a
> server doesn't support protocol 3.1 a client is expected to fall back
> to the 3.0 protocol when communicating.


Also agree. Isn't the point of having a version number to figure out what
features the client wants and subsequently the server can provide?

> Of course you can argue that a
> badly behaved client will fail to connect when it gets a downgrade
> request from the server, but that same argument can be made about a
> server not reporting support for a _pq_.xxx parameter that every
> modern client/pooler requests. So I don't think there's a practical
> difference in the problem you're describing.
>

+1

>
>
>
> But again if I'm alone in this, then I don't
>

I would prefer to see a well defined protocol handshaking mechanism rather
than some strange _pq.xxx dance.

Dave

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

[Jelte Fennema-Nio]

(pressed send to early)

On Sat, 25 May 2024 at 12:39, Jelte Fennema-Nio  wrote:
> But again if I'm alone in this, then I don't

... mind budging on this to move this decision along. Using _pq_.xxx
parameters for all protocol changes would totally be acceptable to me.

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

[Jelte Fennema-Nio]

On Thu, 23 May 2024 at 20:12, Tom Lane  wrote:
>
> Jelte Fennema-Nio  writes:
> > On Fri, 17 May 2024 at 21:24, Robert Haas  wrote:
> >> Perhaps these are all minority positions, but I can't tell you what
> >> everyone thinks, only what I think.
>
> > I'd love to hear some opinions from others on these design choices. So
> > far it seems like we're the only two that have opinions on this (which
> > seems hard to believe) and our opinions are clearly conflicting. And
> > above all I'd like to move forward with this, be it my way or yours
> > (although I'd prefer my way of course ;) )
>
> I got around to looking through this thread in preparation for next
> week's patch review session.  I have a couple of opinions to offer:
>
> 1. Protocol versions suck.  Bumping them is seldom a good answer,
> and should never be done if you have any finer-grained negotiation
> mechanism available.  My aversion to this is over thirty years old:
> I learned that lesson from watching the GIF87-to-GIF89 transition mess.
> Authors of GIF-writing tools tended to take the easy way out and write
> "GIF89" in the header whether they were actually using any of the new
> version's features or not.  This led to an awful lot of pictures that
> couldn't be read by available GIF-displaying tools, for no good reason
> whatsoever.  The PNG committee, a couple years later, reacted to that
> mess by designing PNG to have no version number whatsoever, and yet
> be extensible in a fine-grained way.  (Basically, a PNG file is made
> up of labeled chunks.  If a reader doesn't recognize a particular
> chunk code, it can still tell whether the chunk is "critical" or not,
> and thereby decide if it must give up or can proceed while ignoring
> that chunk.)
>
> So overall, I have a strong preference for using the _pq_.xxx
> mechanism instead of a protocol version bump.  I do not believe
> the latter has any advantage.

I'm not necessarily super opposed to only using the _pq_.xxx
mechanism. I mainly think it's silly to have a protocol version number
and then never use it. And I feel some of the proposed changes don't
really benefit from being able to be turned on-and-off by themselves.
My rule of thumb would be:
1. Things that a modern client/pooler would always request: version bump
2. Everything else: _pq_.xxx

Of the proposed changes so far on the mailing list the only 2 that
would fall under 1 imho are:
1. The ParameterSet message
2. Longer than 32bit secret in BackendKeyData

I also don't think the GIF situation you describe translates fully to
this discussion. We have active protocol version negotiation, so if a
server doesn't support protocol 3.1 a client is expected to fall back
to the 3.0 protocol when communicating. Of course you can argue that a
badly behaved client will fail to connect when it gets a downgrade
request from the server, but that same argument can be made about a
server not reporting support for a _pq_.xxx parameter that every
modern client/pooler requests. So I don't think there's a practical
difference in the problem you're describing.



But again if I'm alone in this, then I don't

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

[Jelte Fennema-Nio]

On Thu, 23 May 2024 at 20:40, Tom Lane  wrote:
>
> Jacob Champion  writes:
> > Would it be good to expand on that idea of criticality? IIRC one of
> > Jelte's complaints earlier was that middleware has to know all the
> > extension types anyway, to be able to figure out whether it has to do
> > something about them or not. HTTP has the concept of hop-by-hop vs
> > end-to-end headers for related reasons.
>
> Yeah, perhaps.  We'd need to figure out just which classes we need
> to divide protocol parameters into, and then think about a way for
> code to understand which class a parameter falls into even when
> it doesn't specifically know that parameter.

I think this class is so rare, that it's not worth complicating the
discussion on new protocol features even more. AFAICT there is only
one proposed protocol change that does not need any pooler support
(apart from syncing the feature value when re-assigning the
connectin): Automatic binary encoding for a list of types

All others need some support from poolers, at the very least they need
new message types to not error out. But in many cases more complex
stuff is needed.

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

[Jelte Fennema-Nio]

On Fri, 24 May 2024 at 15:28, Robert Haas  wrote:
>
> On Thu, May 23, 2024 at 4:00 PM Tom Lane  wrote:
> > I don't recall exactly what I thought earlier, but now I think we'd
> > be better off with separate infrastructure.  guc.c is unduly complex
> > already.  Perhaps there are bits of it that could be factored out
> > and shared, but I bet not a lot.
>
> OK. That seems fine to me, but I bet Jelte is going to disagree.

I indeed disagree. I think the effort needed to make guc.c handle
protocol parameters is extremely little. The 0011 patch are all the
changes that are needed to achieve that, and that patch only adds 65
additional lines. And only 15 of those 65 lines actually have to do
anything somewhat weird, to be able to handle the transactionality
discrepancy between protocol parameters and other GUCs. The other 50
lines are (imho) really clean and fit perfectly with the way guc.c is
currently structured (i.e. they add PGC_PROTOCOL and PGC_SU_PROTOCOL
in a really obvious way)

Separating it from the GUC infrastructure will mean we need to
duplicate a lot of the infrastructure. Assuming we don't care about
SHOW or pg_settings (which I agree are not super important), the
things that we would want for protocol parameters to have that guc.c
gives us for free are:
1. Reporting the value of the parameter to the client (done using
ParameterStatus)
2. Parsing and validating of the input, bool, int, enum, etc, but also
check_hook and assign_hook.
3. Logic in all connection poolers to change GUC values to the
client's expected values whenever a server connection is handed off to
a client
4. Permission checking, if we want some protocol extensions to only be
configurable by a highly privileged user

All of those things would have to be duplicated/re-implemented if we
make protocol parameters their own dedicated thing. Doing that work
seems like a waste of time to me, and would imho add much more
complexity than the proposed 65 lines of code in 0011.

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

[Robert Haas]

On Thu, May 23, 2024 at 4:00 PM Tom Lane  wrote:
> I don't recall exactly what I thought earlier, but now I think we'd
> be better off with separate infrastructure.  guc.c is unduly complex
> already.  Perhaps there are bits of it that could be factored out
> and shared, but I bet not a lot.

OK. That seems fine to me, but I bet Jelte is going to disagree.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

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

[Tom Lane]

Robert Haas  writes:
> I think part of the reason we ended up with the protocol parameters =
> GUCs thing is because you seemed to be concurring with that approach
> upthread. I think it was Jelte's idea originally, but I interpreted
> some of your earlier remarks to be supporting it. I'm not sure whether
> you've revised your opinion, or just refined it, or whether we
> misinterpreted your earlier remarks.

I don't recall exactly what I thought earlier, but now I think we'd
be better off with separate infrastructure.  guc.c is unduly complex
already.  Perhaps there are bits of it that could be factored out
and shared, but I bet not a lot.

regards, tom lane

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

[Robert Haas]

On Thu, May 23, 2024 at 2:12 PM Tom Lane  wrote:
> I got around to looking through this thread in preparation for next
> week's patch review session.  I have a couple of opinions to offer:

I agree with these opinions. Independently of that, I'm glad you shared them.

I think part of the reason we ended up with the protocol parameters =
GUCs thing is because you seemed to be concurring with that approach
upthread. I think it was Jelte's idea originally, but I interpreted
some of your earlier remarks to be supporting it. I'm not sure whether
you've revised your opinion, or just refined it, or whether we
misinterpreted your earlier remarks.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

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

[Tom Lane]

Jacob Champion  writes:
> Would it be good to expand on that idea of criticality? IIRC one of
> Jelte's complaints earlier was that middleware has to know all the
> extension types anyway, to be able to figure out whether it has to do
> something about them or not. HTTP has the concept of hop-by-hop vs
> end-to-end headers for related reasons.

Yeah, perhaps.  We'd need to figure out just which classes we need
to divide protocol parameters into, and then think about a way for
code to understand which class a parameter falls into even when
it doesn't specifically know that parameter.  That seems possible
though.  PNG did it with spelling rules for the chunk labels.
Here, since we don't yet have any existing _pq_.xxx parameter names,
we could maybe say that the names shall follow a pattern like
"_pq_.class.param".  (That works only if the classes are
non-overlapping, an assumption not yet justified by evidence;
but we could do something more complicated if we have to.)

regards, tom lane

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

[Jacob Champion]

On Thu, May 23, 2024 at 11:12 AM Tom Lane  wrote:
> If a reader doesn't recognize a particular
> chunk code, it can still tell whether the chunk is "critical" or not,
> and thereby decide if it must give up or can proceed while ignoring
> that chunk.)

Would it be good to expand on that idea of criticality? IIRC one of
Jelte's complaints earlier was that middleware has to know all the
extension types anyway, to be able to figure out whether it has to do
something about them or not. HTTP has the concept of hop-by-hop vs
end-to-end headers for related reasons.

--Jacob

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

[Tom Lane]

Jelte Fennema-Nio  writes:
> On Fri, 17 May 2024 at 21:24, Robert Haas  wrote:
>> Perhaps these are all minority positions, but I can't tell you what
>> everyone thinks, only what I think.

> I'd love to hear some opinions from others on these design choices. So
> far it seems like we're the only two that have opinions on this (which
> seems hard to believe) and our opinions are clearly conflicting. And
> above all I'd like to move forward with this, be it my way or yours
> (although I'd prefer my way of course ;) )

I got around to looking through this thread in preparation for next
week's patch review session.  I have a couple of opinions to offer:

1. Protocol versions suck.  Bumping them is seldom a good answer,
and should never be done if you have any finer-grained negotiation
mechanism available.  My aversion to this is over thirty years old:
I learned that lesson from watching the GIF87-to-GIF89 transition mess.
Authors of GIF-writing tools tended to take the easy way out and write
"GIF89" in the header whether they were actually using any of the new
version's features or not.  This led to an awful lot of pictures that
couldn't be read by available GIF-displaying tools, for no good reason
whatsoever.  The PNG committee, a couple years later, reacted to that
mess by designing PNG to have no version number whatsoever, and yet
be extensible in a fine-grained way.  (Basically, a PNG file is made
up of labeled chunks.  If a reader doesn't recognize a particular
chunk code, it can still tell whether the chunk is "critical" or not,
and thereby decide if it must give up or can proceed while ignoring
that chunk.)

So overall, I have a strong preference for using the _pq_.xxx
mechanism instead of a protocol version bump.  I do not believe
the latter has any advantage.

2. I share Robert's suspicion of equating protocol parameters
with GUCs.  The GUC mechanism is quite opinionated and already
serves multiple masters.  In particular, the fact that GUC
settings are normally transactional does not play nice with
the way protocol parameters need to behave.  Yeah, no doubt you
could add another dollop of complexity to guc.c to make parameters
work differently from other GUCs, but I think it's the wrong design
direction.  We should handle protocol parameters with a separate
mechanism.  It's not, for instance, clear to me that protocol
parameters should be exposed at the SQL level at all; but if we
don't feel they need to be available via SHOW and pg_settings,
what benefit is guc.c really bringing to the table?

regards, tom lane

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

[Jelte Fennema-Nio]

On Fri, 17 May 2024 at 21:24, Robert Haas  wrote:
> I think the
> fear that we're going to run into cases where such complex handshaking
> is necessary is a major reason why I'm afraid of Jelte's ideas about
> ParameterSet: it seems much more opinionated to me than he seems to
> think it is.

I think that fear is valid, and I agree that we might want to add a
bespoke message for cases where ParameterSet is not enough. But as far
as I can tell ParameterSet would at least cover all the protocol
changes that have been suggested so far. Using an opinionated but
limited message type for 90% of the cases and a bespoke message for
the last 10% seems much better to me than having a bespoke one for
each (especially if currently none of the protocol proposals fall into
the last 10%).

> To the extent that I can wrap my head around what Jelte is proposing,
> and all signs point to that extent being quite limited, I suppose I
> was thinking of something like his option (2). That is, I assumed that
> a client would request all the optional features that said client
> might wish to use at connection startup time. But I can see how that
> assumption might be somewhat problematic in a connection-pooling
> environment.

To be clear, I'd also be totally fine with my option (2). I'm
personally slightly leaning towards my option (1), due to the reasons
listed before. But connection poolers could request all the protocol
extensions at the start just fine (using the default "disabled" value)
to check for support. So I think option (2) would probably be the most
conservative, i.e. we could always decide that option (1) is fine in
some future release.

> Still, at least to me, it seems better than trying to
> rely on GUC_REPORT. My opinion is (1) GUC_REPORT isn't a particularly
> well-designed mechanism so I dislike trying to double down on it

I agree that GUC_REPORT is not particularly well designed,
currently... But even in its current form it's already a very
effective mechanism for connection poolers to find out to which value
a specific GUC is set to, and if something similar to patch 0014 would
be merged my main gripe with GUC_REPORT would be gone. Tracking GUC
settings by using ParameterSet would actually be harder, because if
ParameterSet errors at Postgres then the connection pooler would have
to roll back its cache of that setting. While with the GUC_REPORT
response from Postgres it can simply rely on Postgres telling the
pooler that the GUC has changed, even rollbacks are handled correctly
this way.

> and
> (2) trying to mix these protocol-level parameters and the
> transactional GUCs we have together in a single mechanism seems
> potentially problematic

I don't understand what potential problems you're worried about here.
Could you clarify?

> and (3) I'm still not particularly happy about
> the idea of making protocol parameters into GUCs in the first place.

Similar to the above: Could you clarify why you're not happy about that?

> Perhaps these are all minority positions, but I can't tell you what
> everyone thinks, only what I think.

I'd love to hear some opinions from others on these design choices. So
far it seems like we're the only two that have opinions on this (which
seems hard to believe) and our opinions are clearly conflicting. And
above all I'd like to move forward with this, be it my way or yours
(although I'd prefer my way of course ;) )

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

[Robert Haas]

On Fri, May 17, 2024 at 1:26 PM Jacob Burroughs
 wrote:
> I was leaving the details around triggering that for this conversation
> and in that patch just designing the messages in a way that would
> allow adding the reconfiguration/restarting to be easily added in a
> backwards-compatible way in a future patch.  I would imagine that an
> explicit `ParameterSet` call that sets `_pq_.connection_compression`
> (or whatever the implementation details turn out to be) would also
> trigger a compressor restart, and when restarted it would pick an
> algorithm/configuration based on the new value of the parameter rather
> than the one used at connection establishment.

Hmm, OK, interesting. I suppose that I thought we were going to handle
problems like this by just adding bespoke messages for each case (e.g.
CompressionSet). I wasn't thinking it would be practical to make a
message whose remit was as general as what it seems like Jelte wants
to do with ParameterSet, because I'm not sure everything can be
handled that simply. It's not an exact analogy, but when you want to
stop the server, it's not enough to say that you want to change from
the Running state to the Stopped state. You have to specify which type
of shutdown should be used to make the transition. You could even have
more complicated cases, where one side says "prepare to do X" and the
other side eventually says "OK, I'm prepared" and the first side says
"great, now activate X" and the other side eventually says "OK, it's
activate, please confirm that you've also activated it from your side"
and the first side eventually says "OK, I confirm that". I think the
fear that we're going to run into cases where such complex handshaking
is necessary is a major reason why I'm afraid of Jelte's ideas about
ParameterSet: it seems much more opinionated to me than he seems to
think it is.

To the extent that I can wrap my head around what Jelte is proposing,
and all signs point to that extent being quite limited, I suppose I
was thinking of something like his option (2). That is, I assumed that
a client would request all the optional features that said client
might wish to use at connection startup time. But I can see how that
assumption might be somewhat problematic in a connection-pooling
environment. Still, at least to me, it seems better than trying to
rely on GUC_REPORT. My opinion is (1) GUC_REPORT isn't a particularly
well-designed mechanism so I dislike trying to double down on it and
(2) trying to mix these protocol-level parameters and the
transactional GUCs we have together in a single mechanism seems
potentially problematic and (3) I'm still not particularly happy about
the idea of making protocol parameters into GUCs in the first place.
Perhaps these are all minority positions, but I can't tell you what
everyone thinks, only what I think.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

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

[Jacob Burroughs]

On Fri, May 17, 2024 at 3:15 AM Robert Haas  wrote:
>
> OK, so you made it so that compressed data is fully self-identifying.
> Hence, there's no need to worry if something gets changed: the
> receiver, if properly implemented, can't help but notice. The only
> downside that I can see to this design is that you only have one byte
> to identify the compression algorithm, but that doesn't actually seem
> like a real problem at all, because I expect the number of supported
> compression algorithms to grow very slowly. I think it would take
> centuries, possibly millenia, before we started to get short of
> identifiers. So, cool.
>
> But, in your system, how does the client ask the server to switch to a
> different compression algorithm, or to restart the compression stream?

I was leaving the details around triggering that for this conversation
and in that patch just designing the messages in a way that would
allow adding the reconfiguration/restarting to be easily added in a
backwards-compatible way in a future patch.  I would imagine that an
explicit `ParameterSet` call that sets `_pq_.connection_compression`
(or whatever the implementation details turn out to be) would also
trigger a compressor restart, and when restarted it would pick an
algorithm/configuration based on the new value of the parameter rather
than the one used at connection establishment.



-- 
Jacob Burroughs | Staff Software Engineer
E: jburrou...@instructure.com

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

[Robert Haas]

On Thu, May 16, 2024 at 1:39 PM Jacob Burroughs
 wrote:
> As currently implemented [1], the client sends the server the list of
> all compression algorithms it is willing to accept, and the server can
> use one of them.  If the server knows what `_pq_.compression` means
> but doesn't actually support any compression, it will both send the
> client its empty list of supported algorithms and just never send any
> compressed messages, and everyone involved will be (relatively) happy.
> There is a libpq function that a client can use to check what
> compression is in use if a client *really* doesn't want to continue
> with the conversation without compression, but 99% of the time I can't
> see why a client wouldn't prefer to continue using a connection with
> whatever compression the server supports (or even none) without more
> explicit negotiation.  (Unlike TLS, where automagically picking
> between using and not using TLS has strange security implications and
> effects, compression is a convenience feature for everyone involved.)

This all seems sensible to me.

> As the protocol layer is currently designed [1], it explicitly makes
> it very easy to change/restart compression streams, specifically for
> this use case (and in particular for the general connection pooler use
> case).  Compressed data is already framed in a `CompressedData`
> message, and that message has a header byte that corresponds to an
> enum value for which algorithm is currently in use.  Any time the
> compression stream was restarted by the sender, the first
> `CompressedData` message will set that byte, and then the client will
> restart its decompression stream with the chosen algorithm from that
> point.  For `CompressedData` messages that continue using the
> already-established stream, the byte is simply set to 0.  (This is
> also how the "each side sends a list" form of negotiation is able to
> work without additional round trips, as the `CompressedData` framing
> itself communicates which compression algorithm has been selected.)

OK, so you made it so that compressed data is fully self-identifying.
Hence, there's no need to worry if something gets changed: the
receiver, if properly implemented, can't help but notice. The only
downside that I can see to this design is that you only have one byte
to identify the compression algorithm, but that doesn't actually seem
like a real problem at all, because I expect the number of supported
compression algorithms to grow very slowly. I think it would take
centuries, possibly millenia, before we started to get short of
identifiers. So, cool.

But, in your system, how does the client ask the server to switch to a
different compression algorithm, or to restart the compression stream?

-- 
Robert Haas
EDB: http://www.enterprisedb.com

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

[Jelte Fennema-Nio]

On Thu, 16 May 2024 at 18:57, Robert Haas  wrote:
> Ugh, it's so hard to communicate clearly about this stuff. I didn't
> really have any thought that we'd ever try to handle something as
> complicated as compression using ParameterSet.

Okay, then it's definitely very hard to communicate clearly about
this. Because being able to re-configure compression using
ParameterSet is exactly the type of thing I want to be able to do in
PgBouncer. Otherwise a connection from PgBouncer to Postgres cannot be
handed off to any client, because its compression state cannot be
changed on the fly to the state that a client expects, if the first
one wants lz4 compression, and a second one wants zstd compression.
There's no way the same connection can be reused for both unless you
decompress and recompress in pgbouncer, which is expensive to do.
Being able to reconfigure the stream to compress the messages in the
expected form is much cheaper.

> Does it send a NegotiateCompressionType message after
> the NegotiateProtocolVersion, for example? That seems like it could
> lead to the client having to be prepared for a lot of NegotiateX
> messages somewhere down the road.

Like Jacob explains, you'd want to allow the client to provide a list
of options in order of preference. And then have the server respond
with a ParameterStatus message saying what it ended up being. So no
new NegotiateXXX messages are needed, as long as we make sure any
_pq_.xxx falls back to reporting its default value on failure. This is
exactly why I said I prefer option 1 of the options I listed, because
we need _pq_.xxx messages to report their current value to the client.

To be clear, this is not special to compression. This applies to ALL
proposed protocol parameters. The server should fall back to some
"least common denominator" if it doesn't understand (part of) the
value for the protocol parameter that's provided by the client,
possibly falling back to disabling the protocol extension completely.

> Changing compression algorithms in mid-stream is tricky, too. If I
> tell the server "hey, turn on server-to-client compression!"

Yes it is tricky, but it's something that it would need to support
imho. And Jacob actually implemented it this way, so I feel like we're
discussing a non-problem here.

> I don't know if we want to support changing compression algorithms in
> mid-stream. I don't think there's any reason we can't, but it might be
> a bunch of work for something that nobody really cares about.

Again, I guess I wasn't clear at all in my previous emails and/or
commit messages. Connection poolers care **very much** about this.
Poolers need to be able to re-configure any protocol parameter to be
able to pool the same server connection across clients with
differently configured protocol parameters. Again: This is the primary
reason for me wanting to introduce the ParameterSet message.

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

[Jacob Burroughs]

On Thu, May 16, 2024 at 6:57 AM Robert Haas  wrote:
>
> Ugh, it's so hard to communicate clearly about this stuff. I didn't
> really have any thought that we'd ever try to handle something as
> complicated as compression using ParameterSet. I tend to agree that
> for compression I'd like to see the startup packet contain more than
> _pq_.compression=1, but I'm not sure what would happen after that
> exactly. If the client asks for _pq_.compression=lz4 and the server
> tells the client that it doesn't understand _pq_.compression at all,
> then everybody's on the same page: no compression. But, if the server
> understands the option but isn't OK with the proposed value, what
> happens then? Does it send a NegotiateCompressionType message after
> the NegotiateProtocolVersion, for example? That seems like it could
> lead to the client having to be prepared for a lot of NegotiateX
> messages somewhere down the road.
>
> I think at some point in the past we had discussed having the client
> list all the algorithms it supported in the argument to
> _pq_.compression, and then the server would respond with the algorithm
> it wanted use, or maybe a list of algorithms that it could allow, and
> then we'd go from there. But I'm not entirely sure if that's the right
> idea, either.

As currently implemented [1], the client sends the server the list of
all compression algorithms it is willing to accept, and the server can
use one of them.  If the server knows what `_pq_.compression` means
but doesn't actually support any compression, it will both send the
client its empty list of supported algorithms and just never send any
compressed messages, and everyone involved will be (relatively) happy.
There is a libpq function that a client can use to check what
compression is in use if a client *really* doesn't want to continue
with the conversation without compression, but 99% of the time I can't
see why a client wouldn't prefer to continue using a connection with
whatever compression the server supports (or even none) without more
explicit negotiation.  (Unlike TLS, where automagically picking
between using and not using TLS has strange security implications and
effects, compression is a convenience feature for everyone involved.)

> Changing compression algorithms in mid-stream is tricky, too. If I
> tell the server "hey, turn on server-to-client compression!" then I
> need to be able to identify where exactly that happens. Any messages
> already sent by the server and not yet processed by me, or any
> messages sent after that but before the server handles my request, are
> going to be uncompressed. Then, at some point, I'll start getting
> compressed data. If the compressed data is framed inside some message
> type created for that purpose, like I get a CompressedMessage message
> and then I decompress to get the actual message, this is simpler to
> manage. But even then, it's tricky if the protocol shifts. If I tell
> the server, you know what, gzip was a bad choice, I want lz4, I'll
> need to know where the switch happens to be able to decompress
> properly.
>
> I don't know if we want to support changing compression algorithms in
> mid-stream. I don't think there's any reason we can't, but it might be
> a bunch of work for something that nobody really cares about. Not
> sure.

As the protocol layer is currently designed [1], it explicitly makes
it very easy to change/restart compression streams, specifically for
this use case (and in particular for the general connection pooler use
case).  Compressed data is already framed in a `CompressedData`
message, and that message has a header byte that corresponds to an
enum value for which algorithm is currently in use.  Any time the
compression stream was restarted by the sender, the first
`CompressedData` message will set that byte, and then the client will
restart its decompression stream with the chosen algorithm from that
point.  For `CompressedData` messages that continue using the
already-established stream, the byte is simply set to 0.  (This is
also how the "each side sends a list" form of negotiation is able to
work without additional round trips, as the `CompressedData` framing
itself communicates which compression algorithm has been selected.)

[1] 
https://www.postgresql.org/message-id/CACzsqT5-7xfbz%2BSi35TBYHzerNX3XJVzAUH9AewQ%2BPp13fYBoQ%40mail.gmail.com

-- 
Jacob Burroughs | Staff Software Engineer
E: jburrou...@instructure.com

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

[Robert Haas]

On Thu, May 16, 2024 at 12:09 PM Jelte Fennema-Nio  wrote:
> I don't really understand the benefit of your proposal over option 2
> that I proposed. Afaict you're proposing that for e.g. compression we
> first set _pq_.supports_compression=1 in the StartupMessage and use
> that  to do feature detection, and then after we get the response we
> send ParameterSet("compression", "gzip"). To me this is pretty much
> identical to option 2, except that it introduces an extra round trip
> for no benefit (as far as I can see). Why not go for option 2 and send
> _pq_.compression=gzip in the StartupMessage directly.

Ugh, it's so hard to communicate clearly about this stuff. I didn't
really have any thought that we'd ever try to handle something as
complicated as compression using ParameterSet. I tend to agree that
for compression I'd like to see the startup packet contain more than
_pq_.compression=1, but I'm not sure what would happen after that
exactly. If the client asks for _pq_.compression=lz4 and the server
tells the client that it doesn't understand _pq_.compression at all,
then everybody's on the same page: no compression. But, if the server
understands the option but isn't OK with the proposed value, what
happens then? Does it send a NegotiateCompressionType message after
the NegotiateProtocolVersion, for example? That seems like it could
lead to the client having to be prepared for a lot of NegotiateX
messages somewhere down the road.

I think at some point in the past we had discussed having the client
list all the algorithms it supported in the argument to
_pq_.compression, and then the server would respond with the algorithm
it wanted use, or maybe a list of algorithms that it could allow, and
then we'd go from there. But I'm not entirely sure if that's the right
idea, either.

Changing compression algorithms in mid-stream is tricky, too. If I
tell the server "hey, turn on server-to-client compression!" then I
need to be able to identify where exactly that happens. Any messages
already sent by the server and not yet processed by me, or any
messages sent after that but before the server handles my request, are
going to be uncompressed. Then, at some point, I'll start getting
compressed data. If the compressed data is framed inside some message
type created for that purpose, like I get a CompressedMessage message
and then I decompress to get the actual message, this is simpler to
manage. But even then, it's tricky if the protocol shifts. If I tell
the server, you know what, gzip was a bad choice, I want lz4, I'll
need to know where the switch happens to be able to decompress
properly.

I don't know if we want to support changing compression algorithms in
mid-stream. I don't think there's any reason we can't, but it might be
a bunch of work for something that nobody really cares about. Not
sure.

--
Robert Haas
EDB: http://www.enterprisedb.com

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

[Jelte Fennema-Nio]

On Thu, 16 May 2024 at 17:29, Robert Haas  wrote:
> You're probably not going to like this answer, but I feel like this is
> another sign that you're trying to use the protocol extensibility
> facilities in the wrong way. In my first reply to the thread, I
> proposed having the client send _pq_.protocol_set=1 in that startup
> message. If the server accepts that message, then you can send
> whatever set of message types are associated with that option, which
> could include messages to list known settings, as well as messages to
> set them. Alternatively, if we used a wire protocol bump for this, you
> could request version 3.1 and everything that I just said still
> applies. In other words, I feel that if you had adopted the design
> that I proposed back in March, or some variant of it, the problem
> you're having now wouldn't exist.

I don't really understand the benefit of your proposal over option 2
that I proposed. Afaict you're proposing that for e.g. compression we
first set _pq_.supports_compression=1 in the StartupMessage and use
that  to do feature detection, and then after we get the response we
send ParameterSet("compression", "gzip"). To me this is pretty much
identical to option 2, except that it introduces an extra round trip
for no benefit (as far as I can see). Why not go for option 2 and send
_pq_.compression=gzip in the StartupMessage directly.

> IMHO, we need to negotiate the language that we're going to use to
> communicate before we start communicating. We should find out which
> protocol version we're using, and what protocol options are accepted,
> based on sending a StartupMessage and receiving a reply. Then, after
> that, having established a common language, we can do whatever. I
> think you're trying to meld those two steps into one, which is
> understandable from the point of view of saving a round trip, but I
> just don't see it working out well.

I think not increasing the number of needed round trips in the startup
of a connection is extremely important. I think it's so important that
I honestly don't think we should merge a protocol change that
introduces an extra round trip without a VERY good reason, and this
round trip should only be needed when actually using the feature.

> What we can do in the startup
> message is extremely limited, because any startup messages we generate
> can't break existing servers, and also because of the concerns I
> raised earlier about leaving room for more extension in the future.
> Once we get past the startup message negotiation, the sky's the limit!

Sure, what we can do in the StartupMessage is extremely limited, but
what it does allow is passing arbitrary key value pairs to the server.
But by only using _pq_.feature_name=1, we're effectively only using
the key part of the key value pair. Limiting ourselves even more, by
throwing half of our communication channel away, seems like a bad idea
to me. But maybe I'm just not understanding the problem you're seeing
with using the value too.

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

[Robert Haas]

On Thu, May 16, 2024 at 5:22 AM Jelte Fennema-Nio  wrote:
> If we're not setting the protocol parameter in the StartupMessage,
> there's currently no way for us to know if the protocol parameter is
> supported by the server. If protocol parameters were unchangable then
> that would be fine, but the whole point of introducing ParameterSet is
> to make it possible to change protocol parameters on an existing
> connection. Having the function SupportsProtocolCompression return
> false, even though you can enable compression just fine, only because
> we didn't ask for compression when connecting seems quite silly and
> confusing.

You're probably not going to like this answer, but I feel like this is
another sign that you're trying to use the protocol extensibility
facilities in the wrong way. In my first reply to the thread, I
proposed having the client send _pq_.protocol_set=1 in that startup
message. If the server accepts that message, then you can send
whatever set of message types are associated with that option, which
could include messages to list known settings, as well as messages to
set them. Alternatively, if we used a wire protocol bump for this, you
could request version 3.1 and everything that I just said still
applies. In other words, I feel that if you had adopted the design
that I proposed back in March, or some variant of it, the problem
you're having now wouldn't exist.

IMHO, we need to negotiate the language that we're going to use to
communicate before we start communicating. We should find out which
protocol version we're using, and what protocol options are accepted,
based on sending a StartupMessage and receiving a reply. Then, after
that, having established a common language, we can do whatever. I
think you're trying to meld those two steps into one, which is
understandable from the point of view of saving a round trip, but I
just don't see it working out well. What we can do in the startup
message is extremely limited, because any startup messages we generate
can't break existing servers, and also because of the concerns I
raised earlier about leaving room for more extension in the future.
Once we get past the startup message negotiation, the sky's the limit!

-- 
Robert Haas
EDB: http://www.enterprisedb.com

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

[Jelte Fennema-Nio]

On Fri, 5 Apr 2024 at 18:30, Dave Cramer  wrote:
> On Fri, 5 Apr 2024 at 12:09, Jelte Fennema-Nio  wrote:
>> I'll take a look at redesigning the protocol parameter stuff. To work
>> with dedicated functions instead.
>
> +1

It's been a while, but I now actually took the time to look into this.
And I ran into a problem that I'd like to get some feedback on before
continuing the implementation:

If we're not setting the protocol parameter in the StartupMessage,
there's currently no way for us to know if the protocol parameter is
supported by the server. If protocol parameters were unchangable then
that would be fine, but the whole point of introducing ParameterSet is
to make it possible to change protocol parameters on an existing
connection. Having the function SupportsProtocolCompression return
false, even though you can enable compression just fine, only because
we didn't ask for compression when connecting seems quite silly and
confusing.

I see five ways around this problem and would love some feedback on
which you think is best (or if you can think of any other/better
ones):
1. Have protocol parameters always be GUC_REPORT, so that the presence
of a ParameterStatus message during connection startup can be used as
a way of detecting support for the protocol parameter.
2. Make libpq always send each known protocol parameter in the
StartupMessage to check for their support, even if the connection
string does not contain the related parameters (set them to their
default value then). Then the non-presence of the parameter in the
NegotiateProtocolVersion message can be used reliably to determine
support for the feature. We could even disallow changing a protocol
parameter at the server side using ParameterSet if it was not
requested in the StartupMessage.
3. Very similar to 1, but require explicit user input in the
connection string to request the feature on connection startup by
having the user explicitly provide its default value. If it's not
requested on connection startup assume its unsupported and disallow
usage of the feature (even if the server might actually support it).
4. Make SupportsProtocolCompression return a tri-state, SUPPORTED,
UNSUPPORTED, UNKNOWN. If it's UNKNOWN people could send a ParameterSet
message themselves to check for feature support after connection
startup. We could even recognize this and change the state that
SupportProtocolCompression function to return SUPPORTED/UNSUPPORTED on
future calls according to the server response.
5. Basically the same as 4 but automatically send a ParameterSet
message internally when calling SupportsProtocolCompression and the
state is UNKNOWN, so we'd only ever return SUPPORTED or UNSUPPORTED.

The above options are listed in my order of preference, below some
reasoning why:

1 and 2 would increase the bandwidth used during connection handshake
slightly for each protocol parameter that we would add, but they have
the best user experience IMHO.

I slightly prefer 1 over 2 because there is another argument to be
made for always having protocol parameters be GUC_REPORT: these
parameters change what message types a client can send or receive. So
it makes sense to me to have the server tell the client what the
current value of such a parameter is. This might not be a strong
argument though, because this value would only ever change due to user
interaction. But still, one might imagine scenarios where the value
that the client sent is not exactly what the server would set the
parameter to on receiving that value from the client. e.g. for
protocol compression, maybe the client sends a list of prefered
compression methods and the server would send a ParameterStatus
containing only the specific compression method that it will use when
sending messages to the client.

3 seems also an acceptable option to me. While having slightly worse
user experience than 2, it allows the user of the client to make the
decision if the extra bandwidth during connection startup is worth it
to be able to enable the feature later.

4 assumes that we want people to be able to trigger sending
ParameterSet messages for every protocol parameter. I'm not sure we'd
want to give that ability in all cases.

5 would require SupportsProtocolCompression to also have a
non-blocking version, which bloats our API more than I'd like. Also as
a user you wouldn't be able to know if SupportsProtocolCompression
will do a network request or not.

PS. This is only a problem for feature detection for features relying
on protocol parameters, feature-support relying on protocol version
bumps are easy to detect based on the NegotiateProtocolVersion
message.

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

[Jelte Fennema-Nio]

On Tue, 23 Apr 2024 at 19:39, Jacob Champion
 wrote:
>
> On Mon, Apr 22, 2024 at 2:20 PM Jelte Fennema-Nio  wrote:
> > 1. I strongly believe minor protocol version bumps after the initial
> > 3.1 one can be made painless for clients/poolers (so the ones to
> > 3.2/3.3/etc). Similar to how TLS 1.3 can be safely introduced, and not
> > having to worry about breaking TLS 1.2 communication.
>
> Apologies for focusing on a single portion of your argument, but this
> claim in particular stuck out to me. To my understanding, IETF worried
> a _lot_ about breaking TLS 1.2 implementations with the TLS 1.3
> change, to the point that TLS 1.3 clients and servers advertise
> themselves as TLS 1.2 and sneak the actual version used into a TLS
> extension (roughly analogous to the _pq_ stuff). I vaguely recall that
> the engineering work done for that update was pretty far from
> painless.

My bad... I guess TLS 1.3 was a bad example, due to it changing the
handshake itself so significantly.

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

[Jelte Fennema-Nio]

On Tue, 23 Apr 2024 at 17:03, Robert Haas  wrote:
> If a client doesn't know about encrypted columns and sets that
> bit at random, that will break things, and formally I think that's a
> risk, because I don't believe we document anywhere that you shouldn't
> set unused bits in the format mask. But practically, it's not likely.
> (And also, maybe we should document that you shouldn't do that.)

Currently Postgres errors out when you set anything other than 0 or 1
in the format field. And it's already documented that these are the
only allowed values: "Each must presently be zero (text) or one
(binary)."

> Let's consider a hypothetical country much like Canada except that
> there are three official languages rather than two: English, French,
> and Robertish.

I don't really understand the point you are trying to make with this
analogy. Sure an almost equivalent unused language maybe shouldn't be
put on the signs, but having two different names (aka protocol
versions) for English and Robertish seems quite useful to avoid
confusion.

> And so here. If someone codes a connection pooler in the way you
> suppose, then it will break. But, first of all, they probably won't do
> that, both because it's not particularly likely that someone wants to
> gather that particular set of statistics and also because erroring out
> seems like an overreaction.

Is it really that much of an overreaction? Postgres happily errors on
any weirdness in the protocol including unknown format codes. Why
wouldn't a pooler/proxy in the middle be allowed to do the same? The
pooler has no clue what the new format means, maybe it requires some
session state to be passed on to the server to be interpreted
correctly. The pooler would be responsible for syncing that session
state but might not have synced it because it doesn't know that the
format code requires that. An example of this would be a compressed
value of which the compression algorithm is configured using a GUC.
Thus the pooler being strict in what it accepts would be a good way of
notifying the client that the pooler needs to be updated (or the
feature not used).

But I do agree that it seems quite unlikely that a pooler would
implement it like that though. However, a pooler logging a warning
seems not that crazy. And in that case the connection might not be
closed, but the logs would be flooded.

> And secondly, let's imagine that we do
> bump the protocol version and think about whether and how that solves
> the problem. A client will request from the pooler a version 3.1
> connection and the pooler will say, sorry, no can do, I only
> understand 3.0. So the client will now say, oh ok, no problem, I'm
> going to refrain from setting that parameter format bit. Cool, right?
>
> Well, no, not really. First, now the client application is probably
> broken. If the client is varying its behavior based on the server's
> protocol version, that must mean that it cares about accessing
> encrypted columns, and that means that the bit in question is not an
> optional feature. So actually, the fact that the pooler can force the
> client to downgrade hasn't fixed anything at all.

It seems quite a lot nicer if the client can safely fallback to not
writing data using the new format code, instead of getting an error
from the pooler/flooding the pooler logs/having the pooler close the
connection.

> Third, applications, drivers, and connection poolers now all need to
> worry about handling downgrades smoothly. If a connection pooler
> requests a v3.1 connection to the server and gets v3.0, it had better
> make sure that it only advertises 3.0 to the client.

This seems quite straightforward to solve from a pooler perspective:
1. Connect to the server first to find out what the maximum version is
that it support
2. If a client asks for a higher version, advertise the server version

> If the client
> requests v3.0, the pooler had better make sure to either request v3.0
> from the server. Or alternatively, the pooler can be prepared to
> translate between 3.0 and 3.1 wherever that's needed, in either
> direction. But it's not at all clear what that would look like for
> something like TCE. Will the pooler arrange to encrypt parameters
> destined for encrypted tables if the client doesn't do so? Will it
> arrange to decrypt values coming from encrypted tables if the client
> doesn't understand encryption?

This is a harder problem, and indeed one I hadn't considered much.
>From a pooler perspective I really would like to be able to have just
one pool of connections to the server all initially connected with the
3.1 protocol and use those 3.1 server connections for clients that use
the 3.1 protocol but also for clients that use the 3.0 protocol.
Creating separate pools of server connections for different protocol
versions is super annoying to manage for operators (e.g. how should
these pools be sized). One of the reasons I'm proposing the
ParameterSet message is to avoid this problem for protocol p

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

[Jacob Champion]

On Mon, Apr 22, 2024 at 2:20 PM Jelte Fennema-Nio  wrote:
> 1. I strongly believe minor protocol version bumps after the initial
> 3.1 one can be made painless for clients/poolers (so the ones to
> 3.2/3.3/etc). Similar to how TLS 1.3 can be safely introduced, and not
> having to worry about breaking TLS 1.2 communication.

Apologies for focusing on a single portion of your argument, but this
claim in particular stuck out to me. To my understanding, IETF worried
a _lot_ about breaking TLS 1.2 implementations with the TLS 1.3
change, to the point that TLS 1.3 clients and servers advertise
themselves as TLS 1.2 and sneak the actual version used into a TLS
extension (roughly analogous to the _pq_ stuff). I vaguely recall that
the engineering work done for that update was pretty far from
painless.

--Jacob

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

[Robert Haas]

On Mon, Apr 22, 2024 at 5:19 PM Jelte Fennema-Nio  wrote:
> On Mon, 22 Apr 2024 at 16:26, Robert Haas  wrote:
> > That's a fair point, but I'm still not seeing much practical
> > advantage. It's unlikely that a client is going to set a random bit in
> > a format parameter for no reason.
>
> I think you're missing an important point of mine here. The client
> wouldn't be "setting a random bit in a format parameter for no
> reason". The client would decide it is allowed to set this bit,
> because the PG version it connected to supports column encryption
> (e.g. PG18). But this completely breaks protocol and application layer
> separation.

I can't see what the problem is here. If the client is connected to a
database that contains encrypted columns, and its response to seeing
an encrypted column is to set this bit, that's fine and nothing should
break. If a client doesn't know about encrypted columns and sets that
bit at random, that will break things, and formally I think that's a
risk, because I don't believe we document anywhere that you shouldn't
set unused bits in the format mask. But practically, it's not likely.
(And also, maybe we should document that you shouldn't do that.)

> It doesn't seem completely outside of the realm of possibility for a
> pooler to gather some statistics on the amount of Bind messages that
> use text vs binary query parameters. That's very easily doable now,
> while looking only at the protocol layer. If a client then sets the
> new format parameter bit, this pooler could then get confused and
> close the connection.

Right, this is the kind of risk I was worried about. I think it's
similar to my example of a client setting an unused bit for no reason
and breaking everything. Here, you've hypothesized a pooler that tries
to interpret the bit and just errors out when it sees something it
doesn't understand. I agree that *formally* this is enough to justify
bumping the protocol version, but I think *practically* it isn't,
because the incompatibility is so minor as to inconvenience almost
nobody, whereas changing the protocol version affects everybody.

Let's consider a hypothetical country much like Canada except that
there are three official languages rather than two: English, French,
and Robertish. Robertish is just like English except that the meanings
of the words cabbage and rutabaga are reversed. Shall we mandate that
all signs in the country be printed in three languages rather than
two? Formally, we ought, because the substantial minority of our
hypothetical country that proudly speaks Robertish as their mother
tongue will not want to feel that they are second class citizens. But
practically, there are very few situations where the differences
between the two languages are going to inconvenience anyone. Indeed,
the French speakers might be a bit put out if English is effectively
represented twice on every sign while their mother tongue is there
only once. Of course, people are entitled to organize their countries
politically in any way that works for the people who live in them, but
as a practical matter, English and Robertish are mutually
intelligible.

And so here. If someone codes a connection pooler in the way you
suppose, then it will break. But, first of all, they probably won't do
that, both because it's not particularly likely that someone wants to
gather that particular set of statistics and also because erroring out
seems like an overreaction. And secondly, let's imagine that we do
bump the protocol version and think about whether and how that solves
the problem. A client will request from the pooler a version 3.1
connection and the pooler will say, sorry, no can do, I only
understand 3.0. So the client will now say, oh ok, no problem, I'm
going to refrain from setting that parameter format bit. Cool, right?

Well, no, not really. First, now the client application is probably
broken. If the client is varying its behavior based on the server's
protocol version, that must mean that it cares about accessing
encrypted columns, and that means that the bit in question is not an
optional feature. So actually, the fact that the pooler can force the
client to downgrade hasn't fixed anything at all.

Second, if the connection pooler were written to do something other
than close the connection, like say mask out the one bit that it knows
how to deal with or have an "unknown" bucket to count values that it
doesn't recognize, then it wouldn't have needed to care about the
protocol version in the first place. It would have been better off not
even knowing, because then it wouldn't have forced a downgrade onto
the client application for no real reason. Throwing an error wasn't a
wrong decision on the part of the person writing the pooler, but there
are other things they could have done that would have been less
brittle.

Third, applications, drivers, and connection poolers now all need to
worry about handling downgrades smoothly. If a connection pooler
requests a v3.1 conne

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

[Jelte Fennema-Nio]

On Mon, 22 Apr 2024 at 16:26, Robert Haas  wrote:
> That's a fair point, but I'm still not seeing much practical
> advantage. It's unlikely that a client is going to set a random bit in
> a format parameter for no reason.

I think you're missing an important point of mine here. The client
wouldn't be "setting a random bit in a format parameter for no
reason". The client would decide it is allowed to set this bit,
because the PG version it connected to supports column encryption
(e.g. PG18). But this completely breaks protocol and application layer
separation.

It doesn't seem completely outside of the realm of possibility for a
pooler to gather some statistics on the amount of Bind messages that
use text vs binary query parameters. That's very easily doable now,
while looking only at the protocol layer. If a client then sets the
new format parameter bit, this pooler could then get confused and
close the connection.

> Perhaps this is the root of our disagreement, or at least part of it.
> I completely agree that it is important for human beings to be able to
> understand whether, and how, the wire protocol has changed from one
> release to another.

I think this is partially the reason for our disagreement, but I think
there are at least two other large reasons:

1. I strongly believe minor protocol version bumps after the initial
3.1 one can be made painless for clients/poolers (so the ones to
3.2/3.3/etc). Similar to how TLS 1.3 can be safely introduced, and not
having to worry about breaking TLS 1.2 communication. Once clients and
poolers implement version negotiation support for 3.1, there's no
reason for version negation support to work for 3.0 and 3.1 to then
suddenly break on the 3.2 bump. To be clear, I'm talking about the act
of bumping the version here, not the actual protocol changes. So
assuming zero/near-zero client implementation effort for the new
features (like never setting the newly supported bit in a format
parameter), then bumping the protocol version for these new features
can never have negative consequences.

2. I very much want to keep a clear split between the protocol layer
and the application layer of our communication. And these layers merge
whenever (like you say) "the wire protocol has changed from one
release to another", but no protocol version bump or protocol
extension is used to indicate that. When that happens the only way for
a client to know what valid wire protocol messages are according to
the server, is by checking the server version. This completely breaks
the separation between layers. So, while checking the server version
indeed works for direct client to postgres communication, it starts to
break down whenever you put a pooler inbetween (as explained in the
example earlier in this email). And it breaks down even more when
connecting to servers that implement the Postgres wire protocol, but
are not postgres at all, like CockroachDB. Right now libpq and other
postgres drivers can be used to talk to these other servers and
poolers, but if we start mixing protocol and application layer stuff
then eventually that will stop being the case.

Afaict from your responses, you disagree with 1. However, it's not at
all clear to me what exact problems you're worried about. It sounds
like you don't know either, and it's more that you're worried about
things breaking for not yet known reasons. I hoped to take away/reduce
those worries using some arguments in a previous email (quoted below),
but you didn't respond to those arguments, so I'm not sure if they
were able to change your mind.

On Thu, 18 Apr 2024 at 21:34, Jelte Fennema-Nio  wrote:
> When the server supports a lower version than the client, the client
> should disable certain features because it gets the
> ProtocolVersionNegotiation message. This is also true if we don't bump
> the version. Negotiating a lower version actually makes it clearer for
> the client what features to disable. Using the reported postgres
> version for this might not, because a connection pooler in the middle
> might not support the features that the client wants and thus throw an
> error (e.g. due to the client sending unknown messages) even if the
> backing Postgres server would support these features. Not to mention
> non-postgresql servers that implement the PostgreSQL protocol (of
> which there are more and more).
>
> When the server supports a higher version, the client never even
> notices this because the server will silently accept and only enable
> the features of the lower version. So this could never cause breakage,
> as from the client's perspective the server didn't bump their protocol
> version.

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

[Robert Haas]

On Thu, Apr 18, 2024 at 5:36 PM Jelte Fennema-Nio  wrote:
> To clarify: My proposed approach is to use a protocol extension
> parameter for to enable the new messages that the server can send
> (like Peter is doing now). And **in addition to that** gate the new
> Bind format type behind a feature switch. There is literally nothing
> clients will have to do to "support" that feature (except for
> requesting a higher version protocol). Your suggestion of not bumping
> the version but still allowing the new format type on version 3.0
> doesn't have any advantage afaict, except secretly hiding from any
> pooler in the middle that such a format type might be sent.

That's a fair point, but I'm still not seeing much practical
advantage. It's unlikely that a client is going to set a random bit in
a format parameter for no reason.

> As a connection pooler maintainer I would definitely love it if every
> protocol change required either a protocol version parameter or a
> protocol version bump. That way I can easily check every release if
> the protocol changed by looking at two things, instead of diffing the
> protocol docs for some tiny "supposedly irrelevant" change was made.

Perhaps this is the root of our disagreement, or at least part of it.
I completely agree that it is important for human beings to be able to
understand whether, and how, the wire protocol has changed from one
release to another. I think it would be useful to document that, and
maybe some agreement to start actually bumping the version number
would come out of that, either immediately or eventually. But I don't
think bumping the protocol version first is going to help anything. If
you know that something has changed at least one time in the release,
you still have to figure out what it was, and whether there were any
more of them that, presumably, would not bump the protocol version
because there would be no good reason to do that more than once per
major release. Not only that, but it's entirely possible that someone
could fail to realize that they were supposed to bump the protocol
version, or have some reason not to do it in a particular instance, so
even if there are no bumps at all in a particular release cycle, that
doesn't prove that there are no changes that you would have liked to
know about.

Said differently, I think bumping the protocol version should be,
first and foremost, a way of telling the computer on the end of the
connection something that it needs to know. There is a separate
problem of making sure that human maintainers know what they need to
know, and I think we're doing that quite poorly right now, but I think
you might be conflating those two problems a bit.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

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

[Jelte Fennema-Nio]

On Thu, 18 Apr 2024 at 23:36, Jelte Fennema-Nio  wrote:
> To clarify: My proposed approach is to use a protocol extension
> parameter for to enable the new messages that the server can send
> (like Peter is doing now). And **in addition to that** gate the new
> Bind format type behind a feature switch.

ugh, correction: gate the new Bind format type behind a **protocol bump**

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

[Jelte Fennema-Nio]

On Thu, 18 Apr 2024 at 22:17, Robert Haas  wrote:
> If we go with Peter's approach, every driver that supports his feature
> will work perfectly, and every driver that doesn't will work exactly
> as it does today. The risk of breaking anything is as near to zero as
> human developers can reasonably hope to achieve. Nobody who doesn't
> care about the feature will have to lift a single finger, today,
> tomorrow, or ever. That's absolutely brilliant.
>
> If we instead go with your approach, then anyone who wants to support
> 3.2 when it materializes will have to also support 3.1, which means
> they have to support this feature.

To clarify: My proposed approach is to use a protocol extension
parameter for to enable the new messages that the server can send
(like Peter is doing now). And **in addition to that** gate the new
Bind format type behind a feature switch. There is literally nothing
clients will have to do to "support" that feature (except for
requesting a higher version protocol). Your suggestion of not bumping
the version but still allowing the new format type on version 3.0
doesn't have any advantage afaict, except secretly hiding from any
pooler in the middle that such a format type might be sent.

> Also, even just 3.1 is going to break
> something for somebody. There's just no way that we've left the
> protocol version unchanged for this long and the first change we make
> doesn't cause some collateral damage.

Sure, but the exact same argument holds for protocol extension
parameters. We've never set them, so they are bound to break something
the first time. My whole point is that once we bite that bullet, the
next protocol parameters and protocol version bumps won't cause such
breakage.

> Sure, those are minor downsides in the grand scheme of things. But
> AFAICS the only downside of Peter's approach that you've alleged is
> that doesn't involve bumping the version number. Of course, if bumping
> the version number is an intrinsic good, then no further justification
> is required, but I don't buy that. I do not believe that users or
> maintainers will throw us a pizza party when they find out that we've
> changed the version number. There's no reason for anyone to be happy
> about that for its own sake.

As a connection pooler maintainer I would definitely love it if every
protocol change required either a protocol version parameter or a
protocol version bump. That way I can easily check every release if
the protocol changed by looking at two things, instead of diffing the
protocol docs for some tiny "supposedly irrelevant" change was made.

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

[Robert Haas]

On Thu, Apr 18, 2024 at 3:34 PM Jelte Fennema-Nio  wrote:
> I really don't understand what exactly you're worried about. What do
> you expect will break when bumping the protocol version? As Dave said,
> clients should never bail out due to protocol version differences.

Sure, and I should never forget to take out the trash or mow the lawn.

> So, I don't understand why you seem to view bumping the protocol
> version with so much negativity. We're also bumping PG versions every
> year. Afaik people only like that, partially because it's immediately
> clear that certain features (e.g. MERGE) are not supported when
> connecting to older servers. To me the same is true for bumping the
> protocol version. There are no downsides to bumping it, only upsides.

I see it the exact opposite way around.

If we go with Peter's approach, every driver that supports his feature
will work perfectly, and every driver that doesn't will work exactly
as it does today. The risk of breaking anything is as near to zero as
human developers can reasonably hope to achieve. Nobody who doesn't
care about the feature will have to lift a single finger, today,
tomorrow, or ever. That's absolutely brilliant.

If we instead go with your approach, then anyone who wants to support
3.2 when it materializes will have to also support 3.1, which means
they have to support this feature. That's not a terrible burden, but
it's not a necessary one either. Also, even just 3.1 is going to break
something for somebody. There's just no way that we've left the
protocol version unchanged for this long and the first change we make
doesn't cause some collateral damage.

Sure, those are minor downsides in the grand scheme of things. But
AFAICS the only downside of Peter's approach that you've alleged is
that doesn't involve bumping the version number. Of course, if bumping
the version number is an intrinsic good, then no further justification
is required, but I don't buy that. I do not believe that users or
maintainers will throw us a pizza party when they find out that we've
changed the version number. There's no reason for anyone to be happy
about that for its own sake.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

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

[Jelte Fennema-Nio]

On Mon, 15 Apr 2024 at 21:47, Dave Cramer  wrote:
>> On Mon, 15 Apr 2024 at 19:52, Robert Haas  wrote:
>> > surely it can't be right to use protocol
>> > version 3.0 to refer to a bunch of different things. But at the same
>> > time, surely we don't want clients to start panicking and bailing out
>> > when everything would have been fine.
>>
>> I feel like the ProtocolVersionNegotiation should make sure people
>> don't panic and bail out. And if not, then feature flags won't help
>> with this either. Because clients would then still bail out if some
>> feature is not supported.
>
> I don't think a client should ever bail out. They may not support something 
> but IMO bailing out is not an option.


On Thu, 18 Apr 2024 at 21:01, Robert Haas  wrote:
> On Thu, Apr 18, 2024 at 1:49 PM Jelte Fennema-Nio  wrote:
> > IMHO that means that we should also bump the protocol version for this
> > change, because it's changing the wire protocol by adding a new
> > parameter format code. And it does so in a way that does not depend on
> > the new protocol extension.
>
> I think we're more or less covering the same ground we did on the
> other thread here -- in theory I don't love the fact that we never
> bump the protocol version when we change stuff, but in practice if we
> start bumping it every time we do anything I think it's going to just
> break a bunch of stuff without any real benefit.

(the second quoted message comes from Peter his column encryption
thread, but responding here to keep this discussion in one place)

I really don't understand what exactly you're worried about. What do
you expect will break when bumping the protocol version? As Dave said,
clients should never bail out due to protocol version differences.

When the server supports a lower version than the client, the client
should disable certain features because it gets the
ProtocolVersionNegotiation message. This is also true if we don't bump
the version. Negotiating a lower version actually makes it clearer for
the client what features to disable. Using the reported postgres
version for this might not, because a connection pooler in the middle
might not support the features that the client wants and thus throw an
error (e.g. due to the client sending unknown messages) even if the
backing Postgres server would support these features. Not to mention
non-postgresql servers that implement the PostgreSQL protocol (of
which there are more and more).

When the server supports a higher version, the client never even
notices this because the server will silently accept and only enable
the features of the lower version. So this could never cause breakage,
as from the client's perspective the server didn't bump their protocol
version.

So, I don't understand why you seem to view bumping the protocol
version with so much negativity. We're also bumping PG versions every
year. Afaik people only like that, partially because it's immediately
clear that certain features (e.g. MERGE) are not supported when
connecting to older servers. To me the same is true for bumping the
protocol version. There are no downsides to bumping it, only upsides.

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

[Dave Cramer]

On Mon, 15 Apr 2024 at 15:38, Jelte Fennema-Nio  wrote:

> On Mon, 15 Apr 2024 at 19:43, Robert Haas  wrote:
> >
> > On Sat, Apr 6, 2024 at 6:14 PM Jelte Fennema-Nio  wrote:
> > > I think for clients/drivers, the work would generally be pretty
> > > minimal. For almost all proposed changes, clients can "support" the
> > > protocol version update by simply not using the new features, ...
> >
> > I mean, I agree if a particular protocol version bump does nothing
> > other than signal the presence of some optional, ignorable feature,
> > then it doesn't cause a problem if we force clients to support it. But
> > that seems a bit like saying that eating wild mushrooms is fine
> > because some of them aren't poisonous. The point is that if we roll
> > out two protocol changes, A and B, each of which requires the client
> > to make some change in order to work with the newer protocol version,
> > then using version numbers as the gating mechanism requires that the
> > client can't support the newer of those two changes without also
> > supporting the older one. Using feature flags doesn't impose that
> > constraint, which I think is a plus.
>
> I think we're in agreement here, i.e. it depends on the situation if a
> feature flag or version bump is more appropriate. I think the
> guidelines could be as follows:
> 1. For protocol changes that require "extremely minimal" work from
> clients & poolers: bump the protocol version.
> 2. For "niche" features that require some work from clients and/or
> poolers: use a protocol parameter feature flag.
> 3. For anything in between, let's discuss on the thread for that
> specific protocol change on the tradeoffs.
>

My first thought here is that all of the above is subjective and we will
end up discussing all of the above.
No real argument just an observation.

>
> On Mon, 15 Apr 2024 at 19:52, Robert Haas  wrote:
> > surely it can't be right to use protocol
> > version 3.0 to refer to a bunch of different things. But at the same
> > time, surely we don't want clients to start panicking and bailing out
> > when everything would have been fine.
>
> I feel like the ProtocolVersionNegotiation should make sure people
> don't panic and bail out. And if not, then feature flags won't help
> with this either. Because clients would then still bail out if some
> feature is not supported.
>

I don't think a client should ever bail out. They may not support something
but IMO bailing out is not an option.

Dave

>
>

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

[Jelte Fennema-Nio]

On Mon, 15 Apr 2024 at 19:43, Robert Haas  wrote:
>
> On Sat, Apr 6, 2024 at 6:14 PM Jelte Fennema-Nio  wrote:
> > I think for clients/drivers, the work would generally be pretty
> > minimal. For almost all proposed changes, clients can "support" the
> > protocol version update by simply not using the new features, ...
>
> I mean, I agree if a particular protocol version bump does nothing
> other than signal the presence of some optional, ignorable feature,
> then it doesn't cause a problem if we force clients to support it. But
> that seems a bit like saying that eating wild mushrooms is fine
> because some of them aren't poisonous. The point is that if we roll
> out two protocol changes, A and B, each of which requires the client
> to make some change in order to work with the newer protocol version,
> then using version numbers as the gating mechanism requires that the
> client can't support the newer of those two changes without also
> supporting the older one. Using feature flags doesn't impose that
> constraint, which I think is a plus.

I think we're in agreement here, i.e. it depends on the situation if a
feature flag or version bump is more appropriate. I think the
guidelines could be as follows:
1. For protocol changes that require "extremely minimal" work from
clients & poolers: bump the protocol version.
2. For "niche" features that require some work from clients and/or
poolers: use a protocol parameter feature flag.
3. For anything in between, let's discuss on the thread for that
specific protocol change on the tradeoffs.

On Mon, 15 Apr 2024 at 19:52, Robert Haas  wrote:
> surely it can't be right to use protocol
> version 3.0 to refer to a bunch of different things. But at the same
> time, surely we don't want clients to start panicking and bailing out
> when everything would have been fine.

I feel like the ProtocolVersionNegotiation should make sure people
don't panic and bail out. And if not, then feature flags won't help
with this either. Because clients would then still bail out if some
feature is not supported.

> I'm unconvinced that we should let ParameterSet change
> non-PGC_PROTOCOL GUCs. The pooler can agree on a list of protocol GUCs
> with the end client that differs from what the server agreed with the
> pooler - e.g., it can add pgbouncer.pool_mode to the list. But for
> truly non-protocol GUCs, we have a lot of ways to set those already.

I feel like you're glossing over something fairly important here. How
exactly would the client know about pgbouncer.pool_mode? Are you
envisioning a list of GUCs which can be changed using ParameterSet,
which the server then sends to the client during connection startup
(using presumably some new protocol message)? If so, then I feel this
same problem still exists. How would the client know which of those
GUCs change wire-protocol behaviour and which don't? It still would
need a hardcoded list (now including pgbouncer.pool_mode and maybe
more) of things that a user is allowed to change using ParameterSet.
So I think a well-known prefix would still be applicable.

To be clear, imho the well-known prefix discussion is separate from
the discussion about whether Postgres should throw an ERROR when
ParameterSet is used to change any non-PGC_PROTOCOL GUC. I'd be fine
with disallowing that if that seems better/safer/clearer to you
(although I'd love to hear your exact concerns about this). But I'd
still want a well-known prefix for protocol parameters. Because that
prefix is not for the benefit of the server, it's for the benefit of
the client and pooler. So the client/pooler can error if any dangerous
GUC is being changed, because the server would accept it and change
the wire-protocol accordingly.

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

[Robert Haas]

[ Hit send too early, sorry. ]

On Mon, Apr 15, 2024 at 1:43 PM Robert Haas  wrote:
> But the wire protocol changes very slowly, and I think that is a
> difference that actually matters quite a bit here. Broadly speaking, I
> can use a psq

...a psql that I just built today to talk to a server from many years
ago, and everything is fine. Sure, there are some marginal wire
protocol changes around the edges, but not in places that are going to
really affect the ability of psql to communicate. But I wouldn't try
to run a psql built against 16 with a libpq even one major release
old, and the other direction (older psql, newer libpq) also carries
some (albeit fewer) risks. So the two situations aren't really
entirely comparable, I feel. I don't quite know what to make of that
as a practical matter: surely it can't be right to use protocol
version 3.0 to refer to a bunch of different things. But at the same
time, surely we don't want clients to start panicking and bailing out
when everything would have been fine.

> > To take it to the extreme: I think we should get to a state, where if
> > we bump the protocol version at the client and server side without
> > actually making any protocol changes, everything should continue to
> > work fine. If we'd do that right now, then libpq wouldn't be able to
> > connect to old postgres versions anymore.
>
> I think I agree with this, but it seems like a bootstrapping problem
> and nothing more.

That is, once we figure out how we want backward compatibility to work
in general, I think we'll probably get pretty close to the state you
want here pretty quickly.

> > Clients might want to allow the user of the client to change regular
> > parameters using ParameterSet (e.g. so that a connection pooler can
> > intercept those ParameterSet messages and change its own behaviour if
> > the parameter name is pgbouncer.pool_mode). But they wouldn't want a
> > user to set any parameters that change the wire-protocol this way. And
> > because an old client might connect to a new server a simple
> > hard-coded list of parameters at the client side is not sufficient.
> >
> > I can see two ways around this:
> > 1. Using a well-known prefix or namespace for parameters that change
> > the wire protocol. (exact prefix to be bikeshedded on)
> > 2. Using a hard-coded list at the client AND disallow changing
> > PGC_PROTOCOL parameters at the server if the negotiated protocol
> > version is lower than the version this parameter was introduced in AND
> > bump the protocol version whenever we add a new PGC_PROTOCOL
> > parameter.
> >
> > I think 1 is easier to implement at the client side, as it only
> > requires a prefix comparison instead of keeping track of a list.

I'm unconvinced that we should let ParameterSet change
non-PGC_PROTOCOL GUCs. The pooler can agree on a list of protocol GUCs
with the end client that differs from what the server agreed with the
pooler - e.g., it can add pgbouncer.pool_mode to the list. But for
truly non-protocol GUCs, we have a lot of ways to set those already.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

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

[Robert Haas]

On Sat, Apr 6, 2024 at 6:14 PM Jelte Fennema-Nio  wrote:
> I think for clients/drivers, the work would generally be pretty
> minimal. For almost all proposed changes, clients can "support" the
> protocol version update by simply not using the new features, ...

I mean, I agree if a particular protocol version bump does nothing
other than signal the presence of some optional, ignorable feature,
then it doesn't cause a problem if we force clients to support it. But
that seems a bit like saying that eating wild mushrooms is fine
because some of them aren't poisonous. The point is that if we roll
out two protocol changes, A and B, each of which requires the client
to make some change in order to work with the newer protocol version,
then using version numbers as the gating mechanism requires that the
client can't support the newer of those two changes without also
supporting the older one. Using feature flags doesn't impose that
constraint, which I think is a plus.

> I think there's an important trade-off here. On one side we don't want
> to make maintainers of clients/poolers do lots of work to support
> features they don't care about. And on the other side it seems quite
> useful to limit the amount of feature combinations that are used it
> the wild (both for users and for us) e.g. the combinations of
> backwards compatibility testing you were talking about would explode
> if every protocol change was a feature flag.

This is a good point, and I agree. It's a little hard to discuss this
in the abstract. I wouldn't want to have feature flags that say:

1. I support rows containing an extra-large number of bytes.
2. I support rows containing an extra-large number of columns.
3. I support queries returning an extra-large number of rows.

It is quite likely that there might be bugs that only manifest with
particular combinations of those flags; knowing that someone who
supports (3) must also support (1) and (2) seems like it would make
everyone's life easier. But on the other hand, I wouldn't mind having
feature flags that say:

1. I support transparent column encryption.
2. I support letting a connection pooler lock down the session role in
a way that can't be reversed from SQL.
3. I support compression.

It's still not theoretically impossible that those features could
interact with each other in some way, but it seems a lot less likely.
Here, I think a driver ought to be able to choose any subset of these
things and support only the ones they care about, without having to
worry about the server deciding to do something for which the driver
is unprepared. Also, in a case like this, if there are bugs that only
occur in certain combinations, we have to test for and fix those
anyway, because even if a driver supports all of those features,
they're not all going to be used for every connection.

> I think there's two parts to a protocol version bump:
> 1. TI ahe changes that cause us to consider a protocol bump
> 2. The actual act of bumping the protocol version
>
> I think 1 is a thing we should be careful about every time (especially
> regarding impact on clients/poolers). But 2 shouldn't be something
> that we should consider dangerous/scary. I think that every change
> that we make to the protocol (no matter how minor or backwards
> compatible it is), should be accompanied with a protocol version bump.
> This isn't what has happened in the past, and it makes it quite hard
> to understand what "supporting" a specific protocol version actually
> means. e.g. PgBouncer currently supports protocol 3.0, but doesn't
> support the NegotiateProtocolVersion message (I'm working on fixing
> that).

I'm generally not a fan of giving things version numbers and then not
changing the version numbers when you change the thing, but I find
myself reluctant to apply that principle to this case. I think it's
bad that we keep adding functions to libpq and sometimes changing the
behavior of existing functions and never, ever bumping the libpq .so
version. I've seen that cause real, practical problems. It means for
example that you can't make an RPM depend on libpq.so.5 and expect
that to do anything meaningful -- every version going back forever is
version 5, even if it doesn't contain the functions (or other behavior
changes) that are needed for some program compiled against a newer
version of PostgreSQL to work.

But the wire protocol changes very slowly, and I think that is a
difference that actually matters quite a bit here. Broadly speaking, I
can use a psq

> To take it to the extreme: I think we should get to a state, where if
> we bump the protocol version at the client and server side without
> actually making any protocol changes, everything should continue to
> work fine. If we'd do that right now, then libpq wouldn't be able to
> connect to old postgres versions anymore.

I think I agree with this, but it seems like a bootstrapping problem
and nothing more.

>
> > I'm not sure what I think about this. Do we need these

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

[Peter Eisentraut]

On 05.04.24 14:55, Robert Haas wrote:

I also wonder how the protocol negotiation for column encryption is
actually going to work. What are the actual wire protocol changes that
are needed? What does the server need to know from the client, or the
client from the server, about what is supported?


I have just posted an updated patch for that: [0]

The protocol changes can be inspected in the diffs for

doc/src/sgml/protocol.sgml
src/backend/access/common/printtup.c
src/interfaces/libpq/fe-protocol3.c

There are various changes, including new messages, additional fields in 
existing messages, and some more flag bits in existing fields.


It all works, so I don't have any requests or anything in this thread, 
but it would be good to get some feedback if I'm using this wrong. 
AFAICT, that patch was the first public one that ever tried to make use 
of the protocol extension facility, so I was mainly guessing about the 
intended way to use this.



[0]: 
https://www.postgresql.org/message-id/f63fe170-cef2-4914-be00-ef9222456505%40eisentraut.org

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

[Jelte Fennema-Nio]

On Fri, 5 Apr 2024 at 18:48, Robert Haas  wrote:
> Maybe we'd be better off adding a libpq connection
> option that forces the use of a specific minor protocol version, but
> then we'll need backward-compatibility code in libpq basically
> forever. But maybe we need that anyway to avoid older and newer
> servers being unable to communicate.

I think this would be good because it makes testing easy and, like you
said, I think we'll need this backward-compatibility code in libpq
anyway to be able to connect to old servers. To have even better and
more realistic test coverage though, I think we might also want to
actually test new libpq against old postgres servers and vice-versa in
a build farm animal though.

> Plus, you've got all of the consequences for non-core drivers, which
> have to both add support for the new wire protocol - if they don't
> want to seem outdated and eventually obsolete - and also test that
> they're still compatible with all supported server versions.

I think for clients/drivers, the work would generally be pretty
minimal. For almost all proposed changes, clients can "support" the
protocol version update by simply not using the new features, e.g. a
client can "support" the ParameterSet feature, by simply never sending
the ParameterSet message. So binding it to a protocol version bump
doesn't make it any harder for that client to support that protocol
version. I'm not saying that is the case for all protocol changes, but
based on what's being proposed so far that's definitely a very common
theme. Overall, I think this is something to discuss for each protocol
change in isolation: i.e. how to make supporting the new feature as
painless as possible for clients/drivers.

> Connection poolers have the same set of problems.

For connection poolers this is indeed a bigger hassle, because they at
least need to be able to handle all the new message types that a
client can send and maybe do something special for them. But I think
if we're careful to keep connection poolers in mind when designing the
features themselves then I think this isn't necessarily a problem. And
probably indeed for the features that we think are hard for connection
poolers to implement, we should be using protocol extension parameter
feature flags. But I think a lot of protocol would be fairly trivial
for a connection pooler to support.

> The whole thing is
> almost a hole with no bottom. Keeping up with core changes in this
> area could become a massive undertaking for lots and lots of people,
> some of whom may be the sole maintainer of some important driver that
> now needs a whole bunch of work.

I agree with Dave here, if you want to benefit from new features
there's some expectation to keep up with the changes. But to be clear,
we'd still support old protocol versions too. So we wouldn't break
connecting using those clients, they simply wouldn't benefit from some
of the new features. I think that's acceptable.

> I'm not sure how much it improves things if we imagine adding feature
> flags to the existing protocol versions, rather than whole new
> protocol versions, but at least it cuts down on the assumption that
> adopting new features is mandatory, and that such features are
> cumulative. If a driver wants to support TDE but not protocol
> parameters or protocol parameters but not TDE, who are we to say no?
> If in supporting those things we bump the protocol version to 3.2, and
> then 3.3 fixes a huge performance problem, are drivers going to be
> required to add support for features they don't care about to get the
> performance fixes?

I think there's an important trade-off here. On one side we don't want
to make maintainers of clients/poolers do lots of work to support
features they don't care about. And on the other side it seems quite
useful to limit the amount of feature combinations that are used it
the wild (both for users and for us) e.g. the combinations of
backwards compatibility testing you were talking about would explode
if every protocol change was a feature flag. I think this trade-off is
something we should be deciding on based on the specific protocol
change. But if work needed to "support" the feature is "minimal"
(to-be-defined exactly what we consider minimal), I think making it
part of a protocol version bump is reasonable.

> I see some benefit in bumping the protocol version
> for major changes, or for changes that we have an important reason to
> make mandatory, or to make previously-optional features for which
> support has become in practical terms universal part of the base
> feature set. But I'm very skeptical of the idea that we should just
> handle as many things as possible via a protocol version bump. We've
> been avoiding protocol version bumps like the plague since forever,
> and swinging all the way to the other extreme doesn't sound like the
> right idea to me.

I think there's two parts to a protocol version bump:
1. The changes that cause us to consider a protocol bump