Re: -d option for pg_isready is broken

handyrep(at)john:~/handyrep$ pg_isready --version
pg_isready (PostgreSQL) 9.3.1

handyrep(at)john:~/handyrep$ pg_isready -h john -p 5432 -U postgres -d
postgres -q
pg_isready: missing "=" after "postgres" in connection info string

handyrep(at)john:~/handyrep$ pg_isready --host=john --port=5432
--user=postgres --dbname=postgres
pg_isready: missing "=" after "postgres" in connection info string

handyrep(at)john:~/handyrep$ pg_isready -h john -p 5432 -U postgres
john:5432 - accepting connections

so apparently the -d option:

a) doesn't work, and
b) doesn't do anything

I suggest simply removing it from the utility.

I'll note that the -U option doesn't appear to do anything relevant
either, but at least it doesn't error unnecessarily:

handyrep(at)john:~/handyrep$ pg_isready -h john -p 5432 -U no_such_user
john:5432 - accepting connections

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

On Wed, Nov 13, 2013 at 9:37 PM, Josh Berkus <josh(at)agliodbs(dot)com> wrote:

>
> handyrep(at)john:~/handyrep$ pg_isready --version
> pg_isready (PostgreSQL) 9.3.1
>
> handyrep(at)john:~/handyrep$ pg_isready -h john -p 5432 -U postgres -d
> postgres -q
> pg_isready: missing "=" after "postgres" in connection info string
>
> handyrep(at)john:~/handyrep$ pg_isready --host=john --port=5432
> --user=postgres --dbname=postgres
> pg_isready: missing "=" after "postgres" in connection info string
>
> handyrep(at)john:~/handyrep$ pg_isready -h john -p 5432 -U postgres
> john:5432 - accepting connections
>
> so apparently the -d option:
>
> a) doesn't work, and
> b) doesn't do anything
>
> I suggest simply removing it from the utility.
>
> I'll note that the -U option doesn't appear to do anything relevant
> either, but at least it doesn't error unnecessarily:
>
> handyrep(at)john:~/handyrep$ pg_isready -h john -p 5432 -U no_such_user
> john:5432 - accepting connections
>
>
The attached patch fix it.

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog sobre TI: http://fabriziomello.blogspot.com
>> Perfil Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello

On Fri, Nov 15, 2013 at 9:01 PM, Fabrízio de Royes Mello
<fabriziomello(at)gmail(dot)com> wrote:
> On Wed, Nov 13, 2013 at 9:37 PM, Josh Berkus <josh(at)agliodbs(dot)com> wrote:
>> handyrep(at)john:~/handyrep$ pg_isready --version
>> pg_isready (PostgreSQL) 9.3.1
>>
>> handyrep(at)john:~/handyrep$ pg_isready -h john -p 5432 -U postgres -d
>> postgres -q
>> pg_isready: missing "=" after "postgres" in connection info string
>>
>> handyrep(at)john:~/handyrep$ pg_isready --host=john --port=5432
>> --user=postgres --dbname=postgres
>> pg_isready: missing "=" after "postgres" in connection info string
>>
>> handyrep(at)john:~/handyrep$ pg_isready -h john -p 5432 -U postgres
>> john:5432 - accepting connections
>>
>> so apparently the -d option:
>>
>> a) doesn't work, and
>> b) doesn't do anything
>>
>> I suggest simply removing it from the utility.
>>
>> I'll note that the -U option doesn't appear to do anything relevant
>> either, but at least it doesn't error unnecessarily:
>>
>> handyrep(at)john:~/handyrep$ pg_isready -h john -p 5432 -U no_such_user
>> john:5432 - accepting connections
>>
>
> The attached patch fix it.

Well, there still seem to be some problems.

[rhaas pgsql]$ src/bin/scripts/pg_isready -d host=sgdasasg
/tmp:5432 - no attempt

I added some debugging instrumentation and it looks like there's a
problem with this code:

if (strcmp(def->keyword, "hostaddr") == 0 ||
strcmp(def->keyword, "host") == 0)

The problem is that we end up executing the code contained in that
block twice, once for host and once for hostaddr. Thus:

[rhaas pgsql]$ src/bin/scripts/pg_isready -d host=sgdasasg
def->keyword=host pghost_str=sgdasasg
def->keyword=hostaddr pghost_str=/tmp
def->keyword=port pgport_str=5432
/tmp:5432 - no attempt

Oops.

Nevertheless, that's kind of a separate problem, so we could address
in a separate commit. But even ignoring that, this still isn't right:
according to the fine documentation, -d will be expanded not only if
it contains an =, but also if it starts with a valid URI prefix. This
would break that documented behavior.

http://www.postgresql.org/docs/9.3/static/app-pg-isready.html

If you submit an updated patch, please fix the comment just above the
code you're changing to more accurately reflect what this does.

The number of bugs in pg_isready certainly seems out of proportion to
the surface area of the problem it solves. Whee!

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Tue, Nov 19, 2013 at 11:51 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Fri, Nov 15, 2013 at 9:01 PM, Fabrízio de Royes Mello
> <fabriziomello(at)gmail(dot)com> wrote:
>> On Wed, Nov 13, 2013 at 9:37 PM, Josh Berkus <josh(at)agliodbs(dot)com> wrote:
>>> handyrep(at)john:~/handyrep$ pg_isready --version
>>> pg_isready (PostgreSQL) 9.3.1
>>>
>>> handyrep(at)john:~/handyrep$ pg_isready -h john -p 5432 -U postgres -d
>>> postgres -q
>>> pg_isready: missing "=" after "postgres" in connection info string
>>>
>>> handyrep(at)john:~/handyrep$ pg_isready --host=john --port=5432
>>> --user=postgres --dbname=postgres
>>> pg_isready: missing "=" after "postgres" in connection info string
>>>
>>> handyrep(at)john:~/handyrep$ pg_isready -h john -p 5432 -U postgres
>>> john:5432 - accepting connections
>>>
>>> so apparently the -d option:
>>>
>>> a) doesn't work, and
>>> b) doesn't do anything
>>>
>>> I suggest simply removing it from the utility.
>>>
>>> I'll note that the -U option doesn't appear to do anything relevant
>>> either, but at least it doesn't error unnecessarily:
>>>
>>> handyrep(at)john:~/handyrep$ pg_isready -h john -p 5432 -U no_such_user
>>> john:5432 - accepting connections
>>>
>>
>> The attached patch fix it.
>
> Well, there still seem to be some problems.
>
> [rhaas pgsql]$ src/bin/scripts/pg_isready -d host=sgdasasg
> /tmp:5432 - no attempt
>
> I added some debugging instrumentation and it looks like there's a
> problem with this code:
>
> if (strcmp(def->keyword, "hostaddr") == 0 ||
> strcmp(def->keyword, "host") == 0)
>
> The problem is that we end up executing the code contained in that
> block twice, once for host and once for hostaddr. Thus:
>
> [rhaas pgsql]$ src/bin/scripts/pg_isready -d host=sgdasasg
> def->keyword=host pghost_str=sgdasasg
> def->keyword=hostaddr pghost_str=/tmp
> def->keyword=port pgport_str=5432
> /tmp:5432 - no attempt
>
> Oops.
>
> Nevertheless, that's kind of a separate problem, so we could address
> in a separate commit. But even ignoring that, this still isn't right:
> according to the fine documentation, -d will be expanded not only if
> it contains an =, but also if it starts with a valid URI prefix. This
> would break that documented behavior.
>
> http://www.postgresql.org/docs/9.3/static/app-pg-isready.html

Attached is the updated version of the patch. Is there any other bug?

Regards,

--
Fujii Masao

On Tue, Nov 19, 2013 at 1:10 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
> On Tue, Nov 19, 2013 at 11:51 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> On Fri, Nov 15, 2013 at 9:01 PM, Fabrízio de Royes Mello
>> <fabriziomello(at)gmail(dot)com> wrote:
>>> On Wed, Nov 13, 2013 at 9:37 PM, Josh Berkus <josh(at)agliodbs(dot)com> wrote:
>>>> handyrep(at)john:~/handyrep$ pg_isready --version
>>>> pg_isready (PostgreSQL) 9.3.1
>>>>
>>>> handyrep(at)john:~/handyrep$ pg_isready -h john -p 5432 -U postgres -d
>>>> postgres -q
>>>> pg_isready: missing "=" after "postgres" in connection info string
>>>>
>>>> handyrep(at)john:~/handyrep$ pg_isready --host=john --port=5432
>>>> --user=postgres --dbname=postgres
>>>> pg_isready: missing "=" after "postgres" in connection info string
>>>>
>>>> handyrep(at)john:~/handyrep$ pg_isready -h john -p 5432 -U postgres
>>>> john:5432 - accepting connections
>>>>
>>>> so apparently the -d option:
>>>>
>>>> a) doesn't work, and
>>>> b) doesn't do anything
>>>>
>>>> I suggest simply removing it from the utility.
>>>>
>>>> I'll note that the -U option doesn't appear to do anything relevant
>>>> either, but at least it doesn't error unnecessarily:
>>>>
>>>> handyrep(at)john:~/handyrep$ pg_isready -h john -p 5432 -U no_such_user
>>>> john:5432 - accepting connections
>>>>
>>>
>>> The attached patch fix it.
>>
>> Well, there still seem to be some problems.
>>
>> [rhaas pgsql]$ src/bin/scripts/pg_isready -d host=sgdasasg
>> /tmp:5432 - no attempt
>>
>> I added some debugging instrumentation and it looks like there's a
>> problem with this code:
>>
>> if (strcmp(def->keyword, "hostaddr") == 0 ||
>> strcmp(def->keyword, "host") == 0)
>>
>> The problem is that we end up executing the code contained in that
>> block twice, once for host and once for hostaddr. Thus:
>>
>> [rhaas pgsql]$ src/bin/scripts/pg_isready -d host=sgdasasg
>> def->keyword=host pghost_str=sgdasasg
>> def->keyword=hostaddr pghost_str=/tmp
>> def->keyword=port pgport_str=5432
>> /tmp:5432 - no attempt
>>
>> Oops.
>>
>> Nevertheless, that's kind of a separate problem, so we could address
>> in a separate commit. But even ignoring that, this still isn't right:
>> according to the fine documentation, -d will be expanded not only if
>> it contains an =, but also if it starts with a valid URI prefix. This
>> would break that documented behavior.
>>
>> http://www.postgresql.org/docs/9.3/static/app-pg-isready.html
>
> Attached is the updated version of the patch. Is there any other bug?

