Re: PQconninfo function for libpq

2012-11-22 12:44 keltezéssel, Magnus Hagander írta:
> 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?

I was thinking about this pg_receivexlog application but PQconninfo
is not needed there.

It would be a nice libpq extension to be able to login using a
PQconninfoOption array instead of the keyword/value arrays.
But the replication/normal distinction is not needed there either.

> 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.

OK, I will post a new pg_basebackup patch with this in mind
in its own thread.

Anyway, here are two small patches, the first is against your
previous patch to fix a typo and and the ignoreMissing argument
to conninfo_storeval().

The second one is the product of what caught my attention while
I was looking at pg_receivexlog. The current coding may write
beyond the end of the allocated arrays and the password may
overwrite a previously set keyword/value pair.

Best regards,
Zoltán Böszörményi

--
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
http://www.postgresql.at/