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

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.


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



>> 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 :)


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

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