Re: Enhance pg_stat_wal_receiver view to display connected host

On Fri, Mar 30, 2018 at 7:26 AM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:

> On Wed, Mar 28, 2018 at 3:09 PM, Michael Paquier <michael(at)paquier(dot)xyz>
> wrote:
> > On Wed, Mar 28, 2018 at 03:41:33PM +1100, Haribabu Kommi wrote:
> >> On Wed, Mar 28, 2018 at 12:54 PM, Michael Paquier <michael(at)paquier(dot)xyz>
> >> wrote:
> >> Updated patch attached.
>
> Thanks for the patch! I'd like to commit this feature for v11.
>

Thanks for the review.

> @@ -753,4 +753,6 @@ CREATE VIEW pg_stat_wal_receiver AS
> s.latest_end_time,
> s.slot_name,
> + s.remote_server,
> + s.remote_port,
>
> As the column names, aren't sender_host and sender_port more intuitive
> rather than remote_server and remote_port?
>

OK. Changed accordingly.

> + ret = PQhost(conn->streamConn);
> + if (ret)
> + *remote_server = pstrdup(ret);
>
> When the connection has an error, PQhost() and PQport() return an empty
> string.
> In this case, pg_stat_wal_receiver reports an empty string in
> remote_server and
> NULL in remote_port. Which looks inconsistent to me. In that case, both of
> them
> should be reported NULL, I think. So I think that the above "if (ret)"
> condition
> should be "if (ret & strcmp(ret, "") == 0)". Thought?
>

OK. Added a check to verify the returned host value.

> Of course, currently it's basically impossible that PQhost() and PQport()
> return
> an empty string in libpqrcv_get_remoteserver_info() because it's called
> just
> after the replication connection is successfully established. But it's
> better to
> handle also that case for robustness of the code.
>

OK.

Updated patch attached.

Regards,
Hari Babu
Fujitsu Australia