Re: PQconninfo function for libpq

On Thu, Nov 22, 2012 at 10:16 AM, Boszormenyi Zoltan <zb(at)cybertec(dot)at> wrote:
> 2012-11-22 08:35 keltezéssel, Boszormenyi Zoltan írta:
>
>> 2012-11-21 22:15 keltezéssel, Magnus Hagander írta:
>>>
>>> On Wed, Nov 21, 2012 at 10:01 PM, Boszormenyi Zoltan <zb(at)cybertec(dot)at>
>>> wrote:
>>>>
>>>> Hi,
>>>>
>>>> 2012-11-21 19:19 keltezéssel, Magnus Hagander írta:
>>>>
>>>>> I'm breaking this out into it's own thread, for my own sanity if
>>>>> nothing else :) And it's an isolated feature after all.
>>>>>
>>>>> I still agree with the previous review at
>>>>>
>>>>>
>>>>> http://archives.postgresql.org/message-id/1349321071.23971.0.camel@vanquo.pezone.net
>>>>> about keeping the data in more than one place.
>>>>
>>>>
>>>> OK, it seems I completely missed that comment.
>>>> (Or forgot about it if I happened to answer it.)
>>>>
>>>>
>>>>> Based on that, I've created a different version of this patch,
>>>>> attached. This way we keep all the data in one struct.
>>>>
>>>>
>>>> I like this single structure but not the way you handle the
>>>> options' classes. In your patch, each option belongs to only
>>>> one class. These classes are:
>>>>
>>>> PG_CONNINFO_REPLICATION = "replication" only
>>>> PG_CONNINFO_PASSWORD = "password" only
>>>> PG_CONNINFO_NORMAL = everything else
>>>>
>>>> How does it help pg_basebackup to filter out e.g. dbname and
>>>> replication?
>>>
>>> PG_CONNINFO_ALL should give pg_basebackup all it needs, no? Or
>>> actually, it shouldn't have the replication=1 part, right? So it
>>> should just use PG_CONNINFO_NORMAL|PG_CONNINFO_PASSWORD?
>>>
>>>
>>>> These are added by the walreceiver module anyway and adding them
>>>> to the primary_conninfo line should even be discouraged by the
>>>> documentation.
>>>
>>> Hmm. I wasn't actually thinking about the dbname part here, I admit that.
>>
>>
>> And not only the dbname, libpqwalreceiver adds these three:
>>
>> [zozo(at)localhost libpqwalreceiver]$ grep dbname *
>> libpqwalreceiver.c: "%s dbname=replication replication=true
>> fallback_application_name=walreceiver",
>>
>> I also excluded "application_name" from PG_CONNINFO_REPLICATION
>> by this reasoning:
>>
>> - for async replication or single standby, it doesn't matter,
>> the connection will show up as "walreceiver"
>> - for sync replication, the administrator has to add the node name
>> manually via application_name.
>>
>>>
>>>> In my view, the classes should be inclusive:
>>>>
>>>> PG_CONNINFO_NORMAL = Everything that's usable for a regular client
>>>> connection. This mean everything, maybe including "password" but
>>>> excluding "replication".
>>>>
>>>> PG_CONNINFO_PASSWORD = "password" only.
>>>>
>>>> PG_CONNINFO_REPLICATION = Everything usable for a replication client
>>>> not added by walreceiver. Maybe including/excluding "password".
>>>>
>>>> Maybe there should be two flags for replication usage:
>>>>
>>>> PG_CONNINFO_WALRECEIVER = everything except those not added
>>>> by walreceiver (and maybe "password" too)
>>>>
>>>> PG_CONNINFO_REPLICATION = "replication" only
>>>>
>>>> And every option can belong to more than one class, just as in my patch.
>>>
>>> Hmm. I kind of liked having each option in just one class, but I see
>>> the problem. Looking at the ones you suggest, all the non-password
>>> ones *have* to be without password, otherwise there i no way to get
>>> the conninfo without password - which is the whole reason for that
>>> parameter to exist. So the PASSWORD one has to be additional - which
>>> means that not making the other ones additional makes them
>>> inconsistent. But maybe we don't really have a choice there.
>>
>>
>> Yes, the PASSWORD part can be on its own, this is what I meant above
>> but wanted a different opinion about having it completely separate
>> is better or not.
>>
>> But the NORMAL and REPLICATION (or WALRECEIVER) classes
>> need to overlap.
>>
>>>>> At this point, the patch is untested beyond compiling and a trivial
>>>>> psql check, because I ran out of time :) But I figured I'd throw it
>>>>> out there for comments on which version people prefer. (And yes, it's
>>>>> quite possible I've made a big think-mistake in it somewhere, but
>>>>> again, better to get some eyes on it early)
>>>>>
>>>>> My version also contains a fixed version of the docs that should be
>>>>> moved back into Zoltans version if that's the one we end up
>>>>> preferring.
>>>>
>>>>
>>>> I also liked your version of the documentation better,
>>>> I am not too good at writing docs.
>>>
>>> np.
>>>
>>>
>>>>> Also, a question was buried in the other review which is - are we OK
>>>>> to remove the requiressl parameter. Both these patches do so, because
>>>>> the code becomes much simpler if we can do that. It has been
>>>>> deprecated since 7.2. Is it OK to remove it, or do we need to put back
>>>>> in the more complex code to deal with both?
>>>
>>> Just going to highlight that we're looking for at least one third
>>> party to comment on this :)
>>
>>
>> Yes, me too. A +1 for removing wouldn't count from me. ;-)
>>
>>>
>>>>> Attached is both Zoltans latest patch (v16) and my slightly different
>>>>> approach.
>>>>>
>>>>> Comments on which approach is best?
>>>>>
>>>>> Test results from somebody who has the time to look at it? :)
>>>
>>> Do you happen to have a set of tests you've been running on your
>>> patches? Can you try them again this one?
>>
>>
>> My set of tests are:
>>
>> 1. initdb the master
>> 2. pg_basebackup -R the first standby from the master
>> 3. pg_basebackup -R the second standby from the master
>> 4. pg_basebackup -R the third standby from the first standby
>>
>> and diff -durpN the different data directories while there is no load on
>> either.
>>
>> I will test it today after fixing the classes in your patch. ;-)
>
>
> Attached is my v17 patch based on your version with the classes fixed.
>
> I introduced 2 new classes: PG_CONNINFO_REPLICATION_HIDDEN
> which contains "dbname" and "fallback_application_name, and
> PG_CONNINFO_REPLICATION_USER which contains everything else
> except "replication".
>
> Since pg_basebackup sets fallback_application_name and not
> application_name, this latter can be part of PG_CONNINFO_REPLICATION_USER.

I'm still not sure I like this. The "since pg_basebackup sets" part
really points to the problem - we're designing a public API for just
internal options here, aren't we? Is there *any* other application
that would ever use this? And if we do it like this, changing how
pg_basebackup works will make a change to our public libpq APIs in
worst case.

fallback_application_name certainly has *nothing* to do with
replication, in principle.

I'm thinking it might be a better idea to just have PG_CONNINFO_NORMAL
and PG_CONNINFO_PASSWORD (which still make sense to separate), and
then plug in the exclusion of specific parameters in pg_basebackup, in
the CreateRecoveryConf() function. pg_basebackup will of course always
know what pg_basebackup is doing with these things.

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/