Enhance pg_stat_wal_receiver view to display connected host

On Thu, Dec 21, 2017 at 8:16 PM, Haribabu Kommi
<kommi.haribabu@gmail.com> wrote:

The current connected host details are already available in the PGconn
structure,
Exposing those details in the view will suffice this requirement. Currently
these
members are characters pointers, I used them as it is and displayed them in
the
view as character arrays except the port number.

Attached the draft patch. Any comments?

I agree that it would be nice to have an equivalent to
pg_stat_replication's client_addr, client_hostname, client_port on the
receiver side, particularly because it is possible to list multiple
hosts and ports. Could you add that to the next commit fest?

Please note that you need to update rules.out and the documentation
because of the new fields.
--
Michael

On Thu, Dec 21, 2017 at 11:12 PM, Michael Paquier <michael.paquier@gmail.com

wrote:

On Thu, Dec 21, 2017 at 8:16 PM, Haribabu Kommi
<kommi.haribabu@gmail.com> wrote:

The current connected host details are already available in the PGconn
structure,
Exposing those details in the view will suffice this requirement.

Currently

these
members are characters pointers, I used them as it is and displayed them

in

the
view as character arrays except the port number.

Attached the draft patch. Any comments?

I agree that it would be nice to have an equivalent to
pg_stat_replication's client_addr, client_hostname, client_port on the
receiver side, particularly because it is possible to list multiple
hosts and ports. Could you add that to the next commit fest?

Added.

Please note that you need to update rules.out and the documentation
because of the new fields.

Updated patch attached with tests and doc changes.

Regards,
Hari Babu
Fujitsu Australia

On Fri, Dec 22, 2017 at 03:11:07PM +1100, Haribabu Kommi wrote:

Updated patch attached with tests and doc changes.

+    <row>
+     <entry><structfield>primary_hostname</structfield></entry>
+     <entry><type>text</type></entry>
+     <entry>Host name of the primary connected by this WAL receiver</entry>
+    </row>
+    <row>
+     <entry><structfield>primary_hostaddr</structfield></entry>
+     <entry><type>text</type></entry>
+     <entry>Host address of the primary connected by this WAL receiver</entry>
+    </row>
+    <row>
+     <entry><structfield>primary_port</structfield></entry>
+     <entry><type>integer</type></entry>
+     <entry>port number of the primary connected by this WAL receiver</entry>
+    </row>
A WAL receiver could be connected to a standby as well with cascading
replication, so "primary" does not sound like a term adapted to me.
Why not just saying "XXX of the PostgreSQL instance this WAL receiver is
connected to"? Similarly the field names don't seem adapted to me. Would
"remote" be more adapted? Other ideas are of course welcome.

+	char		host[NAMEDATALEN];
+	char		hostaddr[NAMEDATALEN];
Those two should use NI_MAXHOST as maximum length. getaddrinfo() relies on
that.

+	retval =  PQconnectedhostinfo(conn->streamConn, PQ_HOST_ADDRESS);
+	if (retval)
+		*hostaddr = pstrdup(retval);
Yes, going through PQconnectedhostinfo() is a good idea at the end of
the day, as well as keeping one API with libpqrcv_get_conninfo().
-- 
Michael

On Fri, Dec 22, 2017 at 4:55 PM, Michael Paquier <michael.paquier@gmail.com>
wrote:

On Fri, Dec 22, 2017 at 03:11:07PM +1100, Haribabu Kommi wrote:

Updated patch attached with tests and doc changes.

Thanks for the review.

+    <row>
+     <entry><structfield>primary_hostname</structfield></entry>
+     <entry><type>text</type></entry>
+     <entry>Host name of the primary connected by this WAL
receiver</entry>
+    </row>
+    <row>
+     <entry><structfield>primary_hostaddr</structfield></entry>
+     <entry><type>text</type></entry>
+     <entry>Host address of the primary connected by this WAL
receiver</entry>
+    </row>
+    <row>
+     <entry><structfield>primary_port</structfield></entry>
+     <entry><type>integer</type></entry>
+     <entry>port number of the primary connected by this WAL
receiver</entry>
+    </row>
A WAL receiver could be connected to a standby as well with cascading
replication, so "primary" does not sound like a term adapted to me.
Why not just saying "XXX of the PostgreSQL instance this WAL receiver is
connected to"? Similarly the field names don't seem adapted to me. Would
"remote" be more adapted? Other ideas are of course welcome.

changed the description and field names also to remote instead of primary.

+       char            host[NAMEDATALEN];
+       char            hostaddr[NAMEDATALEN];
Those two should use NI_MAXHOST as maximum length. getaddrinfo() relies on
that.

Corrected.

+       retval =  PQconnectedhostinfo(conn->streamConn, PQ_HOST_ADDRESS);
+       if (retval)
+               *hostaddr = pstrdup(retval);
Yes, going through PQconnectedhostinfo() is a good idea at the end of
the day, as well as keeping one API with libpqrcv_get_conninfo().

Changed as one API call.

update patch attached.

Regards,
Hari Babu
Fujitsu Australia

On Wed, Jan 3, 2018 at 12:25 PM, Haribabu Kommi <kommi.haribabu@gmail.com>
wrote:

update patch attached.

Last patch has undefined symbol, corrected patch attached.

Regards,
Hari Babu
Fujitsu Australia

On Wed, Jan 03, 2018 at 06:48:07PM +1100, Haribabu Kommi wrote:

On Wed, Jan 3, 2018 at 12:25 PM, Haribabu Kommi <kommi.haribabu@gmail.com>
Last patch has undefined symbol, corrected patch attached.

+	memset(walrcv->host, 0, NAMEDATALEN);
+	if (host)
+		strlcpy((char *) walrcv->host, host, NAMEDATALEN);
+
+	memset(walrcv->hostaddr, 0, NAMEDATALEN);
+	if (hostaddr)
+		strlcpy((char *) walrcv->hostaddr, hostaddr, NAMEDATALEN);
You need to use NI_MAXHOST for both things here.

+    <row>
+     <entry><structfield>remote_hostname</structfield></entry>
+     <entry><type>text</type></entry>
+     <entry>Host name of the PostgreSQL instance this WAL receiver is connected to</entry>
+    </row>
PostgreSQL is usualy referred to with the <productname> markup. Those
should be split on multiple lines. The doc changes are nits though.

I have done some testing with this patch with primary_conninfo using
multiple values of host and port, and the correct values are being
reported, which is a nice feature.
--
Michael

On Thu, Jan 4, 2018 at 11:53 AM, Michael Paquier <michael.paquier@gmail.com>
wrote:

On Wed, Jan 03, 2018 at 06:48:07PM +1100, Haribabu Kommi wrote:

On Wed, Jan 3, 2018 at 12:25 PM, Haribabu Kommi <

kommi.haribabu@gmail.com>

Last patch has undefined symbol, corrected patch attached.

Thanks for the review.

+ memset(walrcv->host, 0, NAMEDATALEN);

+       if (host)
+               strlcpy((char *) walrcv->host, host, NAMEDATALEN);
+
+       memset(walrcv->hostaddr, 0, NAMEDATALEN);
+       if (hostaddr)
+               strlcpy((char *) walrcv->hostaddr, hostaddr, NAMEDATALEN);
You need to use NI_MAXHOST for both things here.

Corrected.

+    <row>
+     <entry><structfield>remote_hostname</structfield></entry>
+     <entry><type>text</type></entry>
+     <entry>Host name of the PostgreSQL instance this WAL receiver is
connected to</entry>
+    </row>
PostgreSQL is usualy referred to with the <productname> markup. Those
should be split on multiple lines. The doc changes are nits though.

updated the documentation with markups.

I have done some testing with this patch with primary_conninfo using

multiple values of host and port, and the correct values are being
reported, which is a nice feature.

Thanks for testing. updated patch attached.

Regards,
Hari Babu
Fujitsu Australia