Not that I can see, but it's not very future-proof. If libpq changes
its idea of what will provoke database-name expansion, this will again
be broken.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 11/19/2013 10:12 AM, Robert Haas wrote:
> On Tue, Nov 19, 2013 at 1:10 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>> On Tue, Nov 19, 2013 at 11:51 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>> On Fri, Nov 15, 2013 at 9:01 PM, Fabrízio de Royes Mello
>>> http://www.postgresql.org/docs/9.3/static/app-pg-isready.html
>>
>> Attached is the updated version of the patch. Is there any other bug?
>
> Not that I can see, but it's not very future-proof. If libpq changes
> its idea of what will provoke database-name expansion, this will again
> be broken.

Why aren't we using the exact same code as psql? Why does pg_isready
have its own code for this?

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

On Tue, Nov 19, 2013 at 1:22 PM, Josh Berkus <josh(at)agliodbs(dot)com> wrote:
> On 11/19/2013 10:12 AM, Robert Haas wrote:
>> On Tue, Nov 19, 2013 at 1:10 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>>> On Tue, Nov 19, 2013 at 11:51 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>>> On Fri, Nov 15, 2013 at 9:01 PM, Fabrízio de Royes Mello
>>>> http://www.postgresql.org/docs/9.3/static/app-pg-isready.html
>>>
>>> Attached is the updated version of the patch. Is there any other bug?
>>
>> Not that I can see, but it's not very future-proof. If libpq changes
>> its idea of what will provoke database-name expansion, this will again
>> be broken.
>
> Why aren't we using the exact same code as psql? Why does pg_isready
> have its own code for this?

Because pg_isready wants to print the host and port we actually tried
to connect to, which no other utility does. Turns out, there's no
clean API for that. We tried to invent something, but the evidence
seems to indicate that what we invented bites.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Wed, Nov 20, 2013 at 3:12 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Tue, Nov 19, 2013 at 1:10 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>> On Tue, Nov 19, 2013 at 11:51 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>> On Fri, Nov 15, 2013 at 9:01 PM, Fabrízio de Royes Mello
>>> <fabriziomello(at)gmail(dot)com> wrote:
>>>> On Wed, Nov 13, 2013 at 9:37 PM, Josh Berkus <josh(at)agliodbs(dot)com> wrote:
>>>>> handyrep(at)john:~/handyrep$ pg_isready --version
>>>>> pg_isready (PostgreSQL) 9.3.1
>>>>>
>>>>> handyrep(at)john:~/handyrep$ pg_isready -h john -p 5432 -U postgres -d
>>>>> postgres -q
>>>>> pg_isready: missing "=" after "postgres" in connection info string
>>>>>
>>>>> handyrep(at)john:~/handyrep$ pg_isready --host=john --port=5432
>>>>> --user=postgres --dbname=postgres
>>>>> pg_isready: missing "=" after "postgres" in connection info string
>>>>>
>>>>> handyrep(at)john:~/handyrep$ pg_isready -h john -p 5432 -U postgres
>>>>> john:5432 - accepting connections
>>>>>
>>>>> so apparently the -d option:
>>>>>
>>>>> a) doesn't work, and
>>>>> b) doesn't do anything
>>>>>
>>>>> I suggest simply removing it from the utility.
>>>>>
>>>>> I'll note that the -U option doesn't appear to do anything relevant
>>>>> either, but at least it doesn't error unnecessarily:
>>>>>
>>>>> handyrep(at)john:~/handyrep$ pg_isready -h john -p 5432 -U no_such_user
>>>>> john:5432 - accepting connections
>>>>>
>>>>
>>>> The attached patch fix it.
>>>
>>> Well, there still seem to be some problems.
>>>
>>> [rhaas pgsql]$ src/bin/scripts/pg_isready -d host=sgdasasg
>>> /tmp:5432 - no attempt
>>>
>>> I added some debugging instrumentation and it looks like there's a
>>> problem with this code:
>>>
>>> if (strcmp(def->keyword, "hostaddr") == 0 ||
>>> strcmp(def->keyword, "host") == 0)
>>>
>>> The problem is that we end up executing the code contained in that
>>> block twice, once for host and once for hostaddr. Thus:
>>>
>>> [rhaas pgsql]$ src/bin/scripts/pg_isready -d host=sgdasasg
>>> def->keyword=host pghost_str=sgdasasg
>>> def->keyword=hostaddr pghost_str=/tmp
>>> def->keyword=port pgport_str=5432
>>> /tmp:5432 - no attempt
>>>
>>> Oops.
>>>
>>> Nevertheless, that's kind of a separate problem, so we could address
>>> in a separate commit. But even ignoring that, this still isn't right:
>>> according to the fine documentation, -d will be expanded not only if
>>> it contains an =, but also if it starts with a valid URI prefix. This
>>> would break that documented behavior.
>>>
>>> http://www.postgresql.org/docs/9.3/static/app-pg-isready.html
>>
>> Attached is the updated version of the patch. Is there any other bug?
>
> Not that I can see, but it's not very future-proof. If libpq changes
> its idea of what will provoke database-name expansion, this will again
> be broken.

