Re: Re: protocol version negotiation (Re: Libpq PGRES_COPY_BOTH - version compatibility)

Hi Robert,

Thanks very much for your quick response. PFA the patch containing the BE changes for pgwire v3.1, correctly formatted using pgindent 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 around in 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(at)gmail(dot)com]
>> Sent: Friday, October 13, 2017 11:16 AM
>> To: Badrul Chowdhury <bachow(at)microsoft(dot)com>
>> Cc: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>; Satyanarayana Narlapuram
>> <Satyanarayana(dot)Narlapuram(at)microsoft(dot)com>; Craig Ringer
>> <craig(at)2ndquadrant(dot)com>; Peter Eisentraut <peter_e(at)gmx(dot)net>; Magnus
>> Hagander <magnus(at)hagander(dot)net>; PostgreSQL-development <pgsql-
>> hackers(at)postgresql(dot)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(at)microsoft(dot)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