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