Yeah, I agree that we should make the logic of pg_isready more future-proof
in 9.4dev. One idea is to expose internal_ping() as a libpq function. Then,
instead of just calling PQpingParams(), we can do PQconnectStartParams(),
libpq-function-version-of-internal_ping(), PQhost(), PQport(), and PQfinish().
That is, we get to know the host and port information by calling
PQhost() and PQport(),
after trying to connect to the server.

But we cannot use this idea in 9.3 because it's too late to add new
libpq function there.
Also I don't think that the minor version release would change that
libpq's logic in 9.3.
So, barring any objections, I will commit the latest version of the
patch in 9.3.

Regards,

--
Fujii Masao

On 11/20/2013 01:55 AM, Fujii Masao wrote:
> Yeah, I agree that we should make the logic of pg_isready more future-proof
> in 9.4dev. One idea is to expose internal_ping() as a libpq function. Then,
> instead of just calling PQpingParams(), we can do PQconnectStartParams(),
> libpq-function-version-of-internal_ping(), PQhost(), PQport(), and PQfinish().
> That is, we get to know the host and port information by calling
> PQhost() and PQport(),
> after trying to connect to the server.

+1

This would allow writers of client drivers to implement their own
pg_ping() functions instead of needing to go through the shell (as I
currently do with pg_isready).

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

On Wed, Nov 20, 2013 at 4:55 AM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>> Not that I can see, but it's not very future-proof. If libpq changes
>> its idea of what will provoke database-name expansion, this will again
>> be broken.
>
> Yeah, I agree that we should make the logic of pg_isready more future-proof
> in 9.4dev. One idea is to expose internal_ping() as a libpq function. Then,
> instead of just calling PQpingParams(), we can do PQconnectStartParams(),
> libpq-function-version-of-internal_ping(), PQhost(), PQport(), and PQfinish().
> That is, we get to know the host and port information by calling
> PQhost() and PQport(),
> after trying to connect to the server.

Hmm, that sounds like a possibly promising approach.

> But we cannot use this idea in 9.3 because it's too late to add new
> libpq function there.
> Also I don't think that the minor version release would change that
> libpq's logic in 9.3.
> So, barring any objections, I will commit the latest version of the
> patch in 9.3.

I think you should commit it to both master and REL9_3_STABLE. Then,
you can make further changes to master in a subsequent commit.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Thu, Nov 21, 2013 at 6:41 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Wed, Nov 20, 2013 at 4:55 AM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>>> Not that I can see, but it's not very future-proof. If libpq changes
>>> its idea of what will provoke database-name expansion, this will again
>>> be broken.
>>
>> Yeah, I agree that we should make the logic of pg_isready more future-proof
>> in 9.4dev. One idea is to expose internal_ping() as a libpq function. Then,
>> instead of just calling PQpingParams(), we can do PQconnectStartParams(),
>> libpq-function-version-of-internal_ping(), PQhost(), PQport(), and PQfinish().
>> That is, we get to know the host and port information by calling
>> PQhost() and PQport(),
>> after trying to connect to the server.
>
> Hmm, that sounds like a possibly promising approach.
>
>> But we cannot use this idea in 9.3 because it's too late to add new
>> libpq function there.
>> Also I don't think that the minor version release would change that
>> libpq's logic in 9.3.
>> So, barring any objections, I will commit the latest version of the
>> patch in 9.3.
>
> I think you should commit it to both master and REL9_3_STABLE.

