-d option for pg_isready is broken

On Wed, Nov 13, 2013 at 9:37 PM, Josh Berkus <josh@agliodbs.com> wrote:

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

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

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

handyrep@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@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

On Fri, Nov 15, 2013 at 9:01 PM, Fabrízio de Royes Mello
<fabriziomello@gmail.com> wrote:

On Wed, Nov 13, 2013 at 9:37 PM, Josh Berkus <josh@agliodbs.com> wrote:

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

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

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

handyrep@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@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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Tue, Nov 19, 2013 at 11:51 PM, Robert Haas <robertmhaas@gmail.com> wrote:

On Fri, Nov 15, 2013 at 9:01 PM, Fabrízio de Royes Mello
<fabriziomello@gmail.com> wrote:

On Wed, Nov 13, 2013 at 9:37 PM, Josh Berkus <josh@agliodbs.com> wrote:

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

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

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

handyrep@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@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.fujii@gmail.com> wrote:

On Tue, Nov 19, 2013 at 11:51 PM, Robert Haas <robertmhaas@gmail.com> wrote:

On Fri, Nov 15, 2013 at 9:01 PM, Fabrízio de Royes Mello
<fabriziomello@gmail.com> wrote:

On Wed, Nov 13, 2013 at 9:37 PM, Josh Berkus <josh@agliodbs.com> wrote:

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

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

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

handyrep@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@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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 11/19/2013 10:12 AM, Robert Haas wrote:

On Tue, Nov 19, 2013 at 1:10 PM, Fujii Masao <masao.fujii@gmail.com> wrote:

On Tue, Nov 19, 2013 at 11:51 PM, Robert Haas <robertmhaas@gmail.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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Tue, Nov 19, 2013 at 1:22 PM, Josh Berkus <josh@agliodbs.com> wrote:

On 11/19/2013 10:12 AM, Robert Haas wrote:

On Tue, Nov 19, 2013 at 1:10 PM, Fujii Masao <masao.fujii@gmail.com> wrote:

On Tue, Nov 19, 2013 at 11:51 PM, Robert Haas <robertmhaas@gmail.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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Wed, Nov 20, 2013 at 3:12 AM, Robert Haas <robertmhaas@gmail.com> wrote:

On Tue, Nov 19, 2013 at 1:10 PM, Fujii Masao <masao.fujii@gmail.com> wrote:

On Tue, Nov 19, 2013 at 11:51 PM, Robert Haas <robertmhaas@gmail.com> wrote:

On Fri, Nov 15, 2013 at 9:01 PM, Fabrízio de Royes Mello
<fabriziomello@gmail.com> wrote:

On Wed, Nov 13, 2013 at 9:37 PM, Josh Berkus <josh@agliodbs.com> wrote:

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

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

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

handyrep@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@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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Wed, Nov 20, 2013 at 4:55 AM, Fujii Masao <masao.fujii@gmail.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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Thu, Nov 21, 2013 at 6:41 AM, Robert Haas <robertmhaas@gmail.com> wrote:

On Wed, Nov 20, 2013 at 4:55 AM, Fujii Masao <masao.fujii@gmail.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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Thu, Nov 21, 2013 at 9:58 PM, Fujii Masao <masao.fujii@gmail.com> wrote:

On Thu, Nov 21, 2013 at 6:41 AM, Robert Haas <robertmhaas@gmail.com> wrote:

On Wed, Nov 20, 2013 at 4:55 AM, Fujii Masao <masao.fujii@gmail.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.fujii@gmail.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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Wed, Dec 11, 2013 at 10:26 PM, Robert Haas <robertmhaas@gmail.com> wrote:

On Tue, Dec 10, 2013 at 11:40 PM, Fujii Masao <masao.fujii@gmail.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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Wed, Dec 11, 2013 at 8:45 AM, Fujii Masao <masao.fujii@gmail.com> wrote:

On Wed, Dec 11, 2013 at 10:26 PM, Robert Haas <robertmhaas@gmail.com> wrote:

On Tue, Dec 10, 2013 at 11:40 PM, Fujii Masao <masao.fujii@gmail.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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Wed, Dec 11, 2013 at 9:35 AM, Andres Freund <andres@2ndquadrant.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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Robert Haas <robertmhaas@gmail.com> writes:

On Wed, Dec 11, 2013 at 9:35 AM, Andres Freund <andres@2ndquadrant.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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Wed, Dec 11, 2013 at 11:24 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@gmail.com> writes:

On Wed, Dec 11, 2013 at 9:35 AM, Andres Freund <andres@2ndquadrant.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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Robert Haas <robertmhaas@gmail.com> writes:

On Wed, Dec 11, 2013 at 11:24 AM, Tom Lane <tgl@sss.pgh.pa.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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers