Re: Is it time to kill support for very old servers?

On Wed, Oct 11, 2017 at 10:14 AM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> Hi,
>
> On 2017-10-11 10:09:34 +0900, Michael Paquier wrote:
>> On Wed, Oct 11, 2017 at 9:39 AM, Andres Freund <andres(at)anarazel(dot)de> wrote:
>> > On 2017-09-20 01:32:36 -0700, Andres Freund wrote:
>> >> Coverage of the relevant files is a good bit higher afterwards. Although
>> >> our libpq coverage is generally pretty damn awful.
>> >
>> > Any opinions on this? Obviously this needs some cleanup, but I'd like to
>> > know whether we've concensus on adding a connection option for this goal
>> > before investing more time into this.
>> >
>> > A nearby thread [1] whacks around some the v2 code, which triggered me
>> > to look into this. I obviously can just use thiese patches to test those
>> > patches during development, but it seems better to keep coverage.
>>
>> FWIW, I think that moving forward with such a possibility is a good
>> thing, including having a connection parameter. This would pay in the
>> long term if a new protocol version is added.
>
>> 0001 should document the new parameter.
>
> I'm actually inclined not to, and keep this as a undocumented debugging
> option. Limiting the use of this option to people willing to read the
> code seems like a good idea to me.

It seems important to me to document things present. There is a
section in the docs listing developer-only parameters for runtime
configuration, why not separating "Parameter Key Words" into two
sections then? One for the main parameters and one for developer-only
parameters.

>> + if (conn->forced_protocol_version != NULL)
>> + {
>> + conn->pversion = atoi(conn->forced_protocol_version);
>> + }
>> This should check for strlen > 0 as well.
>
> Why? Note that we don't do elsehwere in fe-connect.c.

Because it seems to me that the default value of the parameter should
be an empty string instead of D. Feels more consistent with the
others.
--
Michael