I think more attention should be given to the libpq side of this patch;
maybe have a 0001 with only the new libpq function, to easily verify
that it does all it needs to do. It needs docs for the new function in
libpq.sgml; also I wonder if checking conn->status before reporting
values is necessary; finally, has the application any good way to check
that the values can be safely read after calling the new function?

Nit: the new libpq function name is not great with all those lowercase
letters. Better to make it camelCase like the rest of the libpq API?

Thanks for the patch,

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

On Thu, Jan 04, 2018 at 05:21:02PM +1100, Haribabu Kommi wrote:

On Thu, Jan 4, 2018 at 11:53 AM, Michael Paquier <michael.paquier@gmail.com>
wrote:

On Wed, Jan 03, 2018 at 06:48:07PM +1100, Haribabu Kommi wrote:

On Wed, Jan 3, 2018 at 12:25 PM, Haribabu Kommi <

kommi.haribabu@gmail.com>

Last patch has undefined symbol, corrected patch attached.

Thanks for the review.

Almost there.

Sorry, I have just noticed that the comment on top of
libpqrcv_get_conninfo() needs a refresh. With your patch more
information than a siple connection string are returned to the caller.

Some initialization of the return values should happen directly inside
walrcv_get_conninfo(), or get the feeling that we'll be trapped in the
future if this gets called somewhere else.

[nit]
+     <entry>
+      port number of the <productname>PostgreSQL</productname> instance
+      this WAL receiver is connected to.
+     </entry>
Missing an upper case at the beginning of the sentence here.
[/nit]
--
Michael

On Thu, Jan 04, 2018 at 08:54:37AM -0300, Alvaro Herrera wrote:

I think more attention should be given to the libpq side of this patch;
maybe have a 0001 with only the new libpq function, to easily verify
that it does all it needs to do. It needs docs for the new function in
libpq.sgml; also I wonder if checking conn->status before reporting
values is necessary; finally, has the application any good way to check
that the values can be safely read after calling the new function?

Or instead of reinventing again the wheel, why not removing
remote_hostaddr, and fetch the wanted values from PQhost() and PQport()
after making sure that the connection status is good? There is no need
for a new API this way. And as bonus points, we can also rely on
defaults.
--
Michael

On Fri, Jan 5, 2018 at 12:05 AM, Michael Paquier <michael.paquier@gmail.com>
wrote:

On Thu, Jan 04, 2018 at 08:54:37AM -0300, Alvaro Herrera wrote:

I think more attention should be given to the libpq side of this patch;
maybe have a 0001 with only the new libpq function, to easily verify
that it does all it needs to do. It needs docs for the new function in
libpq.sgml; also I wonder if checking conn->status before reporting
values is necessary; finally, has the application any good way to check
that the values can be safely read after calling the new function?

Or instead of reinventing again the wheel, why not removing
remote_hostaddr, and fetch the wanted values from PQhost() and PQport()
after making sure that the connection status is good? There is no need
for a new API this way. And as bonus points, we can also rely on
defaults.

PQhost() doesn't provide the proper details even if we remove the
remote_hostaddr. For example with the following conninfo,

host=host1,host2 hostaddr=127.0.0.1,127.0.0.1 port=5434,5432

The connection type for both address of the above conninfo is
CHT_HOST_ADDRESS. so the PQhost() returns the conn->pghost
value i.e "host1,host2". That returned value doesn't give the clarity to
which host it is exactly connected. Because of this reason only, I came
up with a new function.

How about changing the PQhost() function behavior? Instead of checking
the connection type, checking whether there exists any host name or not?
And also not returning "default host" details, because for the conninfo
without any host details, the return value must be NULL. But this change
may break the backward compatibility of the function.

or

write two new functions PQconnhost() and PQconnhostaddr() to return the
connected host and hostaddr and reuse the PQport() function.

Regards,
Hari Babu
Fujitsu Australia

Haribabu Kommi wrote:

