Re: PQconninfo function for libpq - Mailing list pgsql-hackers

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

The REPLICATION and REPLICATION_HIDDEN options may be united,
I don't have a strong opinion on it.

I also flipped the "ignoreMissing" argument to conninfo_storeval()
from false to true in PQconninfo() since we don't want error
reporting there.

This patch works with my pg_basebackup patch after fixing the flags value.

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/