Committed.

> Then,
> you can make further changes to master in a subsequent commit.

Yeah, I will implement that patch.

Regards,

--
Fujii Masao

On Thu, Nov 21, 2013 at 9:58 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
> On Thu, Nov 21, 2013 at 6:41 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> On Wed, Nov 20, 2013 at 4:55 AM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>>>> Not that I can see, but it's not very future-proof. If libpq changes
>>>> its idea of what will provoke database-name expansion, this will again
>>>> be broken.
>>>
>>> Yeah, I agree that we should make the logic of pg_isready more future-proof
>>> in 9.4dev. One idea is to expose internal_ping() as a libpq function. Then,
>>> instead of just calling PQpingParams(), we can do PQconnectStartParams(),
>>> libpq-function-version-of-internal_ping(), PQhost(), PQport(), and PQfinish().
>>> That is, we get to know the host and port information by calling
>>> PQhost() and PQport(),
>>> after trying to connect to the server.

While I was investigaing PQhost() for that approach, I found several
problems of PQhost().

(1) PQhost() can return Unix-domain socket directory path even in the
platform that
doesn't support Unix-domain socket.

(2) In the platform that doesn't support Unix-domain socket, when
neither host nor hostaddr
are specified, the default host 'localhost' is used to connect to
the server and
PQhost() must return that, but it doesn't.

(3) PQhost() cannot return the hostaddr.

As the result of these problems, you can see the incorrect result of
\conninfo, for example.

$ psql -d "hostaddr=127.0.0.1"
=# \conninfo
You are connected to database "postgres" as user "postgres" via socket
in "/tmp" at port "5432".

Obviously "/tmp" should be "127.0.0.1".

The attached patch fixes these problems of PQhost(). But I'm concerned
about that
this change may break the existing application using PQhost(). That is, some
existing application might not assume that PQhost() returns hostaddr.
If my concern
is true, maybe we need to implement something like PQhostaddr(). It's too late
to add such new libpq function into the current stable release,
though... Thought?

If it's okay to change the behavior of PQhost() as I explained, I will commit
the patch in all supported versions.

Regards,

--
Fujii Masao

On Tue, Dec 10, 2013 at 11:40 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
> While I was investigaing PQhost() for that approach, I found several
> problems of PQhost().
>
> (1) PQhost() can return Unix-domain socket directory path even in the
> platform that
> doesn't support Unix-domain socket.
>
> (2) In the platform that doesn't support Unix-domain socket, when
> neither host nor hostaddr
> are specified, the default host 'localhost' is used to connect to
> the server and
> PQhost() must return that, but it doesn't.

I think changing PQhost() so that it returns DefaultHost rather than
conn->pgunixsocket when we don't HAVE_UNIX_SOCKETS is a back-patchable
bug fix, and I'd say go for it.

> (3) PQhost() cannot return the hostaddr.

However, I'm much less sure whether this is something that we want to
do at all. It seems like this might be a definition of what the
function does, and I'm not sure whether the new definition is what
everyone will want. On the other hand, I'm also not sure that it
isn't.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Wed, Dec 11, 2013 at 10:26 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Tue, Dec 10, 2013 at 11:40 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>> While I was investigaing PQhost() for that approach, I found several
>> problems of PQhost().
>>
>> (1) PQhost() can return Unix-domain socket directory path even in the
>> platform that
>> doesn't support Unix-domain socket.
>>
>> (2) In the platform that doesn't support Unix-domain socket, when
>> neither host nor hostaddr
>> are specified, the default host 'localhost' is used to connect to
>> the server and
>> PQhost() must return that, but it doesn't.
>
> I think changing PQhost() so that it returns DefaultHost rather than
> conn->pgunixsocket when we don't HAVE_UNIX_SOCKETS is a back-patchable
> bug fix, and I'd say go for it.

Agreed.

>> (3) PQhost() cannot return the hostaddr.
>
> However, I'm much less sure whether this is something that we want to
> do at all. It seems like this might be a definition of what the
> function does, and I'm not sure whether the new definition is what
> everyone will want. On the other hand, I'm also not sure that it
> isn't.

If we don't change PQhost() in that way, we cannot fix the following problem
of wrong output of \conninfo, for example.

$ psql -d "hostaddr=127.0.0.1"
=# \conninfo
You are connected to database "postgres" as user "postgres" via socket
in "/tmp" at port "5432".

Regards.

--
Fujii Masao

