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

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?
These are added by the walreceiver module anyway and adding them
to the primary_conninfo line should even be discouraged by the documentation.

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.

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

> 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?
>
> 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? :)
>
> --
>   Magnus Hagander
>   Me: http://www.hagander.net/
>   Work: http://www.redpill-linpro.com/

Thanks for four work on this.

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/