Re: PQHost() undefined behavior if connecting string contains bothhost and hostaddr types - Mailing list pgsql-hackers

Hello.

At Mon, 26 Mar 2018 17:49:22 +1100, Haribabu Kommi <kommi.haribabu@gmail.com> wrote in <CAJrrPGdYQ92R1hNArCByu+gN_SGsFMjGvbErjH0N9W8Ry24CxA@mail.gmail.com>

+       The <function>PQhost</function> function returns NULL when the
+       connection is not established, or returns an empty string when status
+       of the connection is not <literal>CONNECTION_OK</literal>.

This may be wrong. NULL is only for the case conn == NULL and ""
for other connection failure. I'm not sure how to express the OOM
case in the documentation but we could reuturn "" for the
conn==NULL case if we don't want to distinguish the state in
PQhost and its family.

> > > My opinion is to add a description that is saying like "these
> > > functions return unreliable values for a failed connection".
> >
> > At the same time I don't think that this is sufficient either, because
> > for multiple hosts, PQhost() just returns the first one, which makes
> > absolutely no sense because the value is wrong.  So I think that using a
> > third, separate value has some advantages:
> > - If NULL, this just means that the initialization did not happen.
> > - If using an empty string, then the value cannot be evaluated.
> > - If this returns a host or hostaddr (if host has not been specified),
> > then that's the host which is actually used for the connection.
> > Having those three states has value for applications in my opinion.
> >
> > The same can apply to PQport, or any other functions which for whatever
> > reason add support for multiple values like host, hostaddr or port.
> >
>
> I hope that I updated the documentation properly to explain all the above
> cases.

I'm not sure but I'm afraid that some authentication methods
requires that PQhost() returns a host name for the states other
than CONNECTION_OK, perhaps CONNECTION_MADE, AWAITING_RESPONSE
and so, which happens after a connection is established. Even
without considering that, we can return a sane value after raw
connection (not a PGconn) is established.

> > >> Updated patch attached with behavior of returning NULL for connections
> > of
> > >> CONNECTION_BAD status.
> > >
> > > The patch does Assert() in PQhost. I suppose that Assert() in
> > > client library is usable only when no more (library's) operation
> > > cannot continue. It would be better to return a fallback value in
> > > this criteria.
> >
> > The patch has to return a value as well.  A shaped the patch causes
> > again compilation warnings because not all code paths have a return
> > value.  So my previous remark has not been fixed.  Hari, what do you use
> > as compiler, my gcc blows a warning and reading the patch that's
> > obviously incorrect.
> >
>
> In my assert enabled build, I didn't get any warning. Yes that patch to fix
> the
> warning is clearly wrong. I corrected in a different way of adding Assert
> checks
> for the hostaddr, because definitely host or hostaddr must be present.

As I wrote upthread, the assertion doesn't seem to be needed. I
think that a library should allow callers to decide how to handle
error cases if it is possible.

> > + PQHost returns NULL when the connection is not established
or
> > In the docs, this is wrong for two reasons:
> > - There should be a <function> markup.
> > - The name of the function is PQhost, not PGHost.
> >
>
> Corrected.
>
> Updated patch attached.

The documentation is written as the following.

-       Returns the server host name of the connection.
+       Returns the server host name or host address of the active connection.
        This can be a host name, an IP address, or a directory path if the

Is the replacement is required? The following line is stating the
same thing including the local-socket case.