On Wed, Dec 11, 2013 at 8:45 AM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
> On Wed, Dec 11, 2013 at 10:26 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> On Tue, Dec 10, 2013 at 11:40 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>>> While I was investigaing PQhost() for that approach, I found several
>>> problems of PQhost().
>>>
>>> (1) PQhost() can return Unix-domain socket directory path even in the
>>> platform that
>>> doesn't support Unix-domain socket.
>>>
>>> (2) In the platform that doesn't support Unix-domain socket, when
>>> neither host nor hostaddr
>>> are specified, the default host 'localhost' is used to connect to
>>> the server and
>>> PQhost() must return that, but it doesn't.
>>
>> I think changing PQhost() so that it returns DefaultHost rather than
>> conn->pgunixsocket when we don't HAVE_UNIX_SOCKETS is a back-patchable
>> bug fix, and I'd say go for it.
>
> Agreed.
>
>>> (3) PQhost() cannot return the hostaddr.
>>
>> However, I'm much less sure whether this is something that we want to
>> do at all. It seems like this might be a definition of what the
>> function does, and I'm not sure whether the new definition is what
>> everyone will want. On the other hand, I'm also not sure that it
>> isn't.
>
> If we don't change PQhost() in that way, we cannot fix the following problem
> of wrong output of \conninfo, for example.
>
> $ psql -d "hostaddr=127.0.0.1"
> =# \conninfo
> You are connected to database "postgres" as user "postgres" via socket
> in "/tmp" at port "5432".

Yeah, that's true. But the whole point of having both host and
hostaddr seems to be that you can lie about where you're connecting.
If you set host=some.pretty.domain.name hostaddr=1.2.3.4, the point is
to say that you're connecting to the first while, under the covers,
actually connecting to the second. Now, I am unclear what value this
has, but someone at some point evidently thought it was a good idea,
so we need to be careful about changing it.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2013-12-11 08:56:43 -0500, Robert Haas wrote:
> > $ psql -d "hostaddr=127.0.0.1"
> > =# \conninfo
> > You are connected to database "postgres" as user "postgres" via socket
> > in "/tmp" at port "5432".
>
> Yeah, that's true. But the whole point of having both host and
> hostaddr seems to be that you can lie about where you're connecting.
> If you set host=some.pretty.domain.name hostaddr=1.2.3.4, the point is
> to say that you're connecting to the first while, under the covers,
> actually connecting to the second. Now, I am unclear what value this
> has, but someone at some point evidently thought it was a good idea,
> so we need to be careful about changing it.

One use case is accessing a particular host when using DNS round robin
to standbys in combination with SSL.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Wed, Dec 11, 2013 at 9:35 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> On 2013-12-11 08:56:43 -0500, Robert Haas wrote:
>> > $ psql -d "hostaddr=127.0.0.1"
>> > =# \conninfo
>> > You are connected to database "postgres" as user "postgres" via socket
>> > in "/tmp" at port "5432".
>>
>> Yeah, that's true. But the whole point of having both host and
>> hostaddr seems to be that you can lie about where you're connecting.
>> If you set host=some.pretty.domain.name hostaddr=1.2.3.4, the point is
>> to say that you're connecting to the first while, under the covers,
>> actually connecting to the second. Now, I am unclear what value this
>> has, but someone at some point evidently thought it was a good idea,
>> so we need to be careful about changing it.
>
> One use case is accessing a particular host when using DNS round robin
> to standbys in combination with SSL.

Ah, interesting point. And it's not inconceivable that some
application out there could be using PQhost() to retrieve the host
from an existing connection object and reusing that value for a new
connection, in which case redefining it to sometimes return hostaddr
would break things. So I think we shouldn't do that.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Wed, Dec 11, 2013 at 9:35 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>> One use case is accessing a particular host when using DNS round robin
>> to standbys in combination with SSL.

> Ah, interesting point. And it's not inconceivable that some
> application out there could be using PQhost() to retrieve the host
> from an existing connection object and reusing that value for a new
> connection, in which case redefining it to sometimes return hostaddr
> would break things. So I think we shouldn't do that.

I think the only reasonable way to fix this is to improve the logic
in psql, not turn PQhost() into a mess with no understandable definition.
If that means we need to add a separate PQhostaddr() query function,
so be it. We won't be able to fix the complained-of bug in back branches,
but I'd rather live with that (it's basically just cosmetic anyway)
than risk introducing perhaps-not-so-cosmetic bugs into other existing
applications.

In general, I think the definition of these query functions ought to
be "what was the value of this parameter when the connection was made".
As such, I'm not even sure that the pgunixsocket behavior that's in
PQhost now is a good idea, much less that we should extend that hack
to cover DefaultHost.

