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

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

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


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