The problems of PQhost()

Hi,

I reported in other thread that PQhost() has three problems.

http://www.postgresql.org/message-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.
http://www.postgresql.org/message-id/8913B51B3B534F878D54B89E1EB964C6@maumau

Regards,

--
Fujii Masao

On Wed, Jan 22, 2014 at 11:48 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
> Hi,
>
> I reported in other thread that PQhost() has three problems.
>
> http://www.postgresql.org/message-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.
> http://www.postgresql.org/message-id/8913B51B3B534F878D54B89E1EB964C6@maumau

Committed.

Regards,

--
Fujii Masao

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?

On Tue, Nov 25, 2014 at 12:37 PM, Noah Misch <noah(at)leadboat(dot)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

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(at)leadboat(dot)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.

On Thu, Nov 27, 2014 at 12:38 PM, Noah Misch <noah(at)leadboat(dot)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(at)leadboat(dot)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

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(at)leadboat(dot)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.

On Fri, Nov 28, 2014 at 3:43 AM, Noah Misch <noah(at)leadboat(dot)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(at)leadboat(dot)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

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(at)leadboat(dot)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(at)leadboat(dot)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.