Re: [HACKERS] Re: protocol version negotiation (Re: Libpq PGRES_COPY_BOTH - version compatibility)

2017-10-30 Thread Badrul Chowdhury
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:robertmh...@gmail.com]
>> Sent: Friday, October 27, 2017 5:56 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 > hack...@postgresql.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
>> <bac...@microsoft.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, 

Re: [HACKERS] Re: protocol version negotiation (Re: Libpq PGRES_COPY_BOTH - version compatibility)

2017-10-18 Thread Badrul Chowdhury
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 > 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 %

Re: [HACKERS] Re: protocol version negotiation (Re: Libpq PGRES_COPY_BOTH - version compatibility)

2017-10-06 Thread Badrul Chowdhury
Hi Tom and Robert, 

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.

Thanks,
Badrul

-Original Message-
From: Robert Haas [mailto:robertmh...@gmail.com] 
Sent: Wednesday, October 4, 2017 4:54 AM
To: Tom Lane <t...@sss.pgh.pa.us>
Cc: Badrul Chowdhury <bac...@microsoft.com>; Satyanarayana Narlapuram 
<satyanarayana.narlapu...@microsoft.com>; Craig Ringer <cr...@2ndquadrant.com>; 
Peter Eisentraut <pete...@gmx.net>; Magnus Hagander <mag...@hagander.net>; 
PostgreSQL-development <pgsql-hackers@postgresql.org>
Subject: Re: [HACKERS] Re: protocol version negotiation (Re: Libpq 
PGRES_COPY_BOTH - version compatibility)

On Tue, Oct 3, 2017 at 9:46 PM, Tom Lane <t...@sss.pgh.pa.us> wrote:
> Badrul Chowdhury <bac...@microsoft.com> writes:
>> 1. Pgwire protocol v3.0 with negotiation is called v3.1.
>> 2. There are 2 patches for the change: a BE-specific patch that will be 
>> backported and a FE-specific patch that is only for pg10 and above.
>
> TBH, anything that presupposes a backported change in the backend is 
> broken by definition.  We expect libpq to be able to connect to older 
> servers, and that has to include servers that didn't get this memo.
>
> It would be all right for libpq to make a second connection attempt if 
> its first one fails, as we did in the 2.0 -> 3.0 change.

Hmm, that's another approach, but I prefer the one advocated by Tom Lane.

https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.postgresql.org%2Fmessage-id%2F30788.1498672033%40sss.pgh.pa.us=02%7C01%7Cbachow%40microsoft.com%7Cd183fe16a3a445f4bc7c08d50b1e9e9e%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636427148510331370=jLwhk6twUrlsm9K6yLronVvg%2Fjx93MM37UXm6NndfLY%3D=0
https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.postgresql.org%2Fmessage-id%2F24357.1498703265%2540sss.pgh.pa.us=02%7C01%7Cbachow%40microsoft.com%7Cd183fe16a3a445f4bc7c08d50b1e9e9e%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636427148510331370=gtFfNcxR3qK7rzieQQ0EAOFn%2BsDsw8rjtQeWwyIv6EY%3D=0

--
Robert Haas
EnterpriseDB: 
https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.enterprisedb.com=02%7C01%7Cbachow%40microsoft.com%7Cd183fe16a3a445f4bc7c08d50b1e9e9e%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636427148510331370=wf9cTkQEnRzkdaZxZ1D6NBY9kZbiViyni5lkA7nzEXM%3D=0
The Enterprise PostgreSQL Company


pgwire3.1.patch
Description: pgwire3.1.patch

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Re: protocol version negotiation (Re: Libpq PGRES_COPY_BOTH - version compatibility)

2017-10-04 Thread Badrul Chowdhury
Okay, I will add a mechanism to try connecting with 3.0 if 3.1 fails- that 
should be a few lines of code fe-connect.c; this will eliminate the need for a 
back-patch. What do you think of the rest of the change? 

Thanks,
Badrul

-Original Message-
From: Robert Haas [mailto:robertmh...@gmail.com] 
Sent: Wednesday, October 4, 2017 4:54 AM
To: Tom Lane <t...@sss.pgh.pa.us>
Cc: Badrul Chowdhury <bac...@microsoft.com>; Satyanarayana Narlapuram 
<satyanarayana.narlapu...@microsoft.com>; Craig Ringer <cr...@2ndquadrant.com>; 
Peter Eisentraut <pete...@gmx.net>; Magnus Hagander <mag...@hagander.net>; 
PostgreSQL-development <pgsql-hackers@postgresql.org>
Subject: Re: [HACKERS] Re: protocol version negotiation (Re: Libpq 
PGRES_COPY_BOTH - version compatibility)

On Tue, Oct 3, 2017 at 9:46 PM, Tom Lane <t...@sss.pgh.pa.us> wrote:
> Badrul Chowdhury <bac...@microsoft.com> writes:
>> 1. Pgwire protocol v3.0 with negotiation is called v3.1.
>> 2. There are 2 patches for the change: a BE-specific patch that will be 
>> backported and a FE-specific patch that is only for pg10 and above.
>
> TBH, anything that presupposes a backported change in the backend is 
> broken by definition.  We expect libpq to be able to connect to older 
> servers, and that has to include servers that didn't get this memo.
>
> It would be all right for libpq to make a second connection attempt if 
> its first one fails, as we did in the 2.0 -> 3.0 change.

Hmm, that's another approach, but I prefer the one advocated by Tom Lane.

