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

2017-11-21 Thread Robert Haas 
On Sun, Nov 19, 2017 at 7:49 PM, Badrul Chowdhury  wrote:
>>> I spent a little more time looking at this patch today.  I think that the 
>>> patch
>>> should actually send NegotiateProtocolVersion when *either* the requested
>>> version is differs from the latest one we support *or* an unsupported 
>>> protocol
>>> option is present.  Otherwise, you only find out about unsupported protocol
>>> options if you also request a newer minor version, which isn't good, 
>>> because it
>>> makes it hard to add new protocol options *without* bumping the protocol
>>> version.
>
> It makes sense from a maintainability point of view.
>
>>> Here's an updated version with that change and a proposed commit message.
>
> I have tested the new patch and it works great. The comments look good as 
> well.

Committed and back-patched to all supported branches.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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

2017-11-19 Thread Badrul Chowdhury 
>> I spent a little more time looking at this patch today.  I think that the 
>> patch
>> should actually send NegotiateProtocolVersion when *either* the requested
>> version is differs from the latest one we support *or* an unsupported 
>> protocol
>> option is present.  Otherwise, you only find out about unsupported protocol
>> options if you also request a newer minor version, which isn't good, because 
>> it
>> makes it hard to add new protocol options *without* bumping the protocol
>> version.

It makes sense from a maintainability point of view.

>> Here's an updated version with that change and a proposed commit message.

I have tested the new patch and it works great. The comments look good as well.

Thanks,
Badrul

>> -Original Message-
>> From: Robert Haas [mailto:robertmh...@gmail.com]
>> Sent: Wednesday, November 15, 2017 1:12 PM
>> 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 Mon, Oct 30, 2017 at 9:19 PM, Badrul Chowdhury
>> <bac...@microsoft.com> wrote:
>> > 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.
>> 
>> I spent a little more time looking at this patch today.  I think that the 
>> patch
>> should actually send NegotiateProtocolVersion when *either* the requested
>> version is differs from the latest one we support *or* an unsupported 
>> protocol
>> option is present.  Otherwise, you only find out about unsupported protocol
>> options if you also request a newer minor version, which isn't good, because 
>> it
>> makes it hard to add new protocol options *without* bumping the protocol
>> version.
>> 
>> Here's an updated version with that change and a proposed commit message.
>> 
>> --
>> Robert Haas
>> EnterpriseDB:
>> https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.ent
>> erprisedb.com=02%7C01%7Cbachow%40microsoft.com%7Ce37b69223
>> a144d1e5b7408d52c6d8171%7C72f988bf86f141af91ab2d7cd011db47%7C1
>> %7C0%7C636463771208784375=1%2FDylQIfS2rI2RwIVyZnDCUbzRQJe
>> V4YM8J496QkpiQ%3D=0
>> The Enterprise PostgreSQL Company

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

2017-11-15 Thread Robert Haas 
On Mon, Oct 30, 2017 at 9:19 PM, Badrul Chowdhury  wrote:
> 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.

I spent a little more time looking at this patch today.  I think that
the patch should actually send NegotiateProtocolVersion when *either*
the requested version is differs from the latest one we support *or*
an unsupported protocol option is present.  Otherwise, you only find
out about unsupported protocol options if you also request a newer
minor version, which isn't good, because it makes it hard to add new
protocol options *without* bumping the protocol version.

Here's an updated version with that change and a proposed commit message.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


pgwire-be-rmh-v2.patch
Description: Binary data