Обсуждение: fix psql \conninfo & \connect when using hostaddr

Hello,

This is a follow-up to another patch I posted about libpq confusing 
documentation & psql resulting behavior under host/hostaddr settings.

Although the first mostly documentation patch did not gather much 
enthousiasm, I still think both issues deserve a fix.

About updating psql's behavior, without this patch:

   sh> psql "host=foo hostaddr=127.0.0.1"

   psql> \conninfo
   You are connected to database "fabien" as user "fabien" on host "foo" at port "5432".
    # NOPE, I'm really connected to localhost, foo does not even exist
    # Other apparent inconsistencies are possible when hostaddr overrides
    # "host" which is an socket directory or an IP.

   psql> \c template1
   could not translate host name "foo" to address: Name or service not known
   Previous connection kept
    # hmmm.... what is the meaning of reusing a connection?
    # this issue was pointed out by Arthur Zakirov

After the patch:

   sh> psql "host=foo hostaddr=127.0.0.1"

   psql> \conninfo
   You are connected to database "fabien" as user "fabien" on host "foo" (address "127.0.0.1") at port "5432".
   # better

   psql> \c template1
   You are now connected to database "template1" as user "fabien".
   # thanks

The patch adds a PQhostaddr() function to libpq which reports the 
"hostaddr" setting or the current server ip. The function is used by psql 
for \conninfo and when reusing parameters for \connect.

The messages are slightly more verbose because the IP is output. I think 
that user asking for conninfo should survive to the more precise data. 
This also comes handy if a host name resolves to several IPs (eg IPv6 and 
IPv4, or several IPs...).

-- 
Fabien.

Hello all,

On 07.11.2018 16:23, Pavel Stehule wrote:
> pá 26. 10. 2018 v 15:55 odesílatel Fabien COELHO <coelho@cri.ensmp.fr 
> <mailto:coelho@cri.ensmp.fr>> napsal:
> > 
> >     This is a follow-up to another patch I posted about libpq confusing
> >     documentation & psql resulting behavior under host/hostaddr settings.
> > 
> >     Although the first mostly documentation patch did not gather much
> >     enthousiasm, I still think both issues deserve a fix.
> 
> I checked this patch, and it looks well. The documentation is correct, 
> all tests passed. It does what is proposed.
> 
> I think so some redundant messages can be reduced - see function 
> printConnInfo - attached patch

I have few notes about patches.

> /* If the host is an absolute path, the connection is via socket unless overriden by hostaddr */
> if (is_absolute_path(host))
>     if (hostaddr && *hostaddr)
>         printf(_("You are connected to database \"%s\" as user \"%s\" on address \"%s\" at port \"%s\".\n"),
>                db, PQuser(pset.db), hostaddr, PQport(pset.db));
>     else
>         printf(_("You are connected to database \"%s\" as user \"%s\" via socket in \"%s\" at port \"%s\".\n"),
>                db, PQuser(pset.db), host, PQport(pset.db));
> else
>     if (hostaddr && *hostaddr && strcmp(host, hostaddr) != 0)
>         printf(_("You are connected to database \"%s\" as user \"%s\" on host \"%s\" (address \"%s\") at port
\"%s\".\n"),
>                db, PQuser(pset.db), host, hostaddr, PQport(pset.db));
>     else
>         printf(_("You are connected to database \"%s\" as user \"%s\" on host \"%s\" at port \"%s\".\n"),
>                db, PQuser(pset.db), host, PQport(pset.db));

I think there is lack of necessary braces here for first if and second 
else branches. This is true for both patches.

> /* set connip */
> if (conn->connip != NULL)
> {
>     free(conn->connip);
>     conn->connip = NULL;
> }
> 
> {
>     char        host_addr[NI_MAXHOST];
>     getHostaddr(conn, host_addr);
>     if (strcmp(host_addr, "???") != 0)
>         conn->connip = strdup(host_addr);
> }

Maybe it is better to move this code into the PQhostaddr() function? 
Since connip is used only in PQhostaddr() it might be not worth to do 
additional work in main PQconnectPoll().

-- 
Arthur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company

On Fri, Oct 26, 2018 at 9:54 AM Fabien COELHO <coelho@cri.ensmp.fr> wrote:
> About updating psql's behavior, without this patch:
>
>    sh> psql "host=foo hostaddr=127.0.0.1"
>
>    psql> \conninfo
>    You are connected to database "fabien" as user "fabien" on host "foo" at port "5432".
>     # NOPE, I'm really connected to localhost, foo does not even exist
>     # Other apparent inconsistencies are possible when hostaddr overrides
>     # "host" which is an socket directory or an IP.

