-d option for pg_isready is broken
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
--
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 13, 2013 at 9:37 PM, Josh Berkus <josh@agliodbs.com> wrote:
handyrep@john:~/handyrep$ pg_isready --version
pg_isready (PostgreSQL) 9.3.1handyrep@john:~/handyrep$ pg_isready -h john -p 5432 -U postgres -d
postgres -q
pg_isready: missing "=" after "postgres" in connection info stringhandyrep@john:~/handyrep$ pg_isready --host=john --port=5432
--user=postgres --dbname=postgres
pg_isready: missing "=" after "postgres" in connection info stringhandyrep@john:~/handyrep$ pg_isready -h john -p 5432 -U postgres
john:5432 - accepting connectionsso apparently the -d option:
a) doesn't work, and
b) doesn't do anythingI 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
Show quoted text
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
Attachments:
fix-pg-isready.patchtext/x-diff; charset=US-ASCII; name=fix-pg-isready.patchDownload
diff --git a/src/bin/scripts/pg_isready.c b/src/bin/scripts/pg_isready.c
index d27ccea..7568df5 100644
--- a/src/bin/scripts/pg_isready.c
+++ b/src/bin/scripts/pg_isready.c
@@ -130,7 +130,7 @@ main(int argc, char **argv)
/*
* Get the host and port so we can display them in our output
*/
- if (pgdbname)
+ if (pgdbname && strchr(pgdbname, '=') != NULL)
{
opts = PQconninfoParse(pgdbname, &errmsg);
if (opts == NULL)
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.1handyrep@john:~/handyrep$ pg_isready -h john -p 5432 -U postgres -d
postgres -q
pg_isready: missing "=" after "postgres" in connection info stringhandyrep@john:~/handyrep$ pg_isready --host=john --port=5432
--user=postgres --dbname=postgres
pg_isready: missing "=" after "postgres" in connection info stringhandyrep@john:~/handyrep$ pg_isready -h john -p 5432 -U postgres
john:5432 - accepting connectionsso apparently the -d option:
a) doesn't work, and
b) doesn't do anythingI 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 connectionsThe 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.1handyrep@john:~/handyrep$ pg_isready -h john -p 5432 -U postgres -d
postgres -q
pg_isready: missing "=" after "postgres" in connection info stringhandyrep@john:~/handyrep$ pg_isready --host=john --port=5432
--user=postgres --dbname=postgres
pg_isready: missing "=" after "postgres" in connection info stringhandyrep@john:~/handyrep$ pg_isready -h john -p 5432 -U postgres
john:5432 - accepting connectionsso apparently the -d option:
a) doesn't work, and
b) doesn't do anythingI 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 connectionsThe 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 attemptI 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 attemptOops.
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
Attachments:
fix_pg_isready_fujii.patchtext/x-diff; charset=US-ASCII; name=fix_pg_isready_fujii.patchDownload
*** a/src/bin/scripts/pg_isready.c
--- b/src/bin/scripts/pg_isready.c
***************
*** 31,36 **** main(int argc, char **argv)
--- 31,37 ----
const char *connect_timeout = DEFAULT_CONNECT_TIMEOUT;
const char *pghost_str = NULL;
+ const char *pghostaddr_str = NULL;
const char *pgport_str = NULL;
#define PARAMS_ARRAY_SIZE 7
***************
*** 130,136 **** main(int argc, char **argv)
/*
* Get the host and port so we can display them in our output
*/
! if (pgdbname)
{
opts = PQconninfoParse(pgdbname, &errmsg);
if (opts == NULL)
--- 131,140 ----
/*
* Get the host and port so we can display them in our output
*/
! if (pgdbname &&
! (strncmp(pgdbname, "postgresql://", 13) == 0 ||
! strncmp(pgdbname, "postgres://", 11) == 0 ||
! strchr(pgdbname, '=') != NULL))
{
opts = PQconninfoParse(pgdbname, &errmsg);
if (opts == NULL)
***************
*** 149,156 **** main(int argc, char **argv)
for (opt = opts, def = defs; def->keyword; def++)
{
! if (strcmp(def->keyword, "hostaddr") == 0 ||
! strcmp(def->keyword, "host") == 0)
{
if (opt && opt->val)
pghost_str = opt->val;
--- 153,159 ----
for (opt = opts, def = defs; def->keyword; def++)
{
! if (strcmp(def->keyword, "host") == 0)
{
if (opt && opt->val)
pghost_str = opt->val;
***************
*** 161,166 **** main(int argc, char **argv)
--- 164,176 ----
else
pghost_str = DEFAULT_PGSOCKET_DIR;
}
+ else if (strcmp(def->keyword, "hostaddr") == 0)
+ {
+ if (opt && opt->val)
+ pghostaddr_str = opt->val;
+ else if (def->val)
+ pghostaddr_str = def->val;
+ }
else if (strcmp(def->keyword, "port") == 0)
{
if (opt && opt->val)
***************
*** 179,185 **** main(int argc, char **argv)
if (!quiet)
{
! printf("%s:%s - ", pghost_str, pgport_str);
switch (rv)
{
--- 189,197 ----
if (!quiet)
{
! printf("%s:%s - ",
! pghostaddr_str != NULL ? pghostaddr_str : pghost_str,
! pgport_str);
switch (rv)
{
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.1handyrep@john:~/handyrep$ pg_isready -h john -p 5432 -U postgres -d
postgres -q
pg_isready: missing "=" after "postgres" in connection info stringhandyrep@john:~/handyrep$ pg_isready --host=john --port=5432
--user=postgres --dbname=postgres
pg_isready: missing "=" after "postgres" in connection info stringhandyrep@john:~/handyrep$ pg_isready -h john -p 5432 -U postgres
john:5432 - accepting connectionsso apparently the -d option:
a) doesn't work, and
b) doesn't do anythingI 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 connectionsThe 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 attemptI 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 attemptOops.
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.htmlAttached 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
Import Notes
Reply to msg id not found: WMad0e5ca03bebd48fb59153aecb6ab5664630c476c37ce6ce79145dfecfd253c4a9f083f5d0dd69634977038da825bf60@asav-1.01.com
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.htmlAttached 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.1handyrep@john:~/handyrep$ pg_isready -h john -p 5432 -U postgres -d
postgres -q
pg_isready: missing "=" after "postgres" in connection info stringhandyrep@john:~/handyrep$ pg_isready --host=john --port=5432
--user=postgres --dbname=postgres
pg_isready: missing "=" after "postgres" in connection info stringhandyrep@john:~/handyrep$ pg_isready -h john -p 5432 -U postgres
john:5432 - accepting connectionsso apparently the -d option:
a) doesn't work, and
b) doesn't do anythingI 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 connectionsThe 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 attemptI 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 attemptOops.
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
Import Notes
Reply to msg id not found: WM28d3656820a87cb3e7d347e9e6ce123b2c3d3751cbd3b0cbfc63eec7db539ae060359c77ebfc3cbb99140905c8b65311@asav-2.01.com
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
Attachments:
fix_pqhost_bugs_v1.patchtext/x-patch; charset=US-ASCII; name=fix_pqhost_bugs_v1.patchDownload
*** a/src/interfaces/libpq/fe-connect.c
--- b/src/interfaces/libpq/fe-connect.c
***************
*** 5188,5194 **** PQhost(const PGconn *conn)
{
if (!conn)
return NULL;
! return conn->pghost ? conn->pghost : conn->pgunixsocket;
}
char *
--- 5188,5205 ----
{
if (!conn)
return NULL;
! if (conn->pghostaddr != NULL && conn->pghostaddr[0] != '\0')
! return conn->pghostaddr;
! else if (conn->pghost != NULL && conn->pghost[0] != '\0')
! return conn->pghost;
! else
! {
! #ifdef HAVE_UNIX_SOCKETS
! return conn->pgunixsocket;
! #else
! return DefaultHost;
! #endif
! }
}
char *
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
On Wed, Dec 11, 2013 at 2:10 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
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.
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
--
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 2:10 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Robert Haas <robertmhaas@gmail.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
--
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 2:29 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Robert Haas <robertmhaas@gmail.com> writes:
On Wed, Dec 11, 2013 at 2:10 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Robert Haas <robertmhaas@gmail.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
--
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 2:29 PM, Tom Lane <tgl@sss.pgh.pa.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
--
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, Dec 12, 2013 at 4:48 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Robert Haas <robertmhaas@gmail.com> writes:
On Wed, Dec 11, 2013 at 2:29 PM, Tom Lane <tgl@sss.pgh.pa.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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers