Обсуждение: [HACKERS] Re: protocol version negotiation (Re: Libpq PGRES_COPY_BOTH - versioncompatibility)
[HACKERS] Re: protocol version negotiation (Re: Libpq PGRES_COPY_BOTH - versioncompatibility)
От
Badrul Chowdhury
Дата:
Hello,
Kindly review this patch that implements the proposal for pgwire protocol negotiation described in this thread.
A big thanks to @Satyanarayana Narlapuram for his help and guidance in implementing the patch.
Please note:
1. Pgwire protocol v3.0 with negotiation is called v3.1.
2. There are 2 patches for the change: a BE-specific patch that will be backported and a FE-specific patch that is only for pg10 and above.
/*************************/
/* Feature In A Nutshell */
/*************************/
This feature enables a newer FE to connect to an older BE using a protocol negotiation phase in the connection setup. The FE sends its protocol version and a list of startup parameters to the BE to start a connection. If the FE pgwire version is newer than the BE's or if the BE could not recognize some of the startup parameters,
the BE simply ignores the parameters that it did not recognize and sends a ServerProtocolVersion message containing the minimum and maximum versions it supports as well as a list of the parameters that it did honour. The FE then decides whether it wants to continue connecting- in the patch, the FE continues only if the parameters that the BE did not honour were optional, where optional parameters are denoted by a proper prefix of "_pq_" in their names.
/************/
/* BE Patch */
/************/
Files modified:
src/
└── backend/
└── postmaster/
└── postmaster.c
└── utils/misc/
└── guc.c
src/
└── include/postmaster/
└── postmaster.h
Design decisions:
1. The BE sends a message type of ServerProtocolVersion if the FE is newer or the FE is not using v3.0
1.a Message structure: [ 'Y' | msgLength | min_version | max_version | param_list_len | list of param names ]
1.b 'Y' was selected to denote ServerProtocolVersion messages as it is the first available character from the bottom of the list of available characters that is not being used to denote any message type at present.
2. Added support for BE to accept optional parameters
2.a An optional parameter is a startup parameter with "_pq_" as a proper prefix in its name. The check to add a placeholder in /src/backend/utils/misc/guc.c was modified to allow optional parameters.
2.b The string comparison in is_optional() is encoding-aware as the BE's encoding might be different from the FE's.
/************/
/* FE Patch */
/************/
Files modified:
src/
└── Makefile
src/
└── Makefile.global.in
src/
└── bin/
└── pg_dump/
└── pg_dump.c
└── scripts/
├── clusterdb.c
├── createuser.c
├── reindexdb.c
└── vacuumdb.c
src/
└── common/
└── Makefile
src/
└── fe_utils/
└── Makefile
└── simple_list.c
src/
└── include/
└── fe_utils/
└── simple_list.h
└── libpq/
└── pqcomm.h
src/
└── interfaces/libpq/
├── fe-connect.c
├── fe-protocol3.c
├── libpq-fe.h
└── libpq-int.h
src/
└── tools/msvc/
└── Mkvcbuild.pm
Design decisions:
1. Added mechanism to send startup parameters to BE:
SimpleStringList startup_parameters in /src/interfaces/libpq/fe-connect.c enables sending parameters to the BE. The startup_parameters list is parsed in /src/interfaces/libpq/fe-protocol3.c and the arguments are sent in the startup packet to the BE.
This is mostly used for testing at this point- one would send additional startup parameters like so in PQconnectStartParams():
simple_string_list_append(&startup_parameters, "_pq_A");
simple_string_list_append(&startup_parameters, "1");
startup_parameters.immutable = true; // PQconnectStartParams() is called twice for the same connection; the immutable flag ensures that the same parameter is not added twice
2. Added a CONNECTION_NEGOTIATING state in /src/interfaces/libpq/fe-connect.c to parse the ServerProtocolVersion message from the BE
2.a The FE terminates the connection if BE rejected non-optional parameters.
3. The changes to the following files were due to the addition of the immutable field in the SimpleStringList struct
3.a /src/bin/pg_dump/pg_dump.c
3.b /src/bin/scripts/clusterdb.c
3.c /src/bin/scripts/createuser.c
3.d /src/bin/scripts/reindexdb.c
3.e /src/bin/scripts/vacuumdb.c
4. Added some dependencies to the libpq project to be able to use SimpleStringList in fe-connect.c.
4.a Visual C compiler
4.a.1 /src/tools/msvc/Mkvcbuild.pm was modified to add the reference for visual c compiler
4.b Non-windows compilers: the following files were modified
4.b.1 /src/Makefile
4.b.2 /src/Makefile.global.in
4.b.3 /src/common/Makefile
4.b.4 /src/fe_utils/Makefile
5. Bumped max_pg_protocol version from 3.0 to 3.1 in /src/include/libpq/pqcomm.h
/***********/
/* Testing */
/***********/
An outline of the testing framework used to validate the code:
1. Newer FE can connect to older BE
1.a Change FE protocol version in line 1811 of /src/interfaces/libpq/fe-connect.c
1.a.1 conn->pversion = PG_PROTOCOL(3, 1); succeeds now whereas it would have failed before: a result of bumping max_pg_protocol from 3.0 to 3.1
1.a.2 conn->pversion = PG_PROTOCOL(4, 0); succeds now whereas it would have failed before: older BE is capable of handling newer FE now
2. Older drivers work as expected
2.a Change FE protocol version in line 1811 of /src/interfaces/libpq/fe-connect.c
2.a.1 conn->pversion = PG_PROTOCOL(1, 9); fails now and failed before as well as it is below min_pg_protocol=PG_PROTOCOL(2, 0)
2.a.2 conn->pversion = PG_PROTOCOL(2, 0); succeeds now and succeeded before as well as it is in the supported range PG_PROTOCOL(2, 0) to PG_PROTOCOL(3, 1)
2.a.3 conn->pversion = PG_PROTOCOL(3, 0); succeeds now and succeeded before as well as it is in the supported range PG_PROTOCOL(2, 0) to PG_PROTOCOL(3, 1)
3. BE support for optional parameters
3.a Accept names with "_pq_" as proper prefix, eg "_pq_A" would succeed
3.b Reject all others without crashing
3.b.1 Any string not like "_pq_A" should fail
Looking forward to your feedback,
Thank you,
Badrul Chowdhury
From: pgsql-hackers-owner@postgresql.org [mailto:pgsql-hackers-owner@postgresql.org] On Behalf Of Robert Haas
Sent: Sunday, July 2, 2017 3:45 PM
To: Satyanarayana Narlapuram <Satyanarayana.Narlapuram@microsoft.com>
Cc: Tom Lane <tgl@sss.pgh.pa.us>; Craig Ringer <craig@2ndquadrant.com>; Peter Eisentraut <peter_e@gmx.net>; Magnus Hagander <magnus@hagander.net>; PostgreSQL-development <pgsql-hackers@postgresql.org>
Subject: Re: protocol version negotiation (Re: Libpq PGRES_COPY_BOTH - version compatibility)
On Thu, Jun 29, 2017 at 7:29 PM, Satyanarayana Narlapuram
<Satyanarayana.Narlapuram@microsoft.com> wrote:
> -----Original Message-----
The formatting of this message differs from the style normally used on
this mailing list, and is hard to read.
> 2. If the client version is anything other than 3.0, the server responds with a ServerProtocolVersion indicating the highest version it supports, and ignores any pg_protocol.<whatever> options not known to it as being either third-party extensions or something from a future version. If the initial response to the startup message is anything other than a ServerProtocolVersion message, the client should assume it's talking to a 3.0 server. (To make this work, we would back-patch a change into existing releases to allow any 3.x protocol version and ignore any pg_protocol.<whatever> options that were specified.)
>
> We can avoid one round trip if the server accepts the startupmessage as is (including understanding all the parameters supplied by the client), and in the cases where server couldn’t accept the startupmessage / require negotiation, it should send ServerProtocolVersion message that contains both MIN and MAX versions it can support. Providing Min version helps server enforce the client Min protocol version, and provides a path to deprecate older versions. Thoughts?
With this latest proposal, there are no extra round-trips anyway. I
don't much see the point of having the server advertise a minimum
supported version. The idea of new minor protocol versions is to add
*optional* features, so there shouldn't be an issue with the client
being too old to talk to the server altogether. Of course, the server
might be configured to reject the client unless some particular new
feature is in use, but that's best handled by a message about the
specific problem at hand rather than a generic complaint.
> Does the proposal also include the client can negotiate the protocol version on the same connection rather than going through connection setup process again? The state machine may not sound simple with this proposal but helps bringing down total time taken for the login.
Nothing in that proposal involved an extra connection setup process;
if that's not clear, you might want to reread it.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Вложения
Badrul Chowdhury <bachow@microsoft.com> writes:
> 1. Pgwire protocol v3.0 with negotiation is called v3.1.
> 2. There are 2 patches for the change: a BE-specific patch that will be backported and a FE-specific patch that is
onlyfor pg10 and above.
TBH, anything that presupposes a backported change in the backend is
broken by definition. We expect libpq to be able to connect to older
servers, and that has to include servers that didn't get this memo.
It would be all right for libpq to make a second connection attempt
if its first one fails, as we did in the 2.0 -> 3.0 change.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, Oct 3, 2017 at 9:46 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Badrul Chowdhury <bachow@microsoft.com> writes: >> 1. Pgwire protocol v3.0 with negotiation is called v3.1. >> 2. There are 2 patches for the change: a BE-specific patch that will be backported and a FE-specific patch that is onlyfor pg10 and above. > > TBH, anything that presupposes a backported change in the backend is > broken by definition. We expect libpq to be able to connect to older > servers, and that has to include servers that didn't get this memo. > > It would be all right for libpq to make a second connection attempt > if its first one fails, as we did in the 2.0 -> 3.0 change. Hmm, that's another approach, but I prefer the one advocated by Tom Lane. https://www.postgresql.org/message-id/30788.1498672033@sss.pgh.pa.us https://www.postgresql.org/message-id/24357.1498703265%40sss.pgh.pa.us -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: protocol version negotiation (Re: LibpqPGRES_COPY_BOTH - version compatibility)
От
Badrul Chowdhury
Дата:
Okay, I will add a mechanism to try connecting with 3.0 if 3.1 fails- that should be a few lines of code fe-connect.c; thiswill eliminate the need for a back-patch. What do you think of the rest of the change? Thanks, Badrul -----Original Message----- From: Robert Haas [mailto:robertmhaas@gmail.com] Sent: Wednesday, October 4, 2017 4:54 AM To: Tom Lane <tgl@sss.pgh.pa.us> Cc: Badrul Chowdhury <bachow@microsoft.com>; Satyanarayana Narlapuram <Satyanarayana.Narlapuram@microsoft.com>; Craig Ringer<craig@2ndquadrant.com>; Peter Eisentraut <peter_e@gmx.net>; Magnus Hagander <magnus@hagander.net>; PostgreSQL-development<pgsql-hackers@postgresql.org> Subject: Re: [HACKERS] Re: protocol version negotiation (Re: Libpq PGRES_COPY_BOTH - version compatibility) On Tue, Oct 3, 2017 at 9:46 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Badrul Chowdhury <bachow@microsoft.com> writes: >> 1. Pgwire protocol v3.0 with negotiation is called v3.1. >> 2. There are 2 patches for the change: a BE-specific patch that will be backported and a FE-specific patch that is onlyfor pg10 and above. > > TBH, anything that presupposes a backported change in the backend is > broken by definition. We expect libpq to be able to connect to older > servers, and that has to include servers that didn't get this memo. > > It would be all right for libpq to make a second connection attempt if > its first one fails, as we did in the 2.0 -> 3.0 change. Hmm, that's another approach, but I prefer the one advocated by Tom Lane. https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.postgresql.org%2Fmessage-id%2F30788.1498672033%40sss.pgh.pa.us&data=02%7C01%7Cbachow%40microsoft.com%7Cd183fe16a3a445f4bc7c08d50b1e9e9e%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636427148510331370&sdata=jLwhk6twUrlsm9K6yLronVvg%2Fjx93MM37UXm6NndfLY%3D&reserved=0 https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.postgresql.org%2Fmessage-id%2F24357.1498703265%2540sss.pgh.pa.us&data=02%7C01%7Cbachow%40microsoft.com%7Cd183fe16a3a445f4bc7c08d50b1e9e9e%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636427148510331370&sdata=gtFfNcxR3qK7rzieQQ0EAOFn%2BsDsw8rjtQeWwyIv6EY%3D&reserved=0 -- Robert Haas EnterpriseDB: https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.enterprisedb.com&data=02%7C01%7Cbachow%40microsoft.com%7Cd183fe16a3a445f4bc7c08d50b1e9e9e%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636427148510331370&sdata=wf9cTkQEnRzkdaZxZ1D6NBY9kZbiViyni5lkA7nzEXM%3D&reserved=0 The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: protocol version negotiation (Re: LibpqPGRES_COPY_BOTH - version compatibility)
От
Badrul Chowdhury
Дата:
Hi Tom and Robert, I added a mechanism to fall back to v3.0 if the BE fails to start when FE initiates a connection with v3.1 (with optionalstartup parameters). This completely eliminates the need to backpatch older servers, ie newer FE can connect to olderBE. Please let me know what you think. Thanks, Badrul -----Original Message----- From: Robert Haas [mailto:robertmhaas@gmail.com] Sent: Wednesday, October 4, 2017 4:54 AM To: Tom Lane <tgl@sss.pgh.pa.us> Cc: Badrul Chowdhury <bachow@microsoft.com>; Satyanarayana Narlapuram <Satyanarayana.Narlapuram@microsoft.com>; Craig Ringer<craig@2ndquadrant.com>; Peter Eisentraut <peter_e@gmx.net>; Magnus Hagander <magnus@hagander.net>; PostgreSQL-development<pgsql-hackers@postgresql.org> Subject: Re: [HACKERS] Re: protocol version negotiation (Re: Libpq PGRES_COPY_BOTH - version compatibility) On Tue, Oct 3, 2017 at 9:46 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Badrul Chowdhury <bachow@microsoft.com> writes: >> 1. Pgwire protocol v3.0 with negotiation is called v3.1. >> 2. There are 2 patches for the change: a BE-specific patch that will be backported and a FE-specific patch that is onlyfor pg10 and above. > > TBH, anything that presupposes a backported change in the backend is > broken by definition. We expect libpq to be able to connect to older > servers, and that has to include servers that didn't get this memo. > > It would be all right for libpq to make a second connection attempt if > its first one fails, as we did in the 2.0 -> 3.0 change. Hmm, that's another approach, but I prefer the one advocated by Tom Lane. https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.postgresql.org%2Fmessage-id%2F30788.1498672033%40sss.pgh.pa.us&data=02%7C01%7Cbachow%40microsoft.com%7Cd183fe16a3a445f4bc7c08d50b1e9e9e%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636427148510331370&sdata=jLwhk6twUrlsm9K6yLronVvg%2Fjx93MM37UXm6NndfLY%3D&reserved=0 https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.postgresql.org%2Fmessage-id%2F24357.1498703265%2540sss.pgh.pa.us&data=02%7C01%7Cbachow%40microsoft.com%7Cd183fe16a3a445f4bc7c08d50b1e9e9e%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636427148510331370&sdata=gtFfNcxR3qK7rzieQQ0EAOFn%2BsDsw8rjtQeWwyIv6EY%3D&reserved=0 -- Robert Haas EnterpriseDB: https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.enterprisedb.com&data=02%7C01%7Cbachow%40microsoft.com%7Cd183fe16a3a445f4bc7c08d50b1e9e9e%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636427148510331370&sdata=wf9cTkQEnRzkdaZxZ1D6NBY9kZbiViyni5lkA7nzEXM%3D&reserved=0 The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Вложения
On Fri, Oct 6, 2017 at 5:07 PM, Badrul Chowdhury <bachow@microsoft.com> wrote:
> I added a mechanism to fall back to v3.0 if the BE fails to start when FE initiates a connection with v3.1 (with
optionalstartup parameters). This completely eliminates the need to backpatch older servers, ie newer FE can connect to
olderBE. Please let me know what you think.
Well, I think it needs a good bit of cleanup before we can really get
to the substance of the patch.
+ fe_utils \ interfaces \ backend/replication/libpqwalreceiver \ backend/replication/pgoutput \
- fe_utils \
Useless change, omit.
+ if (whereToSendOutput != DestRemote ||
+ PG_PROTOCOL_MAJOR(FrontendProtocol) < 3)
+ return -1;
+
+ int sendStatus = 0;
Won't compile on older compilers. We generally aim for C89
compliance, with a few exceptions for newer features.
Also, why initialize sendStatus and then overwrite the value in the
very next line of code?
Also, the PG_PROTOCOL_MAJOR check here seems to be redundant with the
one in the caller.
+ /* NegotiateServerProtocol packet structure
+ *
+ * [ 'Y' | msgLength | min_version | max_version | param_list_len
| list of param names ]
+ */
+
Please pgindent your patches. I suspect you'll find this gets garbled.
Is there really any reason to separate NegotiateServerProtocol and
ServerProtocolVersion into separate functions?
-libpq = -L$(libpq_builddir) -lpq
+libpq = -L$(libpq_builddir) -lpq -L$(top_builddir)/src/common
-lpgcommon -L$(top_builddir)/src/fe_utils -lpgfeutils
+ $libpq->AddReference($libpgcommon, $libpgfeutils, $libpgport);
I haven't done any research to try to figure out why you did this, but
I don't think these are likely to be acceptable changes.
SendServerProtocolVersionMessage should be adjusted to use the new
facilities added by commit 1de09ad8eb1fa673ee7899d6dfbb2b49ba204818.
- /* Check we can handle the protocol the frontend is using. */
-
- if (PG_PROTOCOL_MAJOR(proto) < PG_PROTOCOL_MAJOR(PG_PROTOCOL_EARLIEST) ||
- PG_PROTOCOL_MAJOR(proto) > PG_PROTOCOL_MAJOR(PG_PROTOCOL_LATEST) ||
- (PG_PROTOCOL_MAJOR(proto) == PG_PROTOCOL_MAJOR(PG_PROTOCOL_LATEST) &&
- PG_PROTOCOL_MINOR(proto) > PG_PROTOCOL_MINOR(PG_PROTOCOL_LATEST)))
- ereport(FATAL,
- (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("unsupported frontend protocol %u.%u: server supports %
u.0 to %u.%u",
- PG_PROTOCOL_MAJOR(proto), PG_PROTOCOL_MINOR(proto),
- PG_PROTOCOL_MAJOR(PG_PROTOCOL_EARLIEST),
- PG_PROTOCOL_MAJOR(PG_PROTOCOL_LATEST),
- PG_PROTOCOL_MINOR(PG_PROTOCOL_LATEST))));
The way you've arranged things here looks like it'll cause us to
accept connections even for protocol versions 4.x or higher; I don't
think we want that. I suggest checking the major version number at
this point in the code; then, the code path for version 3+ needs no
additional check and the code path for version 2 can enforce 2.0.
+bool
+is_optional(const char *guc_name)
+{
+ const char *optionalPrefix = "_pq_";
+ bool isOptional = false;
+
+ /* "_pq_" must be a proper prefix of the guc name in all encodings */
+ if (guc_name_compare(guc_name, optionalPrefix) == 1 &&
+ strstr(guc_name, optionalPrefix))
+ isOptional = true;
+
+ return isOptional;
+}
This seems like very strange coding for all kinds of reasons. Why is
guc_name_compare() used to do the comparison and strstr() then used
afterwards? Why do we need two tests instead of just one, and why
should one of them be case-sensitive and the other not? Why not just
use strncmp? Why write bool var = false; if (blah blah) var = true;
return var; instead of just return blah blah? Why the comment about
encodings - that doesn't seem particularly relevant here? Why
redeclare the prefix here instead of having a common definition
someplace that can be used by both the frontend and the backend,
probably a header file #define?
Also, this really doesn't belong in guc.c at all. We should be
separating out these options in ProcessStartupPacket() just as we do
for existing protocol-level options. When we actually have some
options, I think they should be segregated into a separate list
hanging off of the port, instead of letting them get mixed into
port->guc_options, but for right now we don't have any yet, so a bunch
of this complexity can go away.
+ ListCell *gucopts = list_head(port->guc_options);
+ while (gucopts)
+ {
+ char *name;
+
+ /* First comes key, which we need. */
+ name = lfirst(gucopts);
+ gucopts = lnext(gucopts);
+
+ /* Then comes value, which we don't need. */
+ gucopts = lnext(gucopts);
+
+ pq_sendstring(&buf, name);
+ }
This is another coding rule violation because the declaration of
gucopts follows executable statements.
- SimpleStringList roles = {NULL, NULL};
+ SimpleStringList roles = {NULL, NULL, NULL};
I don't think it's a good idea to change SimpleStringList like this --
it's used in lots of places already. If we were going to do it, a
bool needs to be set to false, not NULL.
+ /* Cannot append to immutable list */
+ if (list->is_immutable)
+ return;
Even if I were inclined to support changing the SimpleStringList
abstraction, this seems like super-confusing behavior -- just don't
append, and don't warn the user that nothing happened in any way?
Ugh.
+override CPPFLAGS := -DFRONTEND $(CPPFLAGS) -fPIC
+override CPPFLAGS := -DFRONTEND -I$(libpq_srcdir) $(CPPFLAGS) -fPIC
Seems unrelated to anything this patch is about.
+#include "postgres.h"
We never include "postgres.h" in other header files.
+#include "libpq/libpq-be.h"
+
+extern int NegotiateServerProtocol(Port *port);
+extern int SendServerProtocolVersionMessage(Port *port);
These changes aren't needed. The functions aren't called from outside
the file where they are defined, so just make them static and
prototype them in that file. That avoids sucking in additional
headers into this file.
+ /*
+ * Block until message length is read.
+ *
+ * No need to account for 2.x fixed-length message
because this state cannot
+ * be reached by pre-3.0 server.
+ */
Wrong formatting. pgindent will fix it.
+ {
+ return PGRES_POLLING_READING;
+ }
Superfluous braces.
+ {
+ server_is_older = true;
+ }
And here.
+ runningMsgLength -= buf->len + 1; /* +1 for NULL */
NUL or \0, not NULL. You're talking about the byte, not the pointer value.
In general, I think it might be a good idea for the client to send a
3.0 connection request unless the user requests some feature that
requires use of a >3.0 protocol version -- and right now there are no
such features. It's a little hard to predict what we might want to do
with minor protocol versions in the future so maybe at some point
there will be a good reason for us to request the newest protocol
version we can get (e.g. if we make some protocol change that improves
performance). Right now, though, there's a big advantage to not
requesting anything beyond 3.0 unless we need it -- it works with
existing servers. So I suggest that for right now we just make the
server side changes here to 3.x,x>0 protocol versions and accept
_pq_.whatever parameters, and leave all of these libpq changes out
completely. Some future patch might need those changes but this one
doesn't.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: protocol version negotiation (Re: LibpqPGRES_COPY_BOTH - version compatibility)
От
Badrul Chowdhury
Дата:
Hi Robert,
Thanks very much for your quick response. PFA the patch containing the BE changes for pgwire v3.1, correctly formatted
usingpgindent this time 😊
A few salient points:
>> SendServerProtocolVersionMessage should be adjusted to use the new
>> facilities added by commit 1de09ad8eb1fa673ee7899d6dfbb2b49ba204818.
The new functionality is for sending 64bit ints. I think 32bits is sufficient for the information we want to pass
aroundin the protocol negotiation phase, so I left this part unchanged.
>> Also, this really doesn't belong in guc.c at all. We should be separating out
>> these options in ProcessStartupPacket() just as we do for existing protocol-
>> level options. When we actually have some options, I think they should be
>> segregated into a separate list hanging off of the port, instead of letting them
>> get mixed into
>> port->guc_options, but for right now we don't have any yet, so a bunch
>> of this complexity can go away.
You are right, it is more elegant to make this a part of the port struct; I made the necessary changes in the patch.
Thanks,
Badrul
>> -----Original Message-----
>> From: Robert Haas [mailto:robertmhaas@gmail.com]
>> Sent: Friday, October 13, 2017 11:16 AM
>> To: Badrul Chowdhury <bachow@microsoft.com>
>> Cc: Tom Lane <tgl@sss.pgh.pa.us>; Satyanarayana Narlapuram
>> <Satyanarayana.Narlapuram@microsoft.com>; Craig Ringer
>> <craig@2ndquadrant.com>; Peter Eisentraut <peter_e@gmx.net>; Magnus
>> Hagander <magnus@hagander.net>; PostgreSQL-development <pgsql-
>> hackers@postgresql.org>
>> Subject: Re: [HACKERS] Re: protocol version negotiation (Re: Libpq
>> PGRES_COPY_BOTH - version compatibility)
>>
>> On Fri, Oct 6, 2017 at 5:07 PM, Badrul Chowdhury <bachow@microsoft.com>
>> wrote:
>> > I added a mechanism to fall back to v3.0 if the BE fails to start when FE
>> initiates a connection with v3.1 (with optional startup parameters). This
>> completely eliminates the need to backpatch older servers, ie newer FE can
>> connect to older BE. Please let me know what you think.
>>
>> Well, I think it needs a good bit of cleanup before we can really get to the
>> substance of the patch.
>>
>> + fe_utils \
>> interfaces \
>> backend/replication/libpqwalreceiver \
>> backend/replication/pgoutput \
>> - fe_utils \
>>
>> Useless change, omit.
>>
>> + if (whereToSendOutput != DestRemote ||
>> + PG_PROTOCOL_MAJOR(FrontendProtocol) < 3)
>> + return -1;
>> +
>> + int sendStatus = 0;
>>
>> Won't compile on older compilers. We generally aim for C89 compliance, with
>> a few exceptions for newer features.
>>
>> Also, why initialize sendStatus and then overwrite the value in the very next
>> line of code?
>>
>> Also, the PG_PROTOCOL_MAJOR check here seems to be redundant with the
>> one in the caller.
>>
>> + /* NegotiateServerProtocol packet structure
>> + *
>> + * [ 'Y' | msgLength | min_version | max_version | param_list_len
>> | list of param names ]
>> + */
>> +
>>
>> Please pgindent your patches. I suspect you'll find this gets garbled.
>>
>> Is there really any reason to separate NegotiateServerProtocol and
>> ServerProtocolVersion into separate functions?
>>
>> -libpq = -L$(libpq_builddir) -lpq
>> +libpq = -L$(libpq_builddir) -lpq -L$(top_builddir)/src/common
>> -lpgcommon -L$(top_builddir)/src/fe_utils -lpgfeutils
>> + $libpq->AddReference($libpgcommon, $libpgfeutils, $libpgport);
>>
>> I haven't done any research to try to figure out why you did this, but I don't
>> think these are likely to be acceptable changes.
>>
>> SendServerProtocolVersionMessage should be adjusted to use the new
>> facilities added by commit 1de09ad8eb1fa673ee7899d6dfbb2b49ba204818.
>>
>> - /* Check we can handle the protocol the frontend is using. */
>> -
>> - if (PG_PROTOCOL_MAJOR(proto) <
>> PG_PROTOCOL_MAJOR(PG_PROTOCOL_EARLIEST) ||
>> - PG_PROTOCOL_MAJOR(proto) >
>> PG_PROTOCOL_MAJOR(PG_PROTOCOL_LATEST) ||
>> - (PG_PROTOCOL_MAJOR(proto) ==
>> PG_PROTOCOL_MAJOR(PG_PROTOCOL_LATEST) &&
>> - PG_PROTOCOL_MINOR(proto) >
>> PG_PROTOCOL_MINOR(PG_PROTOCOL_LATEST)))
>> - ereport(FATAL,
>> - (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
>> - errmsg("unsupported frontend protocol %u.%u: server supports %
>> u.0 to %u.%u",
>> - PG_PROTOCOL_MAJOR(proto), PG_PROTOCOL_MINOR(proto),
>> - PG_PROTOCOL_MAJOR(PG_PROTOCOL_EARLIEST),
>> - PG_PROTOCOL_MAJOR(PG_PROTOCOL_LATEST),
>> - PG_PROTOCOL_MINOR(PG_PROTOCOL_LATEST))));
>>
>> The way you've arranged things here looks like it'll cause us to accept
>> connections even for protocol versions 4.x or higher; I don't think we want
>> that. I suggest checking the major version number at this point in the code;
>> then, the code path for version 3+ needs no additional check and the code
>> path for version 2 can enforce 2.0.
>>
>> +bool
>> +is_optional(const char *guc_name)
>> +{
>> + const char *optionalPrefix = "_pq_";
>> + bool isOptional = false;
>> +
>> + /* "_pq_" must be a proper prefix of the guc name in all encodings */
>> + if (guc_name_compare(guc_name, optionalPrefix) == 1 &&
>> + strstr(guc_name, optionalPrefix))
>> + isOptional = true;
>> +
>> + return isOptional;
>> +}
>>
>> This seems like very strange coding for all kinds of reasons. Why is
>> guc_name_compare() used to do the comparison and strstr() then used
>> afterwards? Why do we need two tests instead of just one, and why should
>> one of them be case-sensitive and the other not? Why not just use strncmp?
>> Why write bool var = false; if (blah blah) var = true; return var; instead of just
>> return blah blah? Why the comment about encodings - that doesn't seem
>> particularly relevant here? Why redeclare the prefix here instead of having a
>> common definition someplace that can be used by both the frontend and the
>> backend, probably a header file #define?
>>
>> Also, this really doesn't belong in guc.c at all. We should be separating out
>> these options in ProcessStartupPacket() just as we do for existing protocol-
>> level options. When we actually have some options, I think they should be
>> segregated into a separate list hanging off of the port, instead of letting them
>> get mixed into
>> port->guc_options, but for right now we don't have any yet, so a bunch
>> of this complexity can go away.
>>
>> + ListCell *gucopts = list_head(port->guc_options);
>> + while (gucopts)
>> + {
>> + char *name;
>> +
>> + /* First comes key, which we need. */
>> + name = lfirst(gucopts);
>> + gucopts = lnext(gucopts);
>> +
>> + /* Then comes value, which we don't need. */
>> + gucopts = lnext(gucopts);
>> +
>> + pq_sendstring(&buf, name);
>> + }
>>
>> This is another coding rule violation because the declaration of gucopts
>> follows executable statements.
>>
>> - SimpleStringList roles = {NULL, NULL};
>> + SimpleStringList roles = {NULL, NULL, NULL};
>>
>> I don't think it's a good idea to change SimpleStringList like this -- it's used in
>> lots of places already. If we were going to do it, a bool needs to be set to
>> false, not NULL.
>>
>> + /* Cannot append to immutable list */
>> + if (list->is_immutable)
>> + return;
>>
>> Even if I were inclined to support changing the SimpleStringList abstraction,
>> this seems like super-confusing behavior -- just don't append, and don't warn
>> the user that nothing happened in any way?
>> Ugh.
>>
>> +override CPPFLAGS := -DFRONTEND $(CPPFLAGS) -fPIC override CPPFLAGS :=
>> +-DFRONTEND -I$(libpq_srcdir) $(CPPFLAGS) -fPIC
>>
>> Seems unrelated to anything this patch is about.
>>
>> +#include "postgres.h"
>>
>> We never include "postgres.h" in other header files.
>>
>> +#include "libpq/libpq-be.h"
>> +
>> +extern int NegotiateServerProtocol(Port *port); extern int
>> +SendServerProtocolVersionMessage(Port *port);
>>
>> These changes aren't needed. The functions aren't called from outside the file
>> where they are defined, so just make them static and prototype them in that
>> file. That avoids sucking in additional headers into this file.
>>
>> + /*
>> + * Block until message length is read.
>> + *
>> + * No need to account for 2.x fixed-length message
>> because this state cannot
>> + * be reached by pre-3.0 server.
>> + */
>>
>> Wrong formatting. pgindent will fix it.
>>
>> + {
>> + return PGRES_POLLING_READING;
>> + }
>>
>> Superfluous braces.
>>
>> + {
>> + server_is_older = true;
>> + }
>>
>> And here.
>>
>> + runningMsgLength -= buf->len + 1; /* +1 for NULL */
>>
>> NUL or \0, not NULL. You're talking about the byte, not the pointer value.
>>
>> In general, I think it might be a good idea for the client to send a
>> 3.0 connection request unless the user requests some feature that requires
>> use of a >3.0 protocol version -- and right now there are no such features. It's
>> a little hard to predict what we might want to do with minor protocol versions
>> in the future so maybe at some point there will be a good reason for us to
>> request the newest protocol version we can get (e.g. if we make some
>> protocol change that improves performance). Right now, though, there's a big
>> advantage to not requesting anything beyond 3.0 unless we need it -- it works
>> with existing servers. So I suggest that for right now we just make the server
>> side changes here to 3.x,x>0 protocol versions and accept _pq_.whatever
>> parameters, and leave all of these libpq changes out completely. Some future
>> patch might need those changes but this one doesn't.
>>
>> --
>> Robert Haas
>> EnterpriseDB:
>> https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.ent
>> erprisedb.com&data=02%7C01%7Cbachow%40microsoft.com%7Ce68db1491
>> 10b426ceb4e08d51266842c%7C72f988bf86f141af91ab2d7cd011db47%7C1
>> %7C0%7C636435153876276748&sdata=1Y%2FLhfYfN9Km8PxKAN7ghF1siYUt
>> hXoIY0LGxQNywk8%3D&reserved=0
>> The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Вложения
On Thu, Oct 19, 2017 at 1:35 AM, Badrul Chowdhury <bachow@microsoft.com> wrote: > The new functionality is for sending 64bit ints. I think 32bits is sufficient for the information we want to pass aroundin the protocol negotiation phase, so I left this part unchanged. No, it isn't. That commit didn't add any new functionality, but it changed the preferred interfaces for assembling protocol messages. Your patch was still using the old ones. Attached is an updated patch. I've made a few modifications: - I wrote documentation. For future reference, that's really the job of the patch submitter. - I changed the message type byte for the new message from 'Y' to 'v'. 'Y' is apparently used by logical replication as a "type message", but 'v' is not currently used for anything. It's also somewhat mnemonic. - I deleted the minimum protocol version from the new message. I know there were a few votes for including it, but I think it's probably useless. The client should always request the newest version it can support; if that's not new enough for the server, then we're dead anyway and we might as well just handle that via ERROR. Moreover, it seems questionable whether we'd ever deprecate 3.0 support in the server anyway, or if we do, it'll probably be because 4.0 has been stable for a decade or so. Desupporting 3.0 while continuing to support 3.x,x>0 seems like a remote outcome (and, like I say, if it does happen, ERROR is a good-enough response). If there's some use case for having a client request an older protocol version than the newest one it can support, then this change could be revisited, or we can just handle that by retrying the whole connection attempt. - I changed the test for whether to send NegotiateProtocolVersion to send it only when the client requests a version too new for the server. I think that if the client requests a version older than what the server could support, the server should just silently use the older version. That's arguable. You could argue that the message should be sent anyway (at least to post-3.0 clients) as a way of reporting what happened with _pq_.<whatever> parameters, but I have two counter-arguments. Number one, maybe clients just shouldn't send _pq_.<whatever> parameters that the protocol version they're using doesn't support, or if they do, be prepared for them to have no effect. Number two, this is exactly the sort of thing that we can change in future minor protocol versions if we wish. For example, we could define protocol version 3.1 or 3.43 or whatever as always sending a NegotiateProtocolVersion message. There's no need for the code to make 3.0 compatible with future versions to decide what choices those future versions might make. - Made the prefix matching check for "_pq_." rather than "_pq_". I think we're imagining extensions like _pq_.magic_fairy_dust, not _pq_magic_fairy_dust. - I got rid of the optional_parameters thing in Port and just passed a list of unrecognized parameters around directly. Once some parameters are recognized, e.g. _pq_.magic_fairy_dust = 1, we'll probably have dedicated fields in the Port for them (i.e. int magic_fairy_dust) rather than digging them out of some list. Moreover, with your design, we'd end up having to make NegotiateServerProtocol exclude things that are actually recognized, which would be annoying. - I got rid of the pq_flush(). There's no need for this because we're always going to send another protocol message (ErrorResponse or AuthenticationSomething) afterwards. - I renamed NegotiateServerProtocol to SendNegotiateProtocolVersion and moved it to what I think is a more sensible place in the file. - I removed the error check for 2.x, x != 0. I may have advocated for this before, but on reflection, I don't see much advantage in rejecting things that work today. - I fixed the above-mentioned failure to use the new interface for assembling the NegotiateProtocolVersion message. - I did some work on the comments. Barring objections, I plan to commit this and back-patch it all the way. Of course, this is not a bug fix, but Tom previously proposed back-patching it and I think that's a good idea, because if we don't, it will be a very long time before servers with this code become commonplace in the wild. Back-patching doesn't completely fix that problem because not everybody applies upgrades and some people may be running EOL versions, but it will still help. Also attached is a patch I used for testing purposes, some version of which we might eventually use when we actually introduce version 3.1 of the protocol. It bumps the protocol version that libpq uses from 3.0 to 3.1 without changing what the server thinks the latest protocol version is, so the server always replies with a NegotiateProtocolVersion message. It also teaches libpq to ignore the NegotiateProtocolVersion message. With that patch applied, make check-world passes, which seems to show that the server-side changes are not totally broken. More testing would be great, of course. Some thoughts on the future: - libpq should grow an option to force a specific protocol version. Andres already proposed one to force 2.0, but now we probably want to generalize that to also allow forcing a particular minor version. That seems useful for testing, if nothing else, and might let us add TAP tests that this stuff works as intended. - Possibly we should commit the portion of the testing patch which ignores NegotiateProtocolVersion to v11, maybe also adding a connection status function that lets users inquire about whether a NegotiateProtocolVersion message was received and, if so, what parameters it reported as unrecognized and what minor version it reported the server as speaking. The existing PQprotocolVersion interface looks inadequate, as it seems to return only the major version. - On further reflection, I think the reconnect functionality you proposed previously is probably a good idea. It won't be necessary with servers that have been patched to send NegotiateProtocolVersion, but there may be quite a few that haven't for a long time to come, and although an automated reconnect is a little annoying, it's a lot less annoying than an outright connection failure. So that part of your patch should probably be resubmitted when and if we bump the version to 3.1. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Вложения
Re: [HACKERS] Re: protocol version negotiation (Re: LibpqPGRES_COPY_BOTH - version compatibility)
От
Badrul Chowdhury
Дата:
Hi Robert, Thank you for the comprehensive review! We are very much in the early stages of contributing to the PG community and we clearlyhave lots to learn, but we look forward to becoming proficient and active members of the pg community. Regarding the patch, I have tested it extensively by hand and it works great. Some comments on the future direction: >> Some thoughts on the future: >> >> - libpq should grow an option to force a specific protocol version. >> Andres already proposed one to force 2.0, but now we probably want to >> generalize that to also allow forcing a particular minor version. >> That seems useful for testing, if nothing else, and might let us add TAP tests >> that this stuff works as intended. >> >> - Possibly we should commit the portion of the testing patch which ignores >> NegotiateProtocolVersion to v11, maybe also adding a connection status >> function that lets users inquire about whether a NegotiateProtocolVersion >> message was received and, if so, what parameters it reported as unrecognized >> and what minor version it >> reported the server as speaking. The existing PQprotocolVersion >> interface looks inadequate, as it seems to return only the major version. I think these changes are a good idea; I will initiate a design discussion on these targeting the 2018-01 commitfest on aseparate thread. >> - On further reflection, I think the reconnect functionality you proposed >> previously is probably a good idea. It won't be necessary with servers that >> have been patched to send NegotiateProtocolVersion, but there may be quite >> a few that haven't for a long time to come, and although an automated >> reconnect is a little annoying, it's a lot less annoying than an outright >> connection failure. So that part of your patch should probably be resubmitted >> when and if we bump the version to 3.1. I will preserve the FE changes in my local branch so that we have it ready when a decision has been made regarding the bumpingof the pgwire version. Again, thanks very much for your feedback- I am in a much better position to make future contributions to the community explicitlybecause of it. Regards, Badrul >> -----Original Message----- >> From: Robert Haas [mailto:robertmhaas@gmail.com] >> Sent: Friday, October 27, 2017 5:56 AM >> To: Badrul Chowdhury <bachow@microsoft.com> >> Cc: Tom Lane <tgl@sss.pgh.pa.us>; Satyanarayana Narlapuram >> <Satyanarayana.Narlapuram@microsoft.com>; Craig Ringer >> <craig@2ndquadrant.com>; Peter Eisentraut <peter_e@gmx.net>; Magnus >> Hagander <magnus@hagander.net>; PostgreSQL-development <pgsql- >> hackers@postgresql.org> >> Subject: Re: [HACKERS] Re: protocol version negotiation (Re: Libpq >> PGRES_COPY_BOTH - version compatibility) >> >> On Thu, Oct 19, 2017 at 1:35 AM, Badrul Chowdhury >> <bachow@microsoft.com> wrote: >> > The new functionality is for sending 64bit ints. I think 32bits is sufficient for >> the information we want to pass around in the protocol negotiation phase, so >> I left this part unchanged. >> >> No, it isn't. That commit didn't add any new functionality, but it changed the >> preferred interfaces for assembling protocol messages. >> Your patch was still using the old ones. >> >> Attached is an updated patch. I've made a few modifications: >> >> - I wrote documentation. For future reference, that's really the job of the >> patch submitter. >> >> - I changed the message type byte for the new message from 'Y' to 'v'. >> 'Y' is apparently used by logical replication as a "type message", but 'v' is not >> currently used for anything. It's also somewhat mnemonic. >> >> - I deleted the minimum protocol version from the new message. I know there >> were a few votes for including it, but I think it's probably useless. The client >> should always request the newest version it can support; if that's not new >> enough for the server, then we're dead anyway and we might as well just >> handle that via ERROR. Moreover, it seems questionable whether we'd ever >> deprecate 3.0 support in the server anyway, or if we do, it'll probably be >> because 4.0 has been stable for a decade or so. Desupporting 3.0 while >> continuing to support 3.x,x>0 seems like a remote outcome (and, like I say, if it >> does happen, ERROR is a good-enough response). If there's some use case for >> having a client request an older protocol version than the newest one it can >> support, then this change could be revisited, or we can just handle that by >> retrying the whole connection attempt. >> >> - I changed the test for whether to send NegotiateProtocolVersion to send it >> only when the client requests a version too new for the server. I think that if >> the client requests a version older than what the server could support, the >> server should just silently use the older version. That's arguable. You could >> argue that the message should be sent anyway (at least to post-3.0 clients) as >> a way of reporting what happened with _pq_.<whatever> parameters, but I >> have two counter-arguments. Number one, maybe clients just shouldn't send >> _pq_.<whatever> parameters that the protocol version they're using doesn't >> support, or if they do, be prepared for them to have no effect. Number two, >> this is exactly the sort of thing that we can change in future minor protocol >> versions if we wish. For example, we could define protocol version 3.1 or 3.43 >> or whatever as always sending a NegotiateProtocolVersion message. There's >> no need for the code to make 3.0 compatible with future versions to decide >> what choices those future versions might make. >> >> - Made the prefix matching check for "_pq_." rather than "_pq_". I think >> we're imagining extensions like _pq_.magic_fairy_dust, not >> _pq_magic_fairy_dust. >> >> - I got rid of the optional_parameters thing in Port and just passed a list of >> unrecognized parameters around directly. Once some parameters are >> recognized, e.g. _pq_.magic_fairy_dust = 1, we'll probably have dedicated >> fields in the Port for them (i.e. int magic_fairy_dust) rather than digging them >> out of some list. Moreover, with your design, we'd end up having to make >> NegotiateServerProtocol exclude things that are actually recognized, which >> would be annoying. >> >> - I got rid of the pq_flush(). There's no need for this because we're always >> going to send another protocol message (ErrorResponse or >> AuthenticationSomething) afterwards. >> >> - I renamed NegotiateServerProtocol to SendNegotiateProtocolVersion and >> moved it to what I think is a more sensible place in the file. >> >> - I removed the error check for 2.x, x != 0. I may have advocated for this >> before, but on reflection, I don't see much advantage in rejecting things that >> work today. >> >> - I fixed the above-mentioned failure to use the new interface for assembling >> the NegotiateProtocolVersion message. >> >> - I did some work on the comments. >> >> Barring objections, I plan to commit this and back-patch it all the way. Of >> course, this is not a bug fix, but Tom previously proposed back-patching it and >> I think that's a good idea, because if we don't, it will be a very long time >> before servers with this code become commonplace in the wild. Back- >> patching doesn't completely fix that problem because not everybody applies >> upgrades and some people may be running EOL versions, but it will still help. >> >> Also attached is a patch I used for testing purposes, some version of which we >> might eventually use when we actually introduce version 3.1 of the protocol. >> It bumps the protocol version that libpq uses from >> 3.0 to 3.1 without changing what the server thinks the latest protocol version >> is, so the server always replies with a NegotiateProtocolVersion message. It >> also teaches libpq to ignore the NegotiateProtocolVersion message. With that >> patch applied, make check-world passes, which seems to show that the server- >> side changes are not totally broken. More testing would be great, of course. >> >> Some thoughts on the future: >> >> - libpq should grow an option to force a specific protocol version. >> Andres already proposed one to force 2.0, but now we probably want to >> generalize that to also allow forcing a particular minor version. >> That seems useful for testing, if nothing else, and might let us add TAP tests >> that this stuff works as intended. >> >> - Possibly we should commit the portion of the testing patch which ignores >> NegotiateProtocolVersion to v11, maybe also adding a connection status >> function that lets users inquire about whether a NegotiateProtocolVersion >> message was received and, if so, what parameters it reported as unrecognized >> and what minor version it >> reported the server as speaking. The existing PQprotocolVersion >> interface looks inadequate, as it seems to return only the major version. >> >> - On further reflection, I think the reconnect functionality you proposed >> previously is probably a good idea. It won't be necessary with servers that >> have been patched to send NegotiateProtocolVersion, but there may be quite >> a few that haven't for a long time to come, and although an automated >> reconnect is a little annoying, it's a lot less annoying than an outright >> connection failure. So that part of your patch should probably be resubmitted >> when and if we bump the version to 3.1. >> >> -- >> Robert Haas >> EnterpriseDB: >> https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.ent >> erprisedb.com&data=02%7C01%7Cbachow%40microsoft.com%7Cf4348d608f >> 8f421e33a608d51d3a08ef%7C72f988bf86f141af91ab2d7cd011db47%7C1%7 >> C0%7C636447057460416468&sdata=tyWDa%2B5cmxBEEIs5Btnm3PEl7SZF5a >> 0ifhRhoD0QNig%3D&reserved=0 >> The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
On Mon, Oct 30, 2017 at 9:19 PM, Badrul Chowdhury <bachow@microsoft.com> wrote: > Thank you for the comprehensive review! We are very much in the early stages of contributing to the PG community and weclearly have lots to learn, but we look forward to becoming proficient and active members of the pg community. > > Regarding the patch, I have tested it extensively by hand and it works great. I spent a little more time looking at this patch today. I think that the patch should actually send NegotiateProtocolVersion when *either* the requested version is differs from the latest one we support *or* an unsupported protocol option is present. Otherwise, you only find out about unsupported protocol options if you also request a newer minor version, which isn't good, because it makes it hard to add new protocol options *without* bumping the protocol version. Here's an updated version with that change and a proposed commit message. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Вложения
RE: [HACKERS] Re: protocol version negotiation (Re: LibpqPGRES_COPY_BOTH - version compatibility)
От
Badrul Chowdhury
Дата:
>> I spent a little more time looking at this patch today. I think that the patch >> should actually send NegotiateProtocolVersion when *either* the requested >> version is differs from the latest one we support *or* an unsupported protocol >> option is present. Otherwise, you only find out about unsupported protocol >> options if you also request a newer minor version, which isn't good, because it >> makes it hard to add new protocol options *without* bumping the protocol >> version. It makes sense from a maintainability point of view. >> Here's an updated version with that change and a proposed commit message. I have tested the new patch and it works great. The comments look good as well. Thanks, Badrul >> -----Original Message----- >> From: Robert Haas [mailto:robertmhaas@gmail.com] >> Sent: Wednesday, November 15, 2017 1:12 PM >> To: Badrul Chowdhury <bachow@microsoft.com> >> Cc: Tom Lane <tgl@sss.pgh.pa.us>; Satyanarayana Narlapuram >> <Satyanarayana.Narlapuram@microsoft.com>; Craig Ringer >> <craig@2ndquadrant.com>; Peter Eisentraut <peter_e@gmx.net>; Magnus >> Hagander <magnus@hagander.net>; PostgreSQL-development <pgsql- >> hackers@postgresql.org> >> Subject: Re: [HACKERS] Re: protocol version negotiation (Re: Libpq >> PGRES_COPY_BOTH - version compatibility) >> >> On Mon, Oct 30, 2017 at 9:19 PM, Badrul Chowdhury >> <bachow@microsoft.com> wrote: >> > Thank you for the comprehensive review! We are very much in the early >> stages of contributing to the PG community and we clearly have lots to learn, >> but we look forward to becoming proficient and active members of the pg >> community. >> > >> > Regarding the patch, I have tested it extensively by hand and it works great. >> >> I spent a little more time looking at this patch today. I think that the patch >> should actually send NegotiateProtocolVersion when *either* the requested >> version is differs from the latest one we support *or* an unsupported protocol >> option is present. Otherwise, you only find out about unsupported protocol >> options if you also request a newer minor version, which isn't good, because it >> makes it hard to add new protocol options *without* bumping the protocol >> version. >> >> Here's an updated version with that change and a proposed commit message. >> >> -- >> Robert Haas >> EnterpriseDB: >> https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.ent >> erprisedb.com&data=02%7C01%7Cbachow%40microsoft.com%7Ce37b69223 >> a144d1e5b7408d52c6d8171%7C72f988bf86f141af91ab2d7cd011db47%7C1 >> %7C0%7C636463771208784375&sdata=1%2FDylQIfS2rI2RwIVyZnDCUbzRQJe >> V4YM8J496QkpiQ%3D&reserved=0 >> The Enterprise PostgreSQL Company
On Sun, Nov 19, 2017 at 7:49 PM, Badrul Chowdhury <bachow@microsoft.com> wrote: >>> I spent a little more time looking at this patch today. I think that the patch >>> should actually send NegotiateProtocolVersion when *either* the requested >>> version is differs from the latest one we support *or* an unsupported protocol >>> option is present. Otherwise, you only find out about unsupported protocol >>> options if you also request a newer minor version, which isn't good, because it >>> makes it hard to add new protocol options *without* bumping the protocol >>> version. > > It makes sense from a maintainability point of view. > >>> Here's an updated version with that change and a proposed commit message. > > I have tested the new patch and it works great. The comments look good as well. Committed and back-patched to all supported branches. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company