https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.postgresql.org%2Fmessage-id%2F30788.1498672033%40sss.pgh.pa.us=02%7C01%7Cbachow%40microsoft.com%7Cd183fe16a3a445f4bc7c08d50b1e9e9e%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636427148510331370=jLwhk6twUrlsm9K6yLronVvg%2Fjx93MM37UXm6NndfLY%3D=0
https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.postgresql.org%2Fmessage-id%2F24357.1498703265%2540sss.pgh.pa.us=02%7C01%7Cbachow%40microsoft.com%7Cd183fe16a3a445f4bc7c08d50b1e9e9e%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636427148510331370=gtFfNcxR3qK7rzieQQ0EAOFn%2BsDsw8rjtQeWwyIv6EY%3D=0

--
Robert Haas
EnterpriseDB: 
https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.enterprisedb.com=02%7C01%7Cbachow%40microsoft.com%7Cd183fe16a3a445f4bc7c08d50b1e9e9e%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636427148510331370=wf9cTkQEnRzkdaZxZ1D6NBY9kZbiViyni5lkA7nzEXM%3D=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


[HACKERS] Re: protocol version negotiation (Re: Libpq PGRES_COPY_BOTH - version compatibility)

2017-10-03 Thread Badrul Chowdhury
  Added a CONNECTION_NEGOTIATING state in 
/src/interfaces/libpq/fe-connect.c to parse the ServerProtocolVersion message 
from the BE
   2.a The FE terminates the connection if BE rejected 
non-optional parameters.

  3.   The changes to the following files were due to the 
addition of the immutable field in the SimpleStringList struct
   3.a /src/bin/pg_dump/pg_dump.c
   3.b /src/bin/scripts/clusterdb.c
   3.c /src/bin/scripts/createuser.c
   3.d /src/bin/scripts/reindexdb.c
   3.e /src/bin/scripts/vacuumdb.c

  4.   Added some dependencies to the libpq project to be 
able to use SimpleStringList in fe-connect.c.
   4.a Visual C compiler
 4.a.1 /src/tools/msvc/Mkvcbuild.pm was 
modified to add the reference for visual c compiler
   4.b Non-windows compilers: the following files were 
modified
 4.b.1 /src/Makefile
 4.b.2 /src/Makefile.global.in
 4.b.3 /src/common/Makefile
 4.b.4 /src/fe_utils/Makefile

  5.   Bumped max_pg_protocol version from 3.0 to 3.1 in 
/src/include/libpq/pqcomm.h

/***/
/* Testing */
/***/

An outline of the testing framework used to validate the code:

  1.   Newer FE can connect to older BE
   1.a Change FE protocol version in line 1811 of 
/src/interfaces/libpq/fe-connect.c
 1.a.1 conn->pversion = PG_PROTOCOL(3, 
1); succeeds now whereas it would have failed before: a result of bumping 
max_pg_protocol from 3.0 to 3.1
 1.a.2 conn->pversion = PG_PROTOCOL(4, 
0); succeds now whereas it would have failed before: older BE is capable of 
handling newer FE now

  2.   Older drivers work as expected
   2.a Change FE protocol version in line 1811 of 
/src/interfaces/libpq/fe-connect.c
 2.a.1 conn->pversion = PG_PROTOCOL(1, 
9); fails now and failed before as well as it is below 
min_pg_protocol=PG_PROTOCOL(2, 0)
 2.a.2 conn->pversion = PG_PROTOCOL(2, 
0); succeeds now and succeeded before as well as it is in the supported range 
PG_PROTOCOL(2, 0) to PG_PROTOCOL(3, 1)
 2.a.3 conn->pversion = PG_PROTOCOL(3, 
0); succeeds now and succeeded before as well as it is in the supported range 
PG_PROTOCOL(2, 0) to PG_PROTOCOL(3, 1)

  3.   BE support for optional parameters
   3.a Accept names with "_pq_" as proper prefix, eg 
"_pq_A" would succeed
   3.b Reject all others without crashing
 3.b.1 Any string not like "_pq_A" 
should fail

Looking forward to your feedback,
Thank you,
Badrul Chowdhury

From: pgsql-hackers-ow...@postgresql.org 
[mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Robert Haas
Sent: Sunday, July 2, 2017 3:45 PM
To: Satyanarayana Narlapuram <satyanarayana.narlapu...@microsoft.com>
Cc: Tom Lane <t...@sss.pgh.pa.us>; Craig Ringer <cr...@2ndquadrant.com>; Peter 
Eisentraut <pete...@gmx.net>; Magnus Hagander <mag...@hagander.net>; 
PostgreSQL-development <pgsql-hackers@postgresql.org>
Subject: Re: protocol version negotiation (Re: Libpq PGRES_COPY_BOTH - version 
compatibility)

On Thu, Jun 29, 2017 at 7:29 PM, Satyanarayana Narlapuram
<satyanarayana.narlapu...@microsoft.com<mailto:satyanarayana.narlapu...@microsoft.com>>
 wrote:
> -Original Message-

The formatting of this message differs from the style normally used on
this mailing list, and is hard to read.

> 2. If the client version is anything other than 3.0, the server responds with 
> a ServerProtocolVersion indicating the highest version it supports, and 
> ignores any pg_protocol. options not known to it as being either 
> third-party extensions or something from a future version. If the initial 
> response to the startup message is anything other than a 
> ServerProtocolVersion message, the client should assume it's talking to a 3.0 
> server. (To make this work, we would back-patch a change into existing 
> releases to allow any 3.x protocol version and ignore any 
> pg_protocol. options that were specified.)
>
> We can avoid one round trip if the server accepts the startupmessage as is 
> (including understanding all the parameters supplied by the client), and in 
> the cases where server couldn’t accept th