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:robertmh...@gmail.com] >> Sent: Friday, October 13, 2017 11:16 AM >> To: Badrul Chowdhury <bac...@microsoft.com> >> Cc: Tom Lane <t...@sss.pgh.pa.us>; Satyanarayana Narlapuram >> <satyanarayana.narlapu...@microsoft.com>; Craig Ringer >> <cr...@2ndquadrant.com>; Peter Eisentraut <pete...@gmx.net>; Magnus >> Hagander <mag...@hagander.net>; PostgreSQL-development <pgsql- >> hack...@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 <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: >> 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
pgwire_be.patch
Description: pgwire_be.patch
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
