On Wed, Oct 19, 2016 at 12:04 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Thu, Oct 13, 2016 at 5:53 AM, Victor Wagner <vitus@wagner.pp.ru> wrote:
>> On Thu, 13 Oct 2016 12:30:59 +0530
>> Mithun Cy <mithun.cy@enterprisedb.com> wrote:
>>> On Fri, Sep 30, 2016 at 2:14 PM, Victor Wagner <vitus@wagner.pp.ru>
>>> wrote:
>>> Okay but for me consistency is also important. Since we agree to
>>> disagree on some of the comments and others have not expressed any
>>> problem I am moving it to committer.
>>
>> Thank you for your efforts improving my patch
>
> I haven't deeply reviewed this patch, but on a quick read-through it
> certainly seems to need a lot of cleanup work.
Some more comments:
- I am pretty doubtful that the changes to connectOptions2() work as
intended. I think that's only going to be called before actualhost
and actualport could possibly get set. I don't think we keep
reinvoking this function for every new host we try.
- It's pretty clear that this isn't going to work if the host list
includes a mix of hostnames and UNIX socket addresses. The code that
handles the UNIX socket address case is totally separate from the code
that handles the multiple-hostname case.
- This creates a sort of definitional problem for functions like
PQhost(). The documentation says of this and other related functions:
"These values are fixed for the life of the PGconn object." But with
this patch, that would no longer be true. So at least the
documentation needs an update. Actually, though, I think the problem
is more fundamental than that. If the user asks for PQhost(conn), do
they want the list of all hosts, or the one to which we're currently
connected? It might be either, depending on what they intend to do
with the information. If they mean to construct a new connection
string, they might well want them all; but something like psql's
\conninfo only shows the current one. There's also the question of
what "the current one" means if we're not connected to any server at
all. I'm not sure exactly how to solve these problems, but they need
some thought.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
