On Fri, Oct 6, 2017 at 5:07 PM, Badrul Chowdhury <bac...@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: 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