There is room also for a function defined as "give me a textual
description of what I'm connected to", which is not meant to reflect any
single connection parameter but rather the total behavior. Right now
I think PQhost is on the borderline of doing this instead of just
reporting the "host" parameter, but I think rather than pushing it
across that border we'd be better off to invent a function explicitly
charged with doing that. That would give us room to do something
actually meaningful with host+hostaddr cases, for instance. I think
really what ought to be printed in such cases is something like
"host-name (address IP-address)"; leaving out the former would be
unhelpful but leaving out the latter is outright misleading.

On the other hand, I'm not sure how much of a translatability problem
it'd be to wedge such a description into a larger message. Might be
better to just put the logic into psql.

regards, tom lane

On Wed, Dec 11, 2013 at 11:24 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Wed, Dec 11, 2013 at 9:35 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>>> One use case is accessing a particular host when using DNS round robin
>>> to standbys in combination with SSL.
>
>> Ah, interesting point. And it's not inconceivable that some
>> application out there could be using PQhost() to retrieve the host
>> from an existing connection object and reusing that value for a new
>> connection, in which case redefining it to sometimes return hostaddr
>> would break things. So I think we shouldn't do that.
>
> I think the only reasonable way to fix this is to improve the logic
> in psql, not turn PQhost() into a mess with no understandable definition.
> If that means we need to add a separate PQhostaddr() query function,
> so be it. We won't be able to fix the complained-of bug in back branches,
> but I'd rather live with that (it's basically just cosmetic anyway)
> than risk introducing perhaps-not-so-cosmetic bugs into other existing
> applications.

I can't argue with that.

> In general, I think the definition of these query functions ought to
> be "what was the value of this parameter when the connection was made".
> As such, I'm not even sure that the pgunixsocket behavior that's in
> PQhost now is a good idea, much less that we should extend that hack
> to cover DefaultHost.

Well, returning /tmp on Windows is just stupid. I don't see why we
should feel bad about changing that. A bug is a bug.

> There is room also for a function defined as "give me a textual
> description of what I'm connected to", which is not meant to reflect any
> single connection parameter but rather the total behavior. Right now
> I think PQhost is on the borderline of doing this instead of just
> reporting the "host" parameter, but I think rather than pushing it
> across that border we'd be better off to invent a function explicitly
> charged with doing that. That would give us room to do something
> actually meaningful with host+hostaddr cases, for instance. I think
> really what ought to be printed in such cases is something like
> "host-name (address IP-address)"; leaving out the former would be
> unhelpful but leaving out the latter is outright misleading.
>
> On the other hand, I'm not sure how much of a translatability problem
> it'd be to wedge such a description into a larger message. Might be
> better to just put the logic into psql.

libpq needs to expose enough functionality to make this simple for
psql, but psql should be the final arbiter of the output format.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Wed, Dec 11, 2013 at 11:24 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> In general, I think the definition of these query functions ought to
>> be "what was the value of this parameter when the connection was made".
>> As such, I'm not even sure that the pgunixsocket behavior that's in
>> PQhost now is a good idea, much less that we should extend that hack
>> to cover DefaultHost.

> Well, returning /tmp on Windows is just stupid. I don't see why we
> should feel bad about changing that. A bug is a bug.

What I was suggesting was we should take out the pgunixsocket fallback,
not make it even more complicated. That probably implies that we need
still another accessor function to get the socket path.

regards, tom lane

On Wed, Dec 11, 2013 at 2:10 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Wed, Dec 11, 2013 at 11:24 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> In general, I think the definition of these query functions ought to
>>> be "what was the value of this parameter when the connection was made".
>>> As such, I'm not even sure that the pgunixsocket behavior that's in
>>> PQhost now is a good idea, much less that we should extend that hack
>>> to cover DefaultHost.
>
>> Well, returning /tmp on Windows is just stupid. I don't see why we
>> should feel bad about changing that. A bug is a bug.
>
> What I was suggesting was we should take out the pgunixsocket fallback,
> not make it even more complicated. That probably implies that we need
> still another accessor function to get the socket path.

Well, I guess. I have a hard time seeing whatever rejiggering we want
to do in master as a reason not to back-patch that fix, though.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Wed, Dec 11, 2013 at 2:10 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>>> Well, returning /tmp on Windows is just stupid. I don't see why we
>>> should feel bad about changing that. A bug is a bug.

>> What I was suggesting was we should take out the pgunixsocket fallback,
>> not make it even more complicated. That probably implies that we need
>> still another accessor function to get the socket path.

> Well, I guess. I have a hard time seeing whatever rejiggering we want
> to do in master as a reason not to back-patch that fix, though.

I guess as long as the pgunixsocket thing is in there, it makes sense
to substitute DefaultHost for it on Windows, but are we sure that's
something to back-patch?

Right now, as I was saying, PQhost is in some gray area where it's not too
clear what its charter is. It's not "what was the host parameter", for
sure, but we haven't tried to make it an accurate description of the
connection either. It's a bit less accurate on Windows than elsewhere,
but do we want to risk breaking anything to only partially resolve that?

More generally, if we do go over in 9.4 to the position that PQhost
reports the host parameter and nothing but, I'm not sure that introducing
a third behavior into the back branches is something that anybody will
thank us for.

regards, tom lane

On Wed, Dec 11, 2013 at 2:29 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Wed, Dec 11, 2013 at 2:10 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>>>> Well, returning /tmp on Windows is just stupid. I don't see why we
>>>> should feel bad about changing that. A bug is a bug.
>
>>> What I was suggesting was we should take out the pgunixsocket fallback,
>>> not make it even more complicated. That probably implies that we need
>>> still another accessor function to get the socket path.
>
>> Well, I guess. I have a hard time seeing whatever rejiggering we want
>> to do in master as a reason not to back-patch that fix, though.
>
> I guess as long as the pgunixsocket thing is in there, it makes sense
> to substitute DefaultHost for it on Windows, but are we sure that's
> something to back-patch?

Well, it seems like a clear case of returning a ridiculous value, but
I'm willing to be talked out of it if someone can explain how it would
break things. I guess it's possible someone could have code out that
that tests for the exact value /tmp and does something based on that,
but that seems a stretch - and if they did have such code, it would
probably just handle it by substituting localhost anyway.

> Right now, as I was saying, PQhost is in some gray area where it's not too
> clear what its charter is. It's not "what was the host parameter", for
> sure, but we haven't tried to make it an accurate description of the
> connection either. It's a bit less accurate on Windows than elsewhere,
> but do we want to risk breaking anything to only partially resolve that?

I guess it depends on how risky we think it is.

> More generally, if we do go over in 9.4 to the position that PQhost
> reports the host parameter and nothing but, I'm not sure that introducing
> a third behavior into the back branches is something that anybody will
> thank us for.

It doesn't seem very plausible to say that we're just going to
redefine it that way, unless we're planning to bump the soversion.
But maybe we should decide what we *are* going to do in master first,
before deciding what to back-patch.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Wed, Dec 11, 2013 at 2:29 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> More generally, if we do go over in 9.4 to the position that PQhost
>> reports the host parameter and nothing but, I'm not sure that introducing
>> a third behavior into the back branches is something that anybody will
>> thank us for.

> It doesn't seem very plausible to say that we're just going to
> redefine it that way, unless we're planning to bump the soversion.

Well, we didn't bump the soversion (nor touch the documentation)
in commit f6a756e4, which is basically what I'm suggesting we ought
to revert. It was nothing but a quick hack at the time, and hindsight
is saying it was a bad idea. Admittedly, it was long enough ago that
there might be some grandfather status attached to the current behavior;
but that argument can't be made for changing its behavior still further.

> But maybe we should decide what we *are* going to do in master first,
> before deciding what to back-patch.

Right.

regards, tom lane

On Thu, Dec 12, 2013 at 4:48 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Wed, Dec 11, 2013 at 2:29 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> More generally, if we do go over in 9.4 to the position that PQhost
>>> reports the host parameter and nothing but, I'm not sure that introducing
>>> a third behavior into the back branches is something that anybody will
>>> thank us for.
>
>> It doesn't seem very plausible to say that we're just going to
>> redefine it that way, unless we're planning to bump the soversion.
>
> Well, we didn't bump the soversion (nor touch the documentation)
> in commit f6a756e4, which is basically what I'm suggesting we ought
> to revert. It was nothing but a quick hack at the time, and hindsight
> is saying it was a bad idea. Admittedly, it was long enough ago that
> there might be some grandfather status attached to the current behavior;
> but that argument can't be made for changing its behavior still further.
>
>> But maybe we should decide what we *are* going to do in master first,
>> before deciding what to back-patch.
>
> Right.

I'm thinking to implement PQhostaddr() libpq function which returns the
host address of the connection. Also I'd like to fix the following two bugs
of PQhost(), which I reported upthread.

> (1) PQhost() can return Unix-domain socket directory path even in the
> platform that
> doesn't support Unix-domain socket.
>
> (2) In the platform that doesn't support Unix-domain socket, when
> neither host nor hostaddr
> are specified, the default host 'localhost' is used to connect to
> the server and
> PQhost() must return that, but it doesn't.

Then, we can change \conninfo so that it calls both PQhostaddr() and
PQhost(). If PQhostaddr() returns non-NULL, \conninfo should display
the IP address. Otherwise, \conninfo should display the return value of
PQhost().

Regards,

--
Fujii Masao