I remain of the opinion that this is not a bug.  You told it that foo
has address 127.0.0.1 and it believed you; that's YOUR fault.

> After the patch:
>
>    sh> psql "host=foo hostaddr=127.0.0.1"
>
>    psql> \conninfo
>    You are connected to database "fabien" as user "fabien" on host "foo" (address "127.0.0.1") at port "5432".
>    # better

Nevertheless, that seems like a reasonable change to the output.  Will
your patch show the IP address in all cases or only when hostaddr is
specified?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 07.11.2018 20:11, Pavel Stehule wrote:
> st 7. 11. 2018 v 15:11 odesílatel Arthur Zakirov 
> <a.zakirov@postgrespro.ru <mailto:a.zakirov@postgrespro.ru>> napsal:
> >     I think there is lack of necessary braces here for first if and second
> >     else branches. This is true for both patches.
> ?

I just meant something like this (additional "{", "}" braces):

if (is_absolute_path(host))
{
     ...
}
else
{
     ...
}

-- 
Arthur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company

Hello Robert,

>>  psql> \conninfo
>>  You are connected to database "fabien" as user "fabien" on host "foo" at port "5432".
>
> I remain of the opinion that this is not a bug.  You told it that foo 
> has address 127.0.0.1 and it believed you; that's YOUR fault.

Hmmm. For me, if a user asks \conninfo for connection information, they 
expect to be told what the connection actually is, regardless of the 
initial connection string.

Another more stricking instance:

   sh> psql "host=/tmp port=5432 hostaddr=127.0.0.1"
   ...
   fabien=# \conninfo
   You are connected to database "fabien" as user "fabien" via socket in "/tmp" at port "5432".
   SSL connection (protocol: TLSv1.2, cipher: ECDHE-RSA-AES256-GCM-SHA384, bits: 256, compression: off)

It says that there is a socket, but there is none. The SSL bit is a 
giveaway, there is no SSL on Unix-domain sockets.

>>  sh> psql "host=foo hostaddr=127.0.0.1"
>>
>>  psql> \conninfo
>>  You are connected to database "fabien" as user "fabien" on host "foo" (address "127.0.0.1") at port "5432".
>
> Nevertheless, that seems like a reasonable change to the output.  Will 
> your patch show the IP address in all cases or only when hostaddr is 
> specified?

It is always printed, unless both host & address are equal.

The rational is that it is also potentially useful for multi-ip dns 
resolutions, and generating a valid hostaddr allows \connect defaults to 
reuse the actual same connection, including the IP that was chosen.

Also, the added information is quite short, and if a user explicitely asks 
for connection information, I think they can handle the slightly expanded 
answer.

-- 
Fabien.

On 2018-Nov-08, Arthur Zakirov wrote:

> On 07.11.2018 20:11, Pavel Stehule wrote:
> > st 7. 11. 2018 v 15:11 odesílatel Arthur Zakirov
> > <a.zakirov@postgrespro.ru <mailto:a.zakirov@postgrespro.ru>> napsal:
> > >     I think there is lack of necessary braces here for first if and second
> > >     else branches. This is true for both patches.
> > ?
> 
> I just meant something like this (additional "{", "}" braces):

We omit braces when there's an individual statement.  (We do add the
braces when we have a comment atop the individual statement, though, to
avoid pgindent from doing a stupid thing.)

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> On 2018-Nov-08, Arthur Zakirov wrote:
>> I just meant something like this (additional "{", "}" braces):

> We omit braces when there's an individual statement.  (We do add the
> braces when we have a comment atop the individual statement, though, to
> avoid pgindent from doing a stupid thing.)

For the record --- I just checked, and pgindent will not mess up code like

    if (condition)
        /* comment here */
        do_something();

at least not as long as the comment is short enough for one line.
(If it's a multiline comment, it seems to want to put a blank line
in front of it, which is not very nice in this context.)

Visually, however, I think this is better off with braces because
it *looks* like a multi-line if-block.  The braces also make it
clear that your intent was not, say,

    while (some-mutable-condition)
        /* skip */ ;
    do_something_else();

            regards, tom lane

On 2018-Nov-08, Tom Lane wrote:

> For the record --- I just checked, and pgindent will not mess up code like
> 
>     if (condition)
>         /* comment here */
>         do_something();
> 
> at least not as long as the comment is short enough for one line.
> (If it's a multiline comment, it seems to want to put a blank line
> in front of it, which is not very nice in this context.)

Yeah, those blank lines are what I've noticed, and IMO they look pretty
bad.

