Re: PQHost() undefined behavior if connecting string contains both host and hostaddr types

On Mon, Mar 26, 2018 at 11:28:41AM +0900, Kyotaro HORIGUCHI wrote:
> At Sun, 25 Mar 2018 22:27:09 +1100, Haribabu Kommi <kommi(dot)haribabu(at)gmail(dot)com> wrote in <CAJrrPGeaQxWsU3RZ0yxGa=WY+wnA+nvE_YPuKzBDaqRMUt9SyA(at)mail(dot)gmail(dot)com>
>> On Sun, Mar 25, 2018 at 12:56 AM, Michael Paquier <michael(at)paquier(dot)xyz>
>> wrote:
> As the commit message of 50cb21f70, the function is intended not
> to return NULL in order to prevent the user functions from a
> crash in corner cases.

The commit number is not correct here. You mean 40cb21f.

> https://www.postgresql.org/message-id/19297.1493660213%40sss.pgh.pa.us

I quite like the idea of using an empty string value in those cases.
This could prevent crashes at leat for applications not doing NULL-ness
checks.

>> The existing behavior is currently returning wrong value when the connection
>> status is CONNECTION_BAD. As we are changing the behavior of the function,
>> it may be better to handle the CONNECTION_BAD scenario also instead of
>> providing note in the manual?
>
> There's no reason to behave so only for the functions. PQdb() and
> PQuser() returns names that is not actually connected.

For PQdb() and PQuser(), the interface is basicaly intuitive, because you
cannot specify multiple entries. So if a client specifies a value, it
will be sure that the value returned is unique.

> Since the
> purpose of PGhost is not strictly defined, especially on
> connection failure, returning the given host list can be another
> candidate (and the same can be said for returning ""). I think
> users shouldn't (and I believe no one does) rely on the values
> from the functions when CONNECTION_BAD, anyway.

Yeah, this should really be documented and also should refer to the fact
that this happens when specifying multiple hosts.

> 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.

>> 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.

+ 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.
--
Michael