Re: [HACKERS] Re: protocol version negotiation (Re: LibpqPGRES_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
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