fix psql \conninfo & \connect when using hostaddr

Hi

pá 26. 10. 2018 v 15:55 odesílatel Fabien COELHO <coelho@cri.ensmp.fr>
napsal:

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

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

If there are no be a objection, I'll mark this patch as ready for commiters

Regards

Pavel

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

st 7. 11. 2018 v 15:11 odesílatel Arthur Zakirov <a.zakirov@postgrespro.ru>
napsal:

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.

?

Pavel

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 que vivía un hidalgo de los de lanza en astillero_ adarga antigua_ rocín flaco y galgo corredor./Una olla de algo más vaca que carnero_ salpicón las más noches_ duelos y quebrantos los sábados_ lentejas los viernes_ algún palomino de añadidura los domingos_ consumían las tres partes de su hacienda./El resto della concluían sayo de velarte_ calzas de velludo para 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ía en su casa una ama que pasaba de los cuarenta_ y una sobrina que no llegaba a los veinte_ y un mozo de campo y plaza_ que así ensillaba el rocín como tomaba la podadera./Frisaba la edad de nuestro hidalgo con los cincuenta años_ era de complexión recia_ seco de carnes_ enjuto de rostro; gran madrugador y amigo de la caza./Quieren decir que tenía el sobrenombre 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 or service 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 or service 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 service not 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 ama que 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 con los 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 sobrenombre de Quijada o Quesada (que en esto hay alguna diferencia en los autores que deste caso escriben)" to address: Name or service not known
could not translate host name " aunque por conjeturas verosímiles se deja entender que se llama Quijana;/pero esto importa poco a nuestro cuento; basta que en la narración dél no se salga un punto de la verdad." to address: Name or service not 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 nombre no 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.