Re: fix psql \conninfo & \connect when using hostaddr - Mailing list pgsql-hackers

> On Tue, Jun 11, 2019 at 6:46 AM Fabien COELHO <coelho@cri.ensmp.fr> wrote:
>
> > Given that it's an open item for PostgreSQL 12,
>
> I'm working on it, but slowly.

Sorry, didn't mean to hurry you :) In fact if you need a hand, I can prepare a
patch for those parts everyone can agree on.

> > Indeed, looks like 6e5f8d4 introduced a subtle behaviour change, when
> > hostaddr changes are not picked up for subsequent \connect's, and I
> > don't see any mentions of it in the documentation.
>
> ISTM that the documentation is kind of fuzzy enough to match both
> behaviors.

Yeah, right. Then maybe we can add `hostaddr` e.g. somewhere here:

    diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
    --- a/doc/src/sgml/ref/psql-ref.sgml
    +++ b/doc/src/sgml/ref/psql-ref.sgml
    @@ -898,7 +898,7 @@ testdb=>
             </para>

             <para>
    -        Where the command omits database name, user, host, or port, the new
    +        Where the command omits database name, user, host,
hostaddr, or port, the new
             connection can reuse values from the previous connection.
By default,
             values from the previous connection are reused except
when processing

to emphasize the reusing of hostaddr too, and then mention behaviour change in
the release notes or any other place that would be the appropriate for that?

> > Although I guess it can be avoided by `-reuse-previous=off`, probably it
> > makese sense to update the docs.
>
> Yep, that is one option. The other is to revert or alter the subtle
> change, but ISTM that it made sense in some use case, so I wanted some
> time to think about it and test.

Sure, no one argue that the behaviour should be changed, it's only about the
documentation part.

> On Mon, May 27, 2019 at 10:38 PM Noah Misch <noah@leadboat.com> wrote:
> +static void
> +getHostaddr(PGconn *conn, char *host_addr, int host_addr_len)
> +{
> +     struct sockaddr_storage *addr = &conn->raddr.addr;
> +
> +     if (conn->connhost[conn->whichhost].type == CHT_HOST_ADDRESS)
> +             strlcpy(host_addr, conn->connhost[conn->whichhost].hostaddr, host_addr_len);
>
> I recommend removing this branch; inet_net_ntop() would work fine in the
> CHT_HOST_ADDRESS case, and that has the advantage of putting the address in
> standard form.

After short experiments I also couldn't find the reason why CHT_HOST_ADDRESS is
treated specially.

> +PQhostaddr(const PGconn *conn)
> +{
> +     if (!conn)
> +             return NULL;
> +
> +     if (conn->connhost != NULL)
> +     {
> +             if (conn->connhost[conn->whichhost].hostaddr != NULL &&
> +                     conn->connhost[conn->whichhost].hostaddr[0] != '\0')
> +                     return conn->connhost[conn->whichhost].hostaddr;
> +
> +             if (conn->connip != NULL)
> +                     return conn->connip;
> +     }
> +
> +     return "";
> +}
>
> Similar to my comment on getHostaddr(), why ever use hostaddr?  connip should
> always be equivalent to hostaddr when hostaddr is set.  (connip may be NULL if
> a malloc failed, but if we really cared, we'd fail the connection attempt when
> that happens.  If connip differs in any other material way, I'd want the user
> to see connip.)

The same here. Couldn't find any situation so far when `hostaddr` would be
different from `connip`. Maybe it makes sense just to reverse these conditions
and return first `connip` if not NULL. Also this example:

> Currently, hostaddr in nonstandard form stays that way
> (trailing space, in this example):
>
> [nm@gust 8:1 2019-05-23T13:21:54 postgresql 0]$ psql -X "hostaddr='127.0.7.1 '" -c '\conninfo'
> You are connected to database "test" as user "nm" on host "127.0.7.1 " at port "5433".

was a bit confusing for me, since the value here comes from PQhost, not
PQhostaddr, but in the very same way via hostaddr. So I guess any changes
should be replicated there too.