And also not returning "default host" details, because for the conninfo
without any host details, the return value must be NULL. But this change
may break the backward compatibility of the function.

I wouldn't want to have to fight that battle.

or

write two new functions PQconnhost() and PQconnhostaddr() to return the
connected host and hostaddr and reuse the PQport() function.

How about using an API similar to PQconninfo, where we return an array
of connection options used? Say, PQeffectiveConninfo(). This seems to
me to reduce ugliness in the API, and be more generally useful.

walrecvr could display as an array or just flatten to a string -- not
sure what's the better option there.

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

On Fri, Jan 5, 2018 at 11:15 PM, Alvaro Herrera <alvherre@alvh.no-ip.org>
wrote:

Haribabu Kommi wrote:

or

write two new functions PQconnhost() and PQconnhostaddr() to return the
connected host and hostaddr and reuse the PQport() function.

How about using an API similar to PQconninfo, where we return an array
of connection options used? Say, PQeffectiveConninfo(). This seems to
me to reduce ugliness in the API, and be more generally useful.

OK. Added the new API PQeffectiveConninfo() that returns all the connection
options that are actively used. Currently the connection options host,
hostaddr
and port may change based on the active connection and rest of the options
may be same.

walrecvr could display as an array or just flatten to a string -- not

sure what's the better option there.

Currently I went with a string model to display all the effective_conninfo
options. I feel if we go with string approach, adding a new option that gets
updated in future is simple.

postgres=# select conninfo, effective_conninfo from pg_stat_wal_receiver;
-[ RECORD 1 ]------+----------------------------------------------------
------------------------------------------------------------
------------------------------------------------------------
-------------------------------------------------
conninfo | user=kommih passfile=/home/kommih/.pgpass
dbname=replication hostaddr=127.0.0.1,127.0.0.1 port=5434,5432
application_name=s2 fallback_application_name=walreceiver sslmode=disable
sslcompression=1 target_session_attrs=any
effective_conninfo | user=kommih passfile=/home/kommih/.pgpass
dbname=replication hostaddr=127.0.0.1 port=5432 application_name=s2
fallback_application_name=walreceiver sslmode=disable sslcompression=1
target_session_attrs=any

Majority of the options are same in both conninfo and effective_conninfo
columns.
Instead of "effective_conninfo" column, how about something like
"remote_server"
as string that displays only the host, hostaddr and port options that
differs with
each connection?

Current set of patches are attached.

Regards,
Hari Babu
Fujitsu Australia

On Fri, Jan 05, 2018 at 09:15:36AM -0300, Alvaro Herrera wrote:

Haribabu Kommi wrote:

And also not returning "default host" details, because for the conninfo
without any host details, the return value must be NULL. But this change
may break the backward compatibility of the function.

I wouldn't want to have to fight that battle.

Hm. Any users of psql's PROMPT would be equally confused, and this can
actually lead to more confusion from the user prospective I think than
just pg_stat_wal_receiver. If you take the case of Haribabu from
upthread with say this bit in psqlrc:
\set PROMPT1 '[host=%M;port=%>]=%# '

Then you get on HEAD the following set of results using different
connection strings:
1) host=localhost,localhost hostaddr=127.0.0.1,127.0.0.1 port=5432,5433
[host=localhost,localhost;port=5432]=#

2) host=localhost,localhost port=5432,5433
[host=localhost;port=5432]=#

3) hostaddr=127.0.0.1,127.0.0.1 port=5432,5433
[host=[local];port=5432]=#

4) host=/tmp,tmp hostaddr=127.0.0.1,127.0.0.1
[host=[local:/tmp,tmp];port=5432]=#

So for cases 2) and 4), mixing both hostaddr and host is hurting the
experience. The documentation in [1]https://www.postgresql.org/docs/current/static/libpq-connect.html -- Michael also specifies that if both host
and hostaddrs are specified then host is ignored. The same rule applies
for multiple values so for 2) and 4) the correct values ought to be
"local" for both of them. This would be more consistent with the pre-9.6
behavior as well.

[1]: https://www.postgresql.org/docs/current/static/libpq-connect.html -- Michael
--
Michael

On Tue, Jan 9, 2018 at 12:15 PM, Michael Paquier <michael.paquier@gmail.com>
wrote:

On Fri, Jan 05, 2018 at 09:15:36AM -0300, Alvaro Herrera wrote:

Haribabu Kommi wrote:

And also not returning "default host" details, because for the conninfo
without any host details, the return value must be NULL. But this

change

may break the backward compatibility of the function.

I wouldn't want to have to fight that battle.

Hm. Any users of psql's PROMPT would be equally confused, and this can
actually lead to more confusion from the user prospective I think than
just pg_stat_wal_receiver. If you take the case of Haribabu from
upthread with say this bit in psqlrc:
\set PROMPT1 '[host=%M;port=%>]=%# '

Then you get on HEAD the following set of results using different
connection strings:
1) host=localhost,localhost hostaddr=127.0.0.1,127.0.0.1 port=5432,5433
[host=localhost,localhost;port=5432]=#

2) host=localhost,localhost port=5432,5433
[host=localhost;port=5432]=#

3) hostaddr=127.0.0.1,127.0.0.1 port=5432,5433
[host=[local];port=5432]=#

4) host=/tmp,tmp hostaddr=127.0.0.1,127.0.0.1
[host=[local:/tmp,tmp];port=5432]=#

So for cases 2) and 4), mixing both hostaddr and host is hurting the
experience. The documentation in [1] also specifies that if both host
and hostaddrs are specified then host is ignored. The same rule applies
for multiple values so for 2) and 4) the correct values ought to be
"local" for both of them. This would be more consistent with the pre-9.6
behavior as well.

I think you mean to say for the cases 1) and 4)? because those are the
cases where it differs with pre-9.6 behavior. With the attached patch
of changing PQhost() to return the host if exists, irrespective of the
connection type will bring back the pre-9.6 behavior.

1) host=localhost,localhost hostaddr=127.0.0.1,127.0.0.1 port=5432,5433
[host=localhost;port=5432]=#

4) host=/tmp,tmp hostaddr=127.0.0.1,127.0.0.1
[host=[local];port=5432]=#

Even for default unix domain socket connection,
conn->connhost[conn->whichhost].host
is filled with the details, but not the global member. So no need of
checking global member and returning the same in PQhost() function.

Regards,
Hari Babu
Fujitsu Australia

On Wed, Jan 10, 2018 at 04:10:35PM +1100, Haribabu Kommi wrote:

On Tue, Jan 9, 2018 at 12:15 PM, Michael Paquier <michael.paquier@gmail.com>
wrote:

Hm. Any users of psql's PROMPT would be equally confused, and this can
actually lead to more confusion from the user prospective I think than
just pg_stat_wal_receiver. If you take the case of Haribabu from
upthread with say this bit in psqlrc:
\set PROMPT1 '[host=%M;port=%>]=%# '

Then you get on HEAD the following set of results using different
connection strings:
1) host=localhost,localhost hostaddr=127.0.0.1,127.0.0.1 port=5432,5433
[host=localhost,localhost;port=5432]=#

2) host=localhost,localhost port=5432,5433
[host=localhost;port=5432]=#

3) hostaddr=127.0.0.1,127.0.0.1 port=5432,5433
[host=[local];port=5432]=#

4) host=/tmp,tmp hostaddr=127.0.0.1,127.0.0.1
[host=[local:/tmp,tmp];port=5432]=#

So for cases 2) and 4), mixing both hostaddr and host is hurting the
experience. The documentation in [1] also specifies that if both host
and hostaddrs are specified then host is ignored. The same rule applies
for multiple values so for 2) and 4) the correct values ought to be
"local" for both of them. This would be more consistent with the pre-9.6
behavior as well.

I think you mean to say for the cases 1) and 4)? because those are the
cases where it differs with pre-9.6 behavior.

Sure, sorry for the mistake. That's indeed what I meant.

With the attached patch of changing PQhost() to return the host if
exists, irrespective of the connection type will bring back the
pre-9.6 behavior.

1) host=localhost,localhost hostaddr=127.0.0.1,127.0.0.1 port=5432,5433
[host=localhost;port=5432]=#

4) host=/tmp,tmp hostaddr=127.0.0.1,127.0.0.1
[host=[local];port=5432]=#

Even for default unix domain socket connection,
conn->connhost[conn->whichhost].host
is filled with the details, but not the global member. So no need of
checking global member and returning the same in PQhost() function.

Thanks for the patch and the investigation, this visibly points out to
the fact that 11003eb5 did not get it completely right either. I am
adding Robert in CC for some input on the matter. To me, this looks like
a bug that should be applied down to v10. I think that it would be better
to spawn a new thread as well to raise awareness on the matter. This is
quite different than the patch you are presenting here. What do you
think?

I have redone my set of previous tests and can confirm that PQhost is
behaving as I would expect it should, and those results are the same as
yours.

With your patch, please note also that the SSL test suite does not
complain, which is an excellent thing!
--
Michael

On Fri, Jan 12, 2018 at 11:37:22AM +0900, Michael Paquier wrote:

I have redone my set of previous tests and can confirm that PQhost is
behaving as I would expect it should, and those results are the same as
yours.

    if (conn->connhost != NULL &&
-       conn->connhost[conn->whichhost].type != CHT_HOST_ADDRESS)
+           conn->connhost[conn->whichhost].host != NULL &&
+           conn->connhost[conn->whichhost].host[0] != '\0')
        return conn->connhost[conn->whichhost].host;
-   else if (conn->pghost != NULL && conn->pghost[0] != '\0')
-       return conn->pghost;

Upon further review, the second bit of the patch is making me itching. I
think that you should not remove the second check which returns
conn->pghost if a value is found in it.
--
Michael

On Fri, Jan 12, 2018 at 3:26 PM, Michael Paquier <michael.paquier@gmail.com>
wrote:

On Fri, Jan 12, 2018 at 11:37:22AM +0900, Michael Paquier wrote:

I have redone my set of previous tests and can confirm that PQhost is
behaving as I would expect it should, and those results are the same as
yours.

if (conn->connhost != NULL &&
-       conn->connhost[conn->whichhost].type != CHT_HOST_ADDRESS)
+           conn->connhost[conn->whichhost].host != NULL &&
+           conn->connhost[conn->whichhost].host[0] != '\0')
return conn->connhost[conn->whichhost].host;
-   else if (conn->pghost != NULL && conn->pghost[0] != '\0')
-       return conn->pghost;

Upon further review, the second bit of the patch is making me itching. I
think that you should not remove the second check which returns
conn->pghost if a value is found in it.

Thanks for the review.

Before posting the patch, first I did the same, upon further study
I didn't find any scenario where the value is not present in
conn->connhost[conn->whichhost].host and present in conn->pghost.

If user provides "host" as connection option, the value is present
in both the variables. Even if the connection is unix domain socket,
there is a value in conn->connhost[conn->whichhost].host.

In case if user provides only hostaddr and host connection option,
then in that case, both the members are NULL. So even if we add
that case, it will be dead code.

I agree with your opinion of creating a new thread of this discussion.

Regards,
Hari Babu
Fujitsu Australia

On Fri, Jan 12, 2018 at 03:55:04PM +1100, Haribabu Kommi wrote:

Before posting the patch, first I did the same, upon further study
I didn't find any scenario where the value is not present in
conn->connhost[conn->whichhost].host and present in conn->pghost.

If user provides "host" as connection option, the value is present
in both the variables. Even if the connection is unix domain socket,
there is a value in conn->connhost[conn->whichhost].host.

In case if user provides only hostaddr and host connection option,
then in that case, both the members are NULL. So even if we add
that case, it will be dead code.

Hm. Wouldn't it matter for cases where caller has not yet established a
connection to the server but still calls PQhost to get the host string?
--
Michael