The problems of PQhost()
Hi,
I reported in other thread that PQhost() has three problems.
/messages/by-id/CAHGQGwE77AKyabYwde5+8BjLdOpThp7cnswaO_syEdJOGYvnNw@mail.gmail.com
------------------------
(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".
------------------------
Attached patch fixes these problems.
We can fix the problem (3) by changing PQhost() so that it also
returns the hostaddr. But this change might break the existing
application using PQhost(). So, I added new libpq function PQhostaddr()
which returns the hostaddr, and changed \conninfo so that it reports
correct connection information by using both PQhost() and PQhostaddr().
These problems exist in v9.3 or before. Basically we should backport
the patch into those versions. But obviously it's too late to add new libpq
function PQhostaddr() into those versions. Then, I'm thinking to backport
only the change for (1) and (2) because we can fix them without adding
new libpq function.
BTW, I think that the patch also fixes the problem of \conninfo that
MauMau reported in other thread.
/messages/by-id/8913B51B3B534F878D54B89E1EB964C6@maumau
Regards,
--
Fujii Masao
Attachments:
fix_pqhost_bugs_v2.patchtext/x-patch; charset=US-ASCII; name=fix_pqhost_bugs_v2.patchDownload
*** a/doc/src/sgml/libpq.sgml
--- b/doc/src/sgml/libpq.sgml
***************
*** 1464,1469 **** char *PQhost(const PGconn *conn);
--- 1464,1487 ----
</listitem>
</varlistentry>
+ <varlistentry id="libpq-pqhostaddr">
+ <term>
+ <function>PQhostaddr</function>
+ <indexterm>
+ <primary>PQhostaddr</primary>
+ </indexterm>
+ </term>
+
+ <listitem>
+ <para>
+ Returns the server numeric IP address or host name of the connection.
+ <synopsis>
+ char *PQhostaddr(const PGconn *conn);
+ </synopsis>
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry id="libpq-pqport">
<term>
<function>PQport</function>
*** a/src/bin/psql/command.c
--- b/src/bin/psql/command.c
***************
*** 300,306 **** exec_command(const char *cmd,
else if (strcmp(cmd, "conninfo") == 0)
{
char *db = PQdb(pset.db);
! char *host = PQhost(pset.db);
if (db == NULL)
printf(_("You are currently not connected to a database.\n"));
--- 300,306 ----
else if (strcmp(cmd, "conninfo") == 0)
{
char *db = PQdb(pset.db);
! char *host = (PQhostaddr(pset.db) != NULL) ? PQhostaddr(pset.db) : PQhost(pset.db);
if (db == NULL)
printf(_("You are currently not connected to a database.\n"));
*** a/src/interfaces/libpq/exports.txt
--- b/src/interfaces/libpq/exports.txt
***************
*** 165,167 **** lo_lseek64 162
--- 165,168 ----
lo_tell64 163
lo_truncate64 164
PQconninfo 165
+ PQhostaddr 166
*** 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,5211 ----
{
if (!conn)
return NULL;
! if (conn->pghost != NULL && conn->pghost[0] != '\0')
! return conn->pghost;
! else
! {
! #ifdef HAVE_UNIX_SOCKETS
! return conn->pgunixsocket;
! #else
! return DefaultHost;
! #endif
! }
! }
!
! char *
! PQhostaddr(const PGconn *conn)
! {
! if (!conn)
! return NULL;
! return conn->pghostaddr;
}
char *
*** a/src/interfaces/libpq/libpq-fe.h
--- b/src/interfaces/libpq/libpq-fe.h
***************
*** 301,306 **** extern char *PQdb(const PGconn *conn);
--- 301,307 ----
extern char *PQuser(const PGconn *conn);
extern char *PQpass(const PGconn *conn);
extern char *PQhost(const PGconn *conn);
+ extern char *PQhostaddr(const PGconn *conn);
extern char *PQport(const PGconn *conn);
extern char *PQtty(const PGconn *conn);
extern char *PQoptions(const PGconn *conn);
On Wed, Jan 22, 2014 at 11:48 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
Hi,
I reported in other thread that PQhost() has three problems.
/messages/by-id/CAHGQGwE77AKyabYwde5+8BjLdOpThp7cnswaO_syEdJOGYvnNw@mail.gmail.com
------------------------
(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".
------------------------Attached patch fixes these problems.
We can fix the problem (3) by changing PQhost() so that it also
returns the hostaddr. But this change might break the existing
application using PQhost(). So, I added new libpq function PQhostaddr()
which returns the hostaddr, and changed \conninfo so that it reports
correct connection information by using both PQhost() and PQhostaddr().These problems exist in v9.3 or before. Basically we should backport
the patch into those versions. But obviously it's too late to add new libpq
function PQhostaddr() into those versions. Then, I'm thinking to backport
only the change for (1) and (2) because we can fix them without adding
new libpq function.BTW, I think that the patch also fixes the problem of \conninfo that
MauMau reported in other thread.
/messages/by-id/8913B51B3B534F878D54B89E1EB964C6@maumau
Committed.
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, Jan 22, 2014 at 11:48:26PM +0900, Fujii Masao wrote:
(3) PQhost() cannot return the hostaddr.
We can fix the problem (3) by changing PQhost() so that it also
returns the hostaddr. But this change might break the existing
application using PQhost(). So, I added new libpq function PQhostaddr()
which returns the hostaddr, and changed \conninfo so that it reports
correct connection information by using both PQhost() and PQhostaddr().
+ <varlistentry id="libpq-pqhostaddr"> + <term> + <function>PQhostaddr</function> + <indexterm> + <primary>PQhostaddr</primary> + </indexterm> + </term> + + <listitem> + <para> + Returns the server numeric IP address or host name of the connection. + <synopsis> + char *PQhostaddr(const PGconn *conn); + </synopsis> + </para> + </listitem> + </varlistentry>
From reading this documentation, I assumed this function would return a
non-empty value for every TCP connection. After all, every TCP connection has
a peer IP address. In fact, PQhostaddr() returns the raw value of the
"hostaddr" connection parameter, whether from a libpq function argument or
from the PGHOSTADDR environment variable. (If the parameter and environment
variable are absent, it returns NULL. Adding "hostaddr=" to the connection
string makes it return the empty string.) A caller wanting the specific raw
value of a parameter could already use PQconninfo(). I suspect this new
function will confuse more than help. What do you think of reverting it and
having \conninfo use PQconninfo() to discover any "hostaddr" value?
--
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 25, 2014 at 12:37 PM, Noah Misch <noah@leadboat.com> wrote:
On Wed, Jan 22, 2014 at 11:48:26PM +0900, Fujii Masao wrote:
(3) PQhost() cannot return the hostaddr.
We can fix the problem (3) by changing PQhost() so that it also
returns the hostaddr. But this change might break the existing
application using PQhost(). So, I added new libpq function PQhostaddr()
which returns the hostaddr, and changed \conninfo so that it reports
correct connection information by using both PQhost() and PQhostaddr().+ <varlistentry id="libpq-pqhostaddr"> + <term> + <function>PQhostaddr</function> + <indexterm> + <primary>PQhostaddr</primary> + </indexterm> + </term> + + <listitem> + <para> + Returns the server numeric IP address or host name of the connection. + <synopsis> + char *PQhostaddr(const PGconn *conn); + </synopsis> + </para> + </listitem> + </varlistentry>From reading this documentation, I assumed this function would return a
non-empty value for every TCP connection. After all, every TCP connection has
a peer IP address. In fact, PQhostaddr() returns the raw value of the
"hostaddr" connection parameter, whether from a libpq function argument or
from the PGHOSTADDR environment variable. (If the parameter and environment
variable are absent, it returns NULL. Adding "hostaddr=" to the connection
string makes it return the empty string.) A caller wanting the specific raw
value of a parameter could already use PQconninfo(). I suspect this new
function will confuse more than help. What do you think of reverting it and
having \conninfo use PQconninfo() to discover any "hostaddr" value?
Sounds reasonable to me. Are you planning to implement and commit the 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 Tue, Nov 25, 2014 at 09:53:10PM +0900, Fujii Masao wrote:
On Tue, Nov 25, 2014 at 12:37 PM, Noah Misch <noah@leadboat.com> wrote:
On Wed, Jan 22, 2014 at 11:48:26PM +0900, Fujii Masao wrote:
(3) PQhost() cannot return the hostaddr.
We can fix the problem (3) by changing PQhost() so that it also
returns the hostaddr. But this change might break the existing
application using PQhost(). So, I added new libpq function PQhostaddr()
which returns the hostaddr, and changed \conninfo so that it reports
correct connection information by using both PQhost() and PQhostaddr().+ <varlistentry id="libpq-pqhostaddr"> + <term> + <function>PQhostaddr</function> + <indexterm> + <primary>PQhostaddr</primary> + </indexterm> + </term> + + <listitem> + <para> + Returns the server numeric IP address or host name of the connection. + <synopsis> + char *PQhostaddr(const PGconn *conn); + </synopsis> + </para> + </listitem> + </varlistentry>From reading this documentation, I assumed this function would return a
non-empty value for every TCP connection. After all, every TCP connection has
a peer IP address. In fact, PQhostaddr() returns the raw value of the
"hostaddr" connection parameter, whether from a libpq function argument or
from the PGHOSTADDR environment variable. (If the parameter and environment
variable are absent, it returns NULL. Adding "hostaddr=" to the connection
string makes it return the empty string.) A caller wanting the specific raw
value of a parameter could already use PQconninfo(). I suspect this new
function will confuse more than help. What do you think of reverting it and
having \conninfo use PQconninfo() to discover any "hostaddr" value?Sounds reasonable to me. Are you planning to implement and commit the patch?
Sure. I'll first issue "git revert 9f80f48", then apply the attached patch.
Since libpq ignores a hostaddr parameter equal to the empty string, this
implementation does likewise. Apart from that, I anticipate behavior
identical to today's code.
Attachments:
conninfo-hostaddr-v1.patchtext/plain; charset=us-asciiDownload
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index b518fb1..0c60746 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -302,13 +302,32 @@ exec_command(const char *cmd,
else if (strcmp(cmd, "conninfo") == 0)
{
char *db = PQdb(pset.db);
- char *host = PQhost(pset.db);
if (db == NULL)
printf(_("You are currently not connected to a database.\n"));
else
{
- if (host == NULL)
+ char *host;
+ PQconninfoOption *connOptions;
+ PQconninfoOption *option;
+
+ /* Report "hostaddr" if libpq used it, otherwise PQhost(). */
+ host = PQhost(pset.db);
+ connOptions = PQconninfo(pset.db);
+ if (connOptions == NULL)
+ {
+ fprintf(stderr, _("out of memory\n"));
+ exit(EXIT_FAILURE);
+ }
+ for (option = connOptions; option && option->keyword; option++)
+ if (strcmp(option->keyword, "hostaddr") == 0)
+ {
+ if (option->val != NULL && option->val[0] != '\0')
+ host = option->val;
+ break;
+ }
+
+ if (host == NULL) /* can't happen */
host = DEFAULT_PGSOCKET_DIR;
/* If the host is an absolute path, the connection is via socket */
if (is_absolute_path(host))
@@ -318,6 +337,8 @@ exec_command(const char *cmd,
printf(_("You are connected to database \"%s\" as user \"%s\" on host \"%s\" at port \"%s\".\n"),
db, PQuser(pset.db), host, PQport(pset.db));
printSSLInfo();
+
+ PQconninfoFree(connOptions);
}
}
On Thu, Nov 27, 2014 at 12:38 PM, Noah Misch <noah@leadboat.com> wrote:
On Tue, Nov 25, 2014 at 09:53:10PM +0900, Fujii Masao wrote:
On Tue, Nov 25, 2014 at 12:37 PM, Noah Misch <noah@leadboat.com> wrote:
On Wed, Jan 22, 2014 at 11:48:26PM +0900, Fujii Masao wrote:
(3) PQhost() cannot return the hostaddr.
We can fix the problem (3) by changing PQhost() so that it also
returns the hostaddr. But this change might break the existing
application using PQhost(). So, I added new libpq function PQhostaddr()
which returns the hostaddr, and changed \conninfo so that it reports
correct connection information by using both PQhost() and PQhostaddr().+ <varlistentry id="libpq-pqhostaddr"> + <term> + <function>PQhostaddr</function> + <indexterm> + <primary>PQhostaddr</primary> + </indexterm> + </term> + + <listitem> + <para> + Returns the server numeric IP address or host name of the connection. + <synopsis> + char *PQhostaddr(const PGconn *conn); + </synopsis> + </para> + </listitem> + </varlistentry>From reading this documentation, I assumed this function would return a
non-empty value for every TCP connection. After all, every TCP connection has
a peer IP address. In fact, PQhostaddr() returns the raw value of the
"hostaddr" connection parameter, whether from a libpq function argument or
from the PGHOSTADDR environment variable. (If the parameter and environment
variable are absent, it returns NULL. Adding "hostaddr=" to the connection
string makes it return the empty string.) A caller wanting the specific raw
value of a parameter could already use PQconninfo(). I suspect this new
function will confuse more than help. What do you think of reverting it and
having \conninfo use PQconninfo() to discover any "hostaddr" value?Sounds reasonable to me. Are you planning to implement and commit the patch?
Sure. I'll first issue "git revert 9f80f48", then apply the attached patch.
Since libpq ignores a hostaddr parameter equal to the empty string, this
implementation does likewise. Apart from that, I anticipate behavior
identical to today's code.
+ fprintf(stderr, _("out of memory\n"));
psql_error() should be used instead of fprintf()?
+ if (host == NULL) /* can't happen */
Is this comment true? ISTM that "host" can be NULL when the default socket
directory is used, i.e., neither host nor hostaddr are supplied by the user.
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 Fri, Nov 28, 2014 at 03:11:06AM +0900, Fujii Masao wrote:
On Thu, Nov 27, 2014 at 12:38 PM, Noah Misch <noah@leadboat.com> wrote:
Sure. I'll first issue "git revert 9f80f48", then apply the attached patch.
Since libpq ignores a hostaddr parameter equal to the empty string, this
implementation does likewise. Apart from that, I anticipate behavior
identical to today's code.+ fprintf(stderr, _("out of memory\n"));
psql_error() should be used instead of fprintf()?
I copied what pg_malloc() would do. Either way seems reasonable.
+ if (host == NULL) /* can't happen */
Is this comment true? ISTM that "host" can be NULL when the default socket
directory is used, i.e., neither host nor hostaddr are supplied by the user.
Right; I will adjust accordingly. Thanks.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Fri, Nov 28, 2014 at 3:43 AM, Noah Misch <noah@leadboat.com> wrote:
On Fri, Nov 28, 2014 at 03:11:06AM +0900, Fujii Masao wrote:
On Thu, Nov 27, 2014 at 12:38 PM, Noah Misch <noah@leadboat.com> wrote:
Sure. I'll first issue "git revert 9f80f48", then apply the attached patch.
Since libpq ignores a hostaddr parameter equal to the empty string, this
implementation does likewise. Apart from that, I anticipate behavior
identical to today's code.+ fprintf(stderr, _("out of memory\n"));
psql_error() should be used instead of fprintf()?
I copied what pg_malloc() would do. Either way seems reasonable.
psql_error() seems better for the case where psql executes the
specified input file.
In this case, psql_error reports the message in the format like
"psql:filename:lineno: message".
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 Fri, Nov 28, 2014 at 07:55:29PM +0900, Fujii Masao wrote:
On Fri, Nov 28, 2014 at 3:43 AM, Noah Misch <noah@leadboat.com> wrote:
On Fri, Nov 28, 2014 at 03:11:06AM +0900, Fujii Masao wrote:
On Thu, Nov 27, 2014 at 12:38 PM, Noah Misch <noah@leadboat.com> wrote:
Sure. I'll first issue "git revert 9f80f48", then apply the attached patch.
Since libpq ignores a hostaddr parameter equal to the empty string, this
implementation does likewise. Apart from that, I anticipate behavior
identical to today's code.+ fprintf(stderr, _("out of memory\n"));
psql_error() should be used instead of fprintf()?
I copied what pg_malloc() would do. Either way seems reasonable.
psql_error() seems better for the case where psql executes the
specified input file.
In this case, psql_error reports the message in the format like
"psql:filename:lineno: message".
Fair enough. Will do it that way.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers