Date: Mon, 27 May 2019 16:37:53 -0400
From: Noah Misch <noah@leadboat.com>
To: Alvaro Herrera <alvherre@2ndquadrant.com>
Cc: Fabien COELHO <coelho@cri.ensmp.fr>,
	PostgreSQL Developers <pgsql-hackers@lists.postgresql.org>,
	Arthur Zakirov <a.zakirov@postgrespro.ru>,
	Robert Haas <robertmhaas@gmail.com>
Subject: Re: fix psql \conninfo & \connect when using hostaddr
Message-ID: <20190527203713.GA58392@gust.leadboat.com>
References: <alpine.DEB.2.21.1811171703090.16105@lancre>
 <20181119155315.vsrgj2droozqgfqo@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20181119155315.vsrgj2droozqgfqo@alvherre.pgsql>
User-Agent: Mutt/1.5.24 (2015-08-30)
Precedence: bulk

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