Thread: 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
Attachment
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. > > 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@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
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.
Attachment
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
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.
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
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.