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

Hi Robert,

Thank you for the comprehensive review! We are very much in the early stages of contributing to the PG community and we clearly have lots to learn, but we look forward to becoming proficient and active members of the pg community.

Regarding the patch, I have tested it extensively by hand and it works great.

Some comments on the future direction:

>> Some thoughts on the future:
>>
>> - libpq should grow an option to force a specific protocol version.
>> Andres already proposed one to force 2.0, but now we probably want to
>> generalize that to also allow forcing a particular minor version.
>> That seems useful for testing, if nothing else, and might let us add TAP tests
>> that this stuff works as intended.
>>
>> - Possibly we should commit the portion of the testing patch which ignores
>> NegotiateProtocolVersion to v11, maybe also adding a connection status
>> function that lets users inquire about whether a NegotiateProtocolVersion
>> message was received and, if so, what parameters it reported as unrecognized
>> and what minor version it
>> reported the server as speaking. The existing PQprotocolVersion
>> interface looks inadequate, as it seems to return only the major version.

I think these changes are a good idea; I will initiate a design discussion on these targeting the 2018-01 commitfest on a separate thread.

>> - On further reflection, I think the reconnect functionality you proposed
>> previously is probably a good idea. It won't be necessary with servers that
>> have been patched to send NegotiateProtocolVersion, but there may be quite
>> a few that haven't for a long time to come, and although an automated
>> reconnect is a little annoying, it's a lot less annoying than an outright
>> connection failure. So that part of your patch should probably be resubmitted
>> when and if we bump the version to 3.1.

I will preserve the FE changes in my local branch so that we have it ready when a decision has been made regarding the bumping of the pgwire version.

Again, thanks very much for your feedback- I am in a much better position to make future contributions to the community explicitly because of it.

Regards,
Badrul

>> -----Original Message-----
>> From: Robert Haas [mailto:robertmhaas(at)gmail(dot)com]
>> Sent: Friday, October 27, 2017 5:56 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 Thu, Oct 19, 2017 at 1:35 AM, Badrul Chowdhury
>> <bachow(at)microsoft(dot)com> wrote:
>> > 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.
>>
>> No, it isn't. That commit didn't add any new functionality, but it changed the
>> preferred interfaces for assembling protocol messages.
>> Your patch was still using the old ones.
>>
>> Attached is an updated patch. I've made a few modifications:
>>
>> - I wrote documentation. For future reference, that's really the job of the
>> patch submitter.
>>
>> - I changed the message type byte for the new message from 'Y' to 'v'.
>> 'Y' is apparently used by logical replication as a "type message", but 'v' is not
>> currently used for anything. It's also somewhat mnemonic.
>>
>> - I deleted the minimum protocol version from the new message. I know there
>> were a few votes for including it, but I think it's probably useless. The client
>> should always request the newest version it can support; if that's not new
>> enough for the server, then we're dead anyway and we might as well just
>> handle that via ERROR. Moreover, it seems questionable whether we'd ever
>> deprecate 3.0 support in the server anyway, or if we do, it'll probably be
>> because 4.0 has been stable for a decade or so. Desupporting 3.0 while
>> continuing to support 3.x,x>0 seems like a remote outcome (and, like I say, if it
>> does happen, ERROR is a good-enough response). If there's some use case for
>> having a client request an older protocol version than the newest one it can
>> support, then this change could be revisited, or we can just handle that by
>> retrying the whole connection attempt.
>>
>> - I changed the test for whether to send NegotiateProtocolVersion to send it
>> only when the client requests a version too new for the server. I think that if
>> the client requests a version older than what the server could support, the
>> server should just silently use the older version. That's arguable. You could
>> argue that the message should be sent anyway (at least to post-3.0 clients) as
>> a way of reporting what happened with _pq_.<whatever> parameters, but I
>> have two counter-arguments. Number one, maybe clients just shouldn't send
>> _pq_.<whatever> parameters that the protocol version they're using doesn't
>> support, or if they do, be prepared for them to have no effect. Number two,
>> this is exactly the sort of thing that we can change in future minor protocol
>> versions if we wish. For example, we could define protocol version 3.1 or 3.43
>> or whatever as always sending a NegotiateProtocolVersion message. There's
>> no need for the code to make 3.0 compatible with future versions to decide
>> what choices those future versions might make.
>>
>> - Made the prefix matching check for "_pq_." rather than "_pq_". I think
>> we're imagining extensions like _pq_.magic_fairy_dust, not
>> _pq_magic_fairy_dust.
>>
>> - I got rid of the optional_parameters thing in Port and just passed a list of
>> unrecognized parameters around directly. Once some parameters are
>> recognized, e.g. _pq_.magic_fairy_dust = 1, we'll probably have dedicated
>> fields in the Port for them (i.e. int magic_fairy_dust) rather than digging them
>> out of some list. Moreover, with your design, we'd end up having to make
>> NegotiateServerProtocol exclude things that are actually recognized, which
>> would be annoying.
>>
>> - I got rid of the pq_flush(). There's no need for this because we're always
>> going to send another protocol message (ErrorResponse or
>> AuthenticationSomething) afterwards.
>>
>> - I renamed NegotiateServerProtocol to SendNegotiateProtocolVersion and
>> moved it to what I think is a more sensible place in the file.
>>
>> - I removed the error check for 2.x, x != 0. I may have advocated for this
>> before, but on reflection, I don't see much advantage in rejecting things that
>> work today.
>>
>> - I fixed the above-mentioned failure to use the new interface for assembling
>> the NegotiateProtocolVersion message.
>>
>> - I did some work on the comments.
>>
>> Barring objections, I plan to commit this and back-patch it all the way. Of
>> course, this is not a bug fix, but Tom previously proposed back-patching it and
>> I think that's a good idea, because if we don't, it will be a very long time
>> before servers with this code become commonplace in the wild. Back-
>> patching doesn't completely fix that problem because not everybody applies
>> upgrades and some people may be running EOL versions, but it will still help.
>>
>> Also attached is a patch I used for testing purposes, some version of which we
>> might eventually use when we actually introduce version 3.1 of the protocol.
>> It bumps the protocol version that libpq uses from
>> 3.0 to 3.1 without changing what the server thinks the latest protocol version
>> is, so the server always replies with a NegotiateProtocolVersion message. It
>> also teaches libpq to ignore the NegotiateProtocolVersion message. With that
>> patch applied, make check-world passes, which seems to show that the server-
>> side changes are not totally broken. More testing would be great, of course.
>>
>> Some thoughts on the future:
>>
>> - libpq should grow an option to force a specific protocol version.
>> Andres already proposed one to force 2.0, but now we probably want to
>> generalize that to also allow forcing a particular minor version.
>> That seems useful for testing, if nothing else, and might let us add TAP tests
>> that this stuff works as intended.
>>
>> - Possibly we should commit the portion of the testing patch which ignores
>> NegotiateProtocolVersion to v11, maybe also adding a connection status
>> function that lets users inquire about whether a NegotiateProtocolVersion
>> message was received and, if so, what parameters it reported as unrecognized
>> and what minor version it
>> reported the server as speaking. The existing PQprotocolVersion
>> interface looks inadequate, as it seems to return only the major version.
>>
>> - On further reflection, I think the reconnect functionality you proposed
>> previously is probably a good idea. It won't be necessary with servers that
>> have been patched to send NegotiateProtocolVersion, but there may be quite
>> a few that haven't for a long time to come, and although an automated
>> reconnect is a little annoying, it's a lot less annoying than an outright
>> connection failure. So that part of your patch should probably be resubmitted
>> when and if we bump the version to 3.1.
>>
>> --
>> Robert Haas
>> EnterpriseDB:
>> https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.ent
>> erprisedb.com&data=02%7C01%7Cbachow%40microsoft.com%7Cf4348d608f
>> 8f421e33a608d51d3a08ef%7C72f988bf86f141af91ab2d7cd011db47%7C1%7
>> C0%7C636447057460416468&sdata=tyWDa%2B5cmxBEEIs5Btnm3PEl7SZF5a
>> 0ifhRhoD0QNig%3D&reserved=0
>> The Enterprise PostgreSQL Company