Обсуждение: 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 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 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

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

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

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

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

On Fri, Jan 12, 2018 at 04:32:54PM +1100, Haribabu Kommi wrote:
> On Fri, Jan 12, 2018 at 4:06 PM, Michael Paquier <michael.paquier@gmail.com>
> wrote:
> > 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?
> >
>
> Yes I agree that the above scenario leads to a wrong result with the
> earlier patch,
> Updated patch attached by including the conn->pghost. Thanks for the review.

Could you begin a new thread by the way? As you are the one who
discovered the inconsistency and the one who wrote a patch this looks
adapted to me. Or perhaps I should?
--
Michael

On Tue, Jan 16, 2018 at 05:56:22PM +1100, Haribabu Kommi wrote:
> Without PQhostaddr() function, for the connections where the host is not
> specified, it will be difficult to find out to remote server.

That's true as well, but hostaddr should be used with host only to save
IP lookups... There are recent threads on the matter, like this one:
https://www.postgresql.org/message-id/15728.1493654814%40sss.pgh.pa.us
See particularly the commits cited in this message. PQhostaddr has been
already introduced, and reverted in the tree.

This may lead to some confusion as well. Take for example this
connection string:
'port=6666,5432 hostaddr=127.0.0.1,127.0.0.1 host=/tmp,/tmp'
=# select remote_hostname, remote_hostaddr, remote_port from
   pg_stat_wal_receiver;
 remote_hostname | remote_hostaddr | remote_port
-----------------+-----------------+-------------
 /tmp            | 127.0.0.1       |        5432
(1 row)
The documentation states that in this case the IP connection is used,
though this can be confusing for users to show both.  I'll bet that we
would get complains about that, without at least proper documentation.

So my take would be to really just use PQhost and PQport, as this does
not remove any usefulness of this feature.  If you want to use IP
addresses, there is nothing preventing you to use them in host as well,
and those would show up properly.  The commit fest is coming to an end,
so my recommendation would be to move it on the next CF and get feedback
on https://www.postgresql.org/message-id/CAJrrPGdrC4JTJQ4d7PT1Bi7K8nW91XPMPQ5kJ3GWK3ts%2BW-35g%40mail.gmail.com
before concluding on this feature.  The problem with PQhost and multiple
hosts is quite different than the 1st thread I am referring in this
email, so let's wait and see for Robert's input.

> With the above two new API's we can display either string or individual
> columns representation of remote server.

I like the naming "remote_*" by the way.
--
Michael

On Tue, Jan 30, 2018 at 03:10:12PM +1100, Haribabu Kommi wrote:
> Ok, understood. As the libpq gives preference to hostaddr connection
> parameter than host while connecting. How about going with one column
> "remote_host" that displays either hostaddr(if exists) or hostname. So that
> one column that displays the actual remote host to where it connected?
>
> Note : The one column approach for both host and hostaddr will depend on
> how we go with PQhostaddr() function.

Yeah, we don't want to begin a open battle for that.  Using one column as
a first step would still be useful anyway.  If the discussion about
PQhostaddr() comes to a result at some point, then it could make sense
to integrate that with pg_stat_wal_receiver.  The problem with PQhost
handling strangely multiple host values is inconsistent though.

> OK. I will move the patch to next commitfest.

Thanks.  Let's see what others think on all those threads.
--
Michael

On Wed, Mar 28, 2018 at 11:28:32AM +1100, Haribabu Kommi wrote:
> I updated the pg_stat_wal_receiver patch with the new PQhost() function
> behavior and updated the view with two columns, (remote_server and
> remote_port) instead of three as earlier.
>
> Updated patch attached.

Thanks Hari for the updated patch.  I was looking forward to seeing a
ner version.


+/*
+ * Provides remote sever info.
+ */
Typo here.  This could be more precise, like "Provides information of
remote server this WAL receiver is connected to".

+libpqrcv_get_remoteserver_info(WalReceiverConn *conn, char
**remote_server,
+                     int *remote_port)
+{
+   char *ret = NULL;
+
+   Assert(conn->streamConn != NULL);

Okay.  The connection should be established so normally the results from
PQport and PQhost should not be NULL.  Still I agree that this feels
safer for the long term.

Except for the small typo outlined, the rest of the patch lokks fine to
me.  Most of the work has really happened for PQhost..
--
Michael

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@paquier.xyz>
> wrote:
> Updated patch attached.

Thanks, switched as ready for committer.
--
Michael

On Wed, Mar 28, 2018 at 3:09 PM, Michael Paquier <michael@paquier.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@paquier.xyz>
>> wrote:
>> Updated patch attached.

Thanks for the patch! I'd like to commit this feature for v11.

@@ -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?

+ 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?

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.

Regards,

-- 
Fujii Masao

On Fri, Mar 30, 2018 at 10:52:02AM +1100, Haribabu Kommi wrote:
> On Fri, Mar 30, 2018 at 7:26 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
>> @@ -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.

No problems with those names.

+       ret = PQhost(conn->streamConn);
+       if (ret && (strcmp(ret,"") != 0))
+               *sender_host = pstrdup(ret);
The code tends to use more strlen to check for empty strings,
particularly libpq.  A small nit it is.
--
Michael

On Fri, Mar 30, 2018 at 9:34 AM, Michael Paquier <michael@paquier.xyz> wrote:
> On Fri, Mar 30, 2018 at 10:52:02AM +1100, Haribabu Kommi wrote:
>> On Fri, Mar 30, 2018 at 7:26 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
>>> @@ -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.
>
> No problems with those names.
>
> +       ret = PQhost(conn->streamConn);
> +       if (ret && (strcmp(ret,"") != 0))
> +               *sender_host = pstrdup(ret);
> The code tends to use more strlen to check for empty strings,
> particularly libpq.  A small nit it is.

Ok, updated the patch so strlen is used.

I pushed the patch. Many thanks to Haribabu and Michael!

Regards,

-- 
Fujii Masao