> Visually, however, I think this is better off with braces because
> it *looks* like a multi-line if-block.  The braces also make it
> clear that your intent was not, say,
> 
>     while (some-mutable-condition)
>         /* skip */ ;
>     do_something_else();

Right, that too.  Fortunately I think compilers warn about mismatching
indentation nowadays, at least in some cases.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Thu, Nov 08, 2018 at 12:13:31PM -0300, Alvaro Herrera wrote:
> On 2018-Nov-08, Tom Lane wrote:
>> Visually, however, I think this is better off with braces because
>> it *looks* like a multi-line if-block.  The braces also make it
>> clear that your intent was not, say,
>>
>>     while (some-mutable-condition)
>>         /* skip */ ;
>>     do_something_else();
>
> Right, that too.  Fortunately I think compilers warn about mismatching
> indentation nowadays, at least in some cases.

I don't recall seeing a compiler warning about that, but I do recall
coverity complaining loudly about such things.  It is better style to
use braces if there is one line of code with a comment block in my
opinion.  And there is no need for braces if there is no comment.
--
Michael

Looks good to me, save that I would change the API of getHostaddr() to
this:

/* ----------
 * getHostaddr -
 * Fills 'host_addr' with the string representation of the currently connected
 * socket in 'conn'.
 * ----------
 */
static void
getHostaddr(PGconn *conn, char *host_addr, int host_addr_len)

(Trying to pass arrays as such to C functions is a lost cause -- just a
documentation aid at best, and a source of confusion and crashes at
worst.)

I tried to see about overflowing the NI_MAXHOST length with a long
socket dir, but that doesn't actually work, though the first line of
error here is a bit surprising:

LC_ALL=C psql "host="\'"`pwd`"\'""
could not identify current directory: Numerical result out of range
psql: Unix-domain socket path "/tmp/En un lugar de la Mancha_ de cuyo nombre no quiero acordarme_ no ha mucho tiempo
quevivía un hidalgo de los de lanza en astillero_ adarga antigua_ rocín flaco y galgo corredor./Una olla de algo más
vacaque carnero_ salpicón las más noches_ duelos y quebrantos los sábados_ lentejas los viernes_ algún palomino de
añadiduralos domingos_ consumían las tres partes de su hacienda./El resto della concluían sayo de velarte_ calzas de
velludopara las fiestas con sus pantuflos de lo mismo_ los días de entre semana se honraba con su vellori de lo más
fino./Teníaen su casa una ama que pasaba de los cuarenta_ y una sobrina que no llegaba a los veinte_ y un mozo de campo
yplaza_ que así ensillaba el rocín como tomaba la podadera./Frisaba la edad de nuestro hidalgo con los cincuenta años_
erade complexión recia_ seco de carnes_ enjuto de rostro; gran madrugador y amigo de la caza./Quieren decir que tenía
elsobrenombre de Quijada o Quesada (que en esto hay alguna diferencia en los autores que " is too long (maximum 107
bytes)


This is after I replaced all the , to _, because the original was even
more fun:

LC_ALL=C psql "host="\'"`pwd`"\'""
could not identify current directory: Numerical result out of range
psql: could not connect to server: No such file or directory
    Is the server running locally and accepting
    connections on Unix domain socket "/tmp/En un lugar de la Mancha/.s.PGSQL.55432"?
could not translate host name " de cuyo nombre no quiero acordarme" to address: Name or service not known
could not translate host name " no ha mucho tiempo que vivía un hidalgo de los de lanza en astillero" to address: Name
orservice not known 
could not translate host name " adarga antigua" to address: Name or service not known
could not translate host name " rocín flaco y galgo corredor./Una olla de algo más vaca que carnero" to address: Name
orservice not known 
could not translate host name " salpicón las más noches" to address: Name or service not known
could not translate host name " duelos y quebrantos los sábados" to address: Name or service not known
could not translate host name " lentejas los viernes" to address: Name or service not known
could not translate host name " algún palomino de añadidura los domingos" to address: Name or service not known
could not translate host name " consumían las tres partes de su hacienda./El resto della concluían sayo de velarte" to
address:Name or service not known 
could not translate host name " calzas de velludo para las fiestas con sus pantuflos de lo mismo" to address: Name or
servicenot known 
could not translate host name " los días de entre semana se honraba con su vellori de lo más fino./Tenía en su casa una
amaque pasaba de los cuarenta" to address: Name or service not known 
could not translate host name " y una sobrina que no llegaba a los veinte" to address: Name or service not known
could not translate host name " y un mozo de campo y plaza" to address: Name or service not known
could not translate host name " que así ensillaba el rocín como tomaba la podadera./Frisaba la edad de nuestro hidalgo
conlos cincuenta años" to address: Name or service not known 
could not translate host name " era de complexión recia" to address: Name or service not known
could not translate host name " seco de carnes" to address: Name or service not known
could not translate host name " enjuto de rostro; gran madrugador y amigo de la caza./Quieren decir que tenía el
sobrenombrede Quijada o Quesada (que en esto hay alguna diferencia en los autores que deste caso escriben)" to address:
Nameor service not known 
could not translate host name " aunque por conjeturas verosímiles se deja entender que se llama Quijana;/pero esto
importapoco a nuestro cuento; basta que en la narración dél no se salga un punto de la verdad." to address: Name or
servicenot known 


Funky test cases aside, I verified that giving hostaddr behaves better
with the patch.  This is unpatched:

$ LC_ALL=C psql "host="\'"`pwd`"\'" hostaddr=::1"
psql (12devel, server 11.1)
Type "help" for help.

55442 12devel 879890=# \conninfo
You are connected to database "alvherre" as user "alvherre" via socket in "/tmp/En un lugar de la Mancha_ de cuyo
nombreno quiero acordarme_ no ha mucho tiempo" at port "55442". 

and this is patched:

$ LC_ALL=C psql "host="\'"`pwd`"\'" hostaddr=::1"
psql (12devel, server 11.1)
Type "help" for help.

55442 12devel 881754=# \conninfo
You are connected to database "alvherre" as user "alvherre" on address "::1" at port "55442".

--
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 2018-Nov-17, Alvaro Herrera wrote:

> Looks good to me, save that I would change the API of getHostaddr() to
> this:
> 
> /* ----------
>  * getHostaddr -
>  * Fills 'host_addr' with the string representation of the currently connected
>  * socket in 'conn'.
>  * ----------
>  */
> static void
> getHostaddr(PGconn *conn, char *host_addr, int host_addr_len)

Fabien, are you planning to fix things per Arthur's review?

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Sat, 17 Nov 2018, Alvaro Herrera wrote:

>> /* ----------
>>  * getHostaddr -
>>  * Fills 'host_addr' with the string representation of the currently connected
>>  * socket in 'conn'.
>>  * ----------
>>  */
>> static void
>> getHostaddr(PGconn *conn, char *host_addr, int host_addr_len)

> Fabien, are you planning to fix things per Arthur's review?

Yep, I am.

I will not do the above though, because the PQgetHostaddr API would differ 
from all other connection status functions (PQgetHost, PQgetUser...) which 
are all "char * PQget<Something>(PGconn * conn)"

-- 
Fabien.

>> Fabien, are you planning to fix things per Arthur's review?
>
> Yep, I am.
>
> I will not do the above though, because the PQgetHostaddr API would differ 
> from all other connection status functions (PQgetHost, PQgetUser...) which 
> are all "char * PQget<Something>(PGconn * conn)"

Sorry, I'm mixing everything, the function is an internal one.

I'm working on improving the patch.

-- 
Fabien.

On 2018-Nov-17, Fabien COELHO wrote:

> 
> > > Fabien, are you planning to fix things per Arthur's review?
> > 
> > Yep, I am.
> > 
> > I will not do the above though, because the PQgetHostaddr API would
> > differ from all other connection status functions (PQgetHost,
> > PQgetUser...) which are all "char * PQget<Something>(PGconn * conn)"
> 
> Sorry, I'm mixing everything, the function is an internal one.

Yeah :-)

> I'm working on improving the patch.

Cool.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Hello Pavel,

> I think so some redundant messages can be reduced - see function 
> printConnInfo - attached patch

I thought about doing like that, but I made the debatable choice to keep 
the existing redundancy because it minimizes diffs and having a 
print-to-stdout special function does not look like a very clean API, as 
it cannot really be used by non CLI clients.

-- 
Fabien.

On 2018-Nov-17, Fabien COELHO wrote:

> > I think so some redundant messages can be reduced - see function
> > printConnInfo - attached patch
> 
> I thought about doing like that, but I made the debatable choice to keep the
> existing redundancy because it minimizes diffs and having a print-to-stdout
> special function does not look like a very clean API, as it cannot really be
> used by non CLI clients.

What?  This is psql, so it doesn't affect non-CLI clientes, does it?

On the other hand, one message says "you're NOW connected", the other
doesn't have the "now".  If we're dropping the "now" (I think it's
useless), let's make an explicit choice about it.  TBH I'd drop the
"you're" also, so both \conninfo and \c would say

Connected to database foo <conn details>

Anyway, a trivial change that's sure to make bikeshed paint seller cry
with so many customers yelling at each other; not for this patch.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

>>> I think so some redundant messages can be reduced - see function
>>> printConnInfo - attached patch
>>
>> I thought about doing like that, but I made the debatable choice to keep the
>> existing redundancy because it minimizes diffs and having a print-to-stdout
>> special function does not look like a very clean API, as it cannot really be
>> used by non CLI clients.
>
> What?  This is psql, so it doesn't affect non-CLI clientes, does it?

Indeed, you are right, and I'm really mixing everything today. What I 
really thought was to have a function which would return the full 
description.

> On the other hand, one message says "you're NOW connected",

Indeed, the text is slightly different.

> the other doesn't have the "now".  If we're dropping the "now" (I think 
> it's useless), let's make an explicit choice about it.  TBH I'd drop the 
> "you're" also, so both \conninfo and \c would say
>
> Connected to database foo <conn details>
>
> Anyway, a trivial change that's sure to make bikeshed paint seller cry
> with so many customers yelling at each other; not for this patch.

Ok. I'm not planning to refactor "psql" today.

-- 
Fabien.

>> I'm working on improving the patch.
>
> Cool.

Here is the updated v2

  - libpq internal function getHostaddr get a length,
    and I added an assert about it.
  - added a few braces on if/if/else/if/else/else
  - added an UNKNOWN_HOST macro to hide "???"
  - moved host_addr[] declaration earlier to avoid some braces
  - I have not refactored psql connection message,
    but finally agree with Pavel & you have a point.

-- 
Fabien.

On 2018-Nov-17, Fabien COELHO wrote:

> Here is the updated v2
> 
>  - libpq internal function getHostaddr get a length,
>    and I added an assert about it.
>  - added a few braces on if/if/else/if/else/else
>  - added an UNKNOWN_HOST macro to hide "???"
>  - moved host_addr[] declaration earlier to avoid some braces

You forgot to free(conn->connip) during freePGconn().

I found the UNKNOWN_HOST business quite dubious -- not only in your
patch but also in the existing coding.  I changed the getHostname API so
that instead of returning "???" it sets the output buffer to the empty
string.  AFAICS the only user-visible behavior is that we no longer
display the "???" in a parenthical comment next to the server address
when a connection fails (this is pre-existing behavior, not changed by
your patch.)

Now, maybe the thinking was that upon seeing this message:

could not connect to server: some error here
Is the server running on host "pg.mines-paristech.fr" (???) and accepting
connections on port 5432?

the user was going to think "oh, my machine must have run out of memory,
I'll reboot and retry".  However, I highly doubt that anybody would
reach that conclusion without reading the source code.  So I deem this
useless.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Pushed, thanks.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Hello Alvaro,

>>  - libpq internal function getHostaddr get a length,
>>    and I added an assert about it.
>>  - added a few braces on if/if/else/if/else/else
>>  - added an UNKNOWN_HOST macro to hide "???"
>>  - moved host_addr[] declaration earlier to avoid some braces
>
> You forgot to free(conn->connip) during freePGconn().

Argh, indeed:-(

> I found the UNKNOWN_HOST business quite dubious -- not only in your
> patch but also in the existing coding.

So did I:-) I only kept it because it was already done like that.

> I changed the getHostname API so that instead of returning "???" it sets 
> the output buffer to the empty string.

I hesitated to do exactly that. I'm fine with that.

Thanks for the push.

-- 
Fabien.

On 2018-Nov-09, Michael Paquier wrote:

> On Thu, Nov 08, 2018 at 12:13:31PM -0300, Alvaro Herrera wrote:

> > Right, that too.  Fortunately I think compilers warn about
> > mismatching indentation nowadays, at least in some cases.
> 
> I don't recall seeing a compiler warning about that, but I do recall
> coverity complaining loudly about such things.

:-)

/pgsql/source/master/src/backend/catalog/namespace.c: In function 'InitRemoveTempRelations':
/pgsql/source/master/src/backend/catalog/namespace.c:4132:2: warning: this 'if' clause does not guard...
[-Wmisleading-indentation]
  if (OidIsValid(namespaceOid))
  ^~
/pgsql/source/master/src/backend/catalog/namespace.c:4134:3: note: ...this statement, but the latter is misleadingly
indentedas if it is guarded by the 'if'
 
   get_namespace_oid(namespaceName, true);
   ^~~~~~~~~~~~~~~~~

$ gcc --version
gcc (Debian 6.3.0-18+deb9u1) 6.3.0 20170516

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Mon, Nov 19, 2018 at 12:53:15PM -0300, Alvaro Herrera wrote:
> commit 6e5f8d4
> Commit:     Alvaro Herrera <alvherre@alvh.no-ip.org>
> CommitDate: Mon Nov 19 14:34:12 2018 -0300
> 
>     psql: Show IP address in \conninfo

>     Discussion: https://postgr.es/m/alpine.DEB.2.21.1810261532380.27686@lancre
>         https://postgr.es/m/alpine.DEB.2.21.1808201323020.13832@lancre

> --- a/src/bin/psql/command.c
> +++ b/src/bin/psql/command.c
> @@ -2894,12 +2911,27 @@ do_connect(enum trivalue reuse_previous_specification,
>      }
>  
>      /* grab missing values from the old connection */
> -    if (!user && reuse_previous)
> -        user = PQuser(o_conn);
> -    if (!host && reuse_previous)
> -        host = PQhost(o_conn);
> -    if (!port && reuse_previous)
> -        port = PQport(o_conn);
> +    if (reuse_previous)
> +    {
> +        if (!user)
> +            user = PQuser(o_conn);
> +        if (host && strcmp(host, PQhost(o_conn)) == 0)
> +        {
> +            /*
> +             * if we are targetting the same host, reuse its hostaddr for
> +             * consistency
> +             */
> +            hostaddr = PQhostaddr(o_conn);
> +        }
> +        if (!host)
> +        {
> +            host = PQhost(o_conn);
> +            /* also set hostaddr for consistency */
> +            hostaddr = PQhostaddr(o_conn);
> +        }
> +        if (!port)
> +            port = PQport(o_conn);
> +    }
>  
>      /*
>       * Any change in the parameters read above makes us discard the password.

The "hostaddr = PQhostaddr(o_conn)" branches make \connect use the same IP
address as the existing connection.  I like that when the existing connection
uses a hostaddr= parameter, but I doubt it's the right thing otherwise.  If
the server IP changes, \connect should find the server at its new IP.  If the
server has multiple IPs, \connect should have the opportunity to try them all,
just like the original connection attempt could have.

Other than that, the \connect behavior change makes sense to me.  However,
nothing updated \connect documentation.  (Even the commit log message said
nothing about \connect.)

> @@ -3071,14 +3108,27 @@ do_connect(enum trivalue reuse_previous_specification,

>              /* If the host is an absolute path, the connection is via socket */

This comment is no longer correct.  Copy the other "via socket" comment.

> --- a/src/interfaces/libpq/fe-connect.c
> +++ b/src/interfaces/libpq/fe-connect.c
> @@ -1471,6 +1471,39 @@ connectNoDelay(PGconn *conn)
>      return 1;
>  }
>  
> +/* ----------
> + * Write currently connected IP address into host_addr (of len host_addr_len).
> + * If unable to, set it to the empty string.
> + * ----------
> + */
> +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.  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".

> @@ -6173,6 +6202,25 @@ PQhost(const PGconn *conn)
>  }
>  
>  char *
> +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.)

> On Mon, May 27, 2019 at 10:38 PM Noah Misch <noah@leadboat.com> wrote:
>
> On Mon, Nov 19, 2018 at 12:53:15PM -0300, Alvaro Herrera wrote:
> > commit 6e5f8d4
> > Commit:     Alvaro Herrera <alvherre@alvh.no-ip.org>
> > CommitDate: Mon Nov 19 14:34:12 2018 -0300
> >
> >     psql: Show IP address in \conninfo
>
> >     Discussion: https://postgr.es/m/alpine.DEB.2.21.1810261532380.27686@lancre
> >       https://postgr.es/m/alpine.DEB.2.21.1808201323020.13832@lancre
>
> > --- a/src/bin/psql/command.c
> > +++ b/src/bin/psql/command.c
> > @@ -2894,12 +2911,27 @@ do_connect(enum trivalue reuse_previous_specification,
> >       }
> >
> >       /* grab missing values from the old connection */
> > -     if (!user && reuse_previous)
> > -             user = PQuser(o_conn);
> > -     if (!host && reuse_previous)
> > -             host = PQhost(o_conn);
> > -     if (!port && reuse_previous)
> > -             port = PQport(o_conn);
> > +     if (reuse_previous)
> > +     {
> > +             if (!user)
> > +                     user = PQuser(o_conn);
> > +             if (host && strcmp(host, PQhost(o_conn)) == 0)
> > +             {
> > +                     /*
> > +                      * if we are targetting the same host, reuse its hostaddr for
> > +                      * consistency
> > +                      */
> > +                     hostaddr = PQhostaddr(o_conn);
> > +             }
> > +             if (!host)
> > +             {
> > +                     host = PQhost(o_conn);
> > +                     /* also set hostaddr for consistency */
> > +                     hostaddr = PQhostaddr(o_conn);
> > +             }
> > +             if (!port)
> > +                     port = PQport(o_conn);
> > +     }
> >
> >       /*
> >        * Any change in the parameters read above makes us discard the password.
>
> The "hostaddr = PQhostaddr(o_conn)" branches make \connect use the same IP
> address as the existing connection.  I like that when the existing connection
> uses a hostaddr= parameter, but I doubt it's the right thing otherwise.  If
> the server IP changes, \connect should find the server at its new IP.  If the
> server has multiple IPs, \connect should have the opportunity to try them all,
> just like the original connection attempt could have.
>
> Other than that, the \connect behavior change makes sense to me.  However,
> nothing updated \connect documentation.  (Even the commit log message said
> nothing about \connect.)

Given that it's an open item for PostgreSQL 12, I've decided to take a look.
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. Although I guess it can be avoided by
`-reuse-previous=off`, probably it makese sense to update the docs.

Hello Dmitry,

> Given that it's an open item for PostgreSQL 12,

I'm working on it, but slowly.

> I've decided to take a look.

Thanks!

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

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

-- 
Fabien.

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

On Tue, Jun 11, 2019 at 01:59:20PM +0200, Dmitry Dolgov wrote:
> > On Tue, Jun 11, 2019 at 6:46 AM Fabien COELHO <coelho@cri.ensmp.fr> wrote:

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

No, I was arguing that a behavior should revert back its v11 behavior:

\connect mydb myuser myhost
-- should resolve "myhost" again, like it did in v11
\connect

\connect "dbname=mydb host=myhost hostaddr=127.0.0.1"
-- ok to reuse hostaddr=127.0.0.1; I agree that's a feature
\connect

> On Wed, Jun 12, 2019 at 9:45 AM Noah Misch <noah@leadboat.com> wrote:
>
> On Tue, Jun 11, 2019 at 01:59:20PM +0200, Dmitry Dolgov wrote:
> > > On Tue, Jun 11, 2019 at 6:46 AM Fabien COELHO <coelho@cri.ensmp.fr> wrote:
>
> > > > 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.
>
> No, I was arguing that a behavior should revert back its v11 behavior:
>
> \connect mydb myuser myhost
> -- should resolve "myhost" again, like it did in v11
> \connect
>
> \connect "dbname=mydb host=myhost hostaddr=127.0.0.1"
> -- ok to reuse hostaddr=127.0.0.1; I agree that's a feature
> \connect

Oh, ok, sorry for misunderstanding.

Hello Noah,

>>>> 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.
>
> No, I was arguing that a behavior should revert back its v11 behavior:

I got that. I'm working on it, and on the other issues you raised.

The issue I see is what do we want when a name resolves to multiple 
addresses. The answer is not fully obvious to me right now. I'll try to 
send a patch over the week-end.

> \connect mydb myuser myhost
> -- should resolve "myhost" again, like it did in v11
> \connect
>
> \connect "dbname=mydb host=myhost hostaddr=127.0.0.1"
> -- ok to reuse hostaddr=127.0.0.1; I agree that's a feature
> \connect

-- 
Fabien.

Hello,

> I got that. I'm working on it, and on the other issues you raised.
>
> The issue I see is what do we want when a name resolves to multiple 
> addresses. The answer is not fully obvious to me right now. I'll try to send 
> a patch over the week-end.

At Alvaro's request, here is a quick WIP patch, that does not do the right 
thing, because there is no simple way to know whether hostaddr was set at 
the libPQ level, so either we set it always, about which Noah complained, 
or we don't, about which someone else will complain quite easily, i.e. 
with this patch

   \c "host=foo hostaddr=ip"

connects to ip, but then

   \c

will reconnect to foo but ignore ip. Well, ISTM that this is back to the 
previous doubtful behavior, so at least it is not a regression, just the 
same bug:-)

A solution could be to have a PQdoestheconnectionuseshostaddr(conn) 
function, but I cannot say I'd be thrilled.

Another option would be to import PGconn full definition in 
"psql/command.c", but that would break the PQ interface, I cannot say I'd 
be thrilled either.

The patch returns host as defined by the user, but the regenerated 
hostaddr (aka connip), which is not an homogeneous behavior. PQhost should 
probably use connip if host was set as an ip, but that needs guessing.

The underlying issue is that the host/hostaddr stuff is not that easy to 
fix.

At least, after the patch the connection information (\conninfo) is still 
the right one, which is an improvement.

-- 
Fabien.

On Wed, Jun 12, 2019 at 02:44:49PM +0200, Fabien COELHO wrote:
> there is no simple way to know whether hostaddr was set at
> the libPQ level

> A solution could be to have a PQdoestheconnectionuseshostaddr(conn)
> function, but I cannot say I'd be thrilled.

PQconninfo() is the official way to retrieve that.

Hello Noah,

>> there is no simple way to know whether hostaddr was set at
>> the libPQ level
>
>> A solution could be to have a PQdoestheconnectionuseshostaddr(conn)
>> function, but I cannot say I'd be thrilled.
>
> PQconninfo() is the official way to retrieve that.

Thanks for the pointer! I did not notice this one. At least the API looks 
better than the one I was suggesting:-)

ISTM that this function could be used to set other parameters, fixing some 
other issues such as ignoring special parameters on reconnections.

Anyway, attached an attempt at implementing the desired behavior wrt 
hostaddr.

-- 
Fabien.

On 2019-May-27, Noah Misch wrote:

> Other than that, the \connect behavior change makes sense to me.  However,
> nothing updated \connect documentation.  (Even the commit log message said
> nothing about \connect.)

I added this blurb to the paragraph that documents the reusing behavior:

        If <literal>hostaddr</literal> was specified in the original
        connection's <structname>conninfo</structname>, that address is reused
        for the new connection (disregarding any other host specification).

Thanks for reporting these problems.  I'm going to push this shortly.
We can revise over the weekend, if there's need.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 2019-Jun-13, Fabien COELHO wrote:

> Thanks for the pointer! I did not notice this one. At least the API looks
> better than the one I was suggesting:-)
> 
> ISTM that this function could be used to set other parameters, fixing some
> other issues such as ignoring special parameters on reconnections.
> 
> Anyway, attached an attempt at implementing the desired behavior wrt
> hostaddr.

BTW I tested this manually and it seemed to work as intended, ie., if I
change /etc/hosts for the hostname I am using between one connection and
the next, we do keep the hostaddr if it was specified, or we resolve the
name again if it's not.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Fri, Jun 14, 2019 at 06:31:52PM -0400, Alvaro Herrera wrote:
> BTW I tested this manually and it seemed to work as intended, ie., if I
> change /etc/hosts for the hostname I am using between one connection and
> the next, we do keep the hostaddr if it was specified, or we resolve the
> name again if it's not.

Alvaro, did 313f56ce fix all the issues related to this thread?
Perhaps this open item can now be closed?
--
Michael

On 2019-Jun-24, Michael Paquier wrote:

> On Fri, Jun 14, 2019 at 06:31:52PM -0400, Alvaro Herrera wrote:
> > BTW I tested this manually and it seemed to work as intended, ie., if I
> > change /etc/hosts for the hostname I am using between one connection and
> > the next, we do keep the hostaddr if it was specified, or we resolve the
> > name again if it's not.
> 
> Alvaro, did 313f56ce fix all the issues related to this thread?

Yes, as far as I am aware it does.

> Perhaps this open item can now be closed?

I left it there so that others could double-check if there were still
some things needing tweaks.  Feel free to close it now, thanks.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Mon, Jun 24, 2019 at 08:52:00AM -0400, Alvaro Herrera wrote:
> On 2019-Jun-24, Michael Paquier wrote:
> > On Fri, Jun 14, 2019 at 06:31:52PM -0400, Alvaro Herrera wrote:
> > > BTW I tested this manually and it seemed to work as intended, ie., if I
> > > change /etc/hosts for the hostname I am using between one connection and
> > > the next, we do keep the hostaddr if it was specified, or we resolve the
> > > name again if it's not.
> > 
> > Alvaro, did 313f56ce fix all the issues related to this thread?
> 
> Yes, as far as I am aware it does.

I agree, it did.

On Mon, Jun 24, 2019 at 04:51:04PM -0700, Noah Misch wrote:
> On Mon, Jun 24, 2019 at 08:52:00AM -0400, Alvaro Herrera wrote:
>> On 2019-Jun-24, Michael Paquier wrote:
>>> Alvaro, did 313f56ce fix all the issues related to this thread?
>>
>> Yes, as far as I am aware it does.
>
> I agree, it did.

Indeed.  I have been looking at the thread and I can see the
difference with and without the patch committed.  This open item is
closed.
--
Michael