Re: Enhance pg_stat_wal_receiver view to display connected host

[Fujii Masao]

On Fri, Mar 30, 2018 at 9:34 AM, Michael Paquier  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  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

Re: Enhance pg_stat_wal_receiver view to display connected host

[Michael Paquier]

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


signature.asc
Description: PGP signature

Re: Enhance pg_stat_wal_receiver view to display connected host

[Haribabu Kommi]

On Fri, Mar 30, 2018 at 7:26 AM, Fujii Masao  wrote:

> On Wed, Mar 28, 2018 at 3:09 PM, Michael Paquier 
> 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 
> >> 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


pg_stat_wal_receiver-to-display-sender-host-info_v4.patch
Description: Binary data

Re: Enhance pg_stat_wal_receiver view to display connected host

[Fujii Masao]

On Wed, Mar 28, 2018 at 3:09 PM, Michael Paquier  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 
>> 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

Re: Enhance pg_stat_wal_receiver view to display connected host

[Michael Paquier]

On Wed, Mar 28, 2018 at 03:41:33PM +1100, Haribabu Kommi wrote:
> On Wed, Mar 28, 2018 at 12:54 PM, Michael Paquier 
> wrote:
> Updated patch attached.

Thanks, switched as ready for committer.
--
Michael


signature.asc
Description: PGP signature

Re: Enhance pg_stat_wal_receiver view to display connected host

[Haribabu Kommi]

On Wed, Mar 28, 2018 at 12:54 PM, Michael Paquier 
wrote:

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

Thanks for the review.


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

updated as above.

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

OK.

Updated patch attached.

Regards,
Hari Babu
Fujitsu Australia


pg_stat_wal_receiver-to-display-remote-server-info_v3.patch
Description: Binary data

Re: Enhance pg_stat_wal_receiver view to display connected host

[Michael Paquier]

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


signature.asc
Description: PGP signature

Re: Enhance pg_stat_wal_receiver view to display connected host

[Haribabu Kommi]

On Tue, Jan 30, 2018 at 4:02 PM, Michael Paquier 
wrote:

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


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.

Regards,
Hari Babu
Fujitsu Australia


pg_stat_wal_receiver-to-display-remote-server-info_v2.patch
Description: Binary data

Re: Enhance pg_stat_wal_receiver view to display connected host

[Michael Paquier]

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


signature.asc
Description: PGP signature

Re: Enhance pg_stat_wal_receiver view to display connected host

[Haribabu Kommi]

On Mon, Jan 29, 2018 at 7:06 PM, Michael Paquier 
wrote:

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

Thanks for the link, I checked it. The main reason for the revert was
because
of using host to return the hostaddr even though the host is not specified.
But it may be fine to return the hostaddr with a different function.
Following
are sequence of commits that has changed the behavior.

commit 274bb2b3 introduced the support of returning PQhost() function to
return the connected host. (internally hostaddr also)

commit 11003eb5 added a check in the PQhost() function not to return the
host if the connection type is CHT_HOST_ADDRESS.

This is because an earlier change in connectOptions2() function that changes
the host with hostaddr, because of this reason, PQhost() function returns
the hostaddr and it is not expected from PQhost() function when the host is
not specified.

if (conn->pghostaddr != NULL && conn->pghostaddr[0] != '\0')
{
conn->connhost[0].host = strdup(conn->pghostaddr);
if (conn->connhost[0].host == NULL)
goto oom_error;
conn->connhost[0].type = CHT_HOST_ADDRESS;
}


But the above part of the code is changed in commit 7b02ba62
to support "Allow multiple hostaddrs to go with multiple hostnames"
With this commit, the commit  11003eb5 that blocks the host names
that are of  CHT_HOST_ADDRESS connection type is not be required.



> This may lead to some confusion as well. Take for example this
> connection string:
> 'port=,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.
>

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.


> 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/CAJrrPGdrC4JTJQ4d7PT1B
> i7K8nW91XPMPQ5kJ3GWK3ts%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.
>

OK. I will move the patch to next commitfest.

Regards,
Hari Babu
Fujitsu Australia

Re: Enhance pg_stat_wal_receiver view to display connected host

[Michael Paquier]

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


signature.asc
Description: PGP signature

Re: Enhance pg_stat_wal_receiver view to display connected host

[Haribabu Kommi]

On Tue, Jan 16, 2018 at 5:56 PM, Haribabu Kommi 
wrote:

>
> On Tue, Jan 16, 2018 at 2:55 PM, Michael Paquier <
> michael.paqu...@gmail.com> wrote:
>
>>
>> Note that I still find this API confusing, it seems to me that just
>> sorting out the confusion problems with PQhost and then use it would be
>> more simple.
>>
>
> OK, Understood. Even if the confusion problems with PQhost that are
> discussed in [1] are solved, we need two new API's that are required to\
> display the proper remote server details.
>
> PQhostNoDefault - Similar like PQhost but doesn't return default host
> details.
>
> Displaying default value always some confuse even if the user doesn't
> provide
> the host details, so to avoid that confusion, we need this function.
>
> PQhostaddr - Return hostaddr used in the connection.
>
> Without PQhostaddr() function, for the connections where the host is not
> specified, it will be difficult to find out to remote server.
>
> With the above two new API's we can display either string or individual
> columns
> representation of remote server.
>

As I didn't hear objections, I changed the patch as per the above
description
with two new libpq API's and also with three additional columns
"remote_hostname",
"remote_hostaddr" and "remote_port" to the pg_stat_wal_receiver view.

I didn't explicitly add the CONNECTION_BAD, because the added libpq
functions
must return the value even the connection is not established.

Updated patch attached.

Regards,
Hari Babu
Fujitsu Australia


0002-pg_stat_wal_receiver-to-display-connected-host.patch
Description: Binary data


0001-Addition-of-two-new-libpq-API-s.patch
Description: Binary data

Re: Enhance pg_stat_wal_receiver view to display connected host

[Haribabu Kommi]

[ Including Hackers as earlier mail mistakenly removed it ]

On Tue, Jan 16, 2018 at 2:55 PM, Michael Paquier 
wrote:

> On Mon, Jan 15, 2018 at 05:51:58PM +1100, Haribabu Kommi wrote:
> > Instead of effective_conninfo, I changed the column name as
> > remote_serve_info and
> > display only the host, hostaddr and port details. These are the only
> values
> > that differs
> > with each remote connection.
>
> I agree that it is pointless to show duplication between both
> strings. The differences would be made harder to catch.
>
> +PQconninfoOption *
> +PQeffectiveConninfo(PGconn *conn)
> +{
> +   PQExpBufferData errorBuf;
> +   PQconninfoOption *connOptions;
> +
> +   if (conn == NULL)
> +   return NULL;
>
> Shouldn't this check for CONNECTION_BAD as well and return NULL?
>

OK. I will update it.


> If I use something like "port=5432 host=/tmp" as connection string, then
> PQeffectiveConninfo gives me the following with hostaddr being weird:
> host=/tmp hostaddr=(null) port=5432
> This should show an empty hostaddr value.
>

I will correct it.


> + remote_server_info
> + text
> + 
> +  Effective remote server connection info string used by this WAL
> receiver.
> + 
> "Effective" is not a precise term. What about just telling that this is
> the set of parameters used for hte active connection, and that this
> value should be the one used when using multiple host, hostaddr, and
> ports.
>

OK. I will update it.

Note that I still find this API confusing, it seems to me that just
> sorting out the confusion problems with PQhost and then use it would be
> more simple.
>

OK, Understood. Even if the confusion problems with PQhost that are
discussed in [1] are solved, we need two new API's that are required to\
display the proper remote server details.

PQhostNoDefault - Similar like PQhost but doesn't return default host
details.

Displaying default value always some confuse even if the user doesn't
provide
the host details, so to avoid that confusion, we need this function.

PQhostaddr - Return hostaddr used in the connection.

Without PQhostaddr() function, for the connections where the host is not
specified, it will be difficult to find out to remote server.

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

comments?

Regards,
Hari Babu
Fujitsu Australia

Re: Enhance pg_stat_wal_receiver view to display connected host

[Michael Paquier]

On Fri, Jan 12, 2018 at 04:32:54PM +1100, Haribabu Kommi wrote:
> On Fri, Jan 12, 2018 at 4:06 PM, Michael Paquier 
> 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


signature.asc
Description: PGP signature

Re: Enhance pg_stat_wal_receiver view to display connected host

[Haribabu Kommi]

On Fri, Jan 12, 2018 at 4:06 PM, Michael Paquier 
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.

Regards,
Hari Babu
Fujitsu Australia


PQhost-update-to-return-proper-host-details_v2.patch
Description: Binary data

Re: Enhance pg_stat_wal_receiver view to display connected host

[Michael Paquier]

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


signature.asc
Description: PGP signature

Re: Enhance pg_stat_wal_receiver view to display connected host

[Haribabu Kommi]

On Fri, Jan 12, 2018 at 3:26 PM, Michael Paquier 
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

Re: Enhance pg_stat_wal_receiver view to display connected host

[Michael Paquier]

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


signature.asc
Description: PGP signature

Re: Enhance pg_stat_wal_receiver view to display connected host

[Michael Paquier]

On Wed, Jan 10, 2018 at 04:10:35PM +1100, Haribabu Kommi wrote:
> On Tue, Jan 9, 2018 at 12:15 PM, Michael Paquier 
> 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


signature.asc
Description: PGP signature

Re: Enhance pg_stat_wal_receiver view to display connected host

[Haribabu Kommi]

On Tue, Jan 9, 2018 at 12:15 PM, Michael Paquier 
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


PQhost-update-to-return-proper-host-details.patch
Description: Binary data

Re: Enhance pg_stat_wal_receiver view to display connected host

[Michael Paquier]

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


signature.asc
Description: PGP signature

Re: Enhance pg_stat_wal_receiver view to display connected host

[Alvaro Herrera]

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 Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: Enhance pg_stat_wal_receiver view to display connected host

[Haribabu Kommi]

On Fri, Jan 5, 2018 at 12:05 AM, Michael Paquier 
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

Re: Enhance pg_stat_wal_receiver view to display connected host

[Michael Paquier]

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


signature.asc
Description: PGP signature

Re: Enhance pg_stat_wal_receiver view to display connected host

[Michael Paquier]

On Thu, Jan 04, 2018 at 05:21:02PM +1100, Haribabu Kommi wrote:
> On Thu, Jan 4, 2018 at 11:53 AM, Michael Paquier 
> 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.harib...@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]
+ 
+  port number of the PostgreSQL instance
+  this WAL receiver is connected to.
+ 
Missing an upper case at the beginning of the sentence here.
[/nit]
--
Michael


signature.asc
Description: PGP signature

Re: Enhance pg_stat_wal_receiver view to display connected host

[Alvaro Herrera]

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 Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: Enhance pg_stat_wal_receiver view to display connected host

[Michael Paquier]

On Wed, Jan 03, 2018 at 06:48:07PM +1100, Haribabu Kommi wrote:
> On Wed, Jan 3, 2018 at 12:25 PM, Haribabu Kommi 
> 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.

+
+ remote_hostname
+ text
+ Host name of the PostgreSQL instance this WAL receiver is 
connected to
+
PostgreSQL is usualy referred to with the  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


signature.asc
Description: PGP signature

Re: Enhance pg_stat_wal_receiver view to display connected host

[Haribabu Kommi]

On Wed, Jan 3, 2018 at 12:25 PM, Haribabu Kommi 
wrote:
>
>
> update patch attached.
>

Last patch has undefined symbol, corrected patch attached.

Regards,
Hari Babu
Fujitsu Australia


pg_stat_wal_receiver-to-display-connected-host_v3.patch
Description: Binary data

Re: Enhance pg_stat_wal_receiver view to display connected host

[Haribabu Kommi]

On Thu, Dec 21, 2017 at 11:12 PM, Michael Paquier  wrote:

> On Thu, Dec 21, 2017 at 8:16 PM, Haribabu Kommi
>  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


pg_stat_wal_receiver-to-display-connected-host_v1.patch
Description: Binary data

Enhance pg_stat_wal_receiver view to display connected host

[Haribabu Kommi]

Hi Hackers,

With the multi host connection string feature, it is possible to specify
multiple
hosts in primary_conninfo that is used in streaming replication to make sure
that WAL streaming is not affected.

Currently there is no information that is available in the standby node to
find out
to which primary currently it is connected. Need to find out from primary
node only.

I feel it may be better if we can enhance the pg_stat_wal_receiver to
display the
connected host information such as "hostname", "hostaddr" and "port". So
that
it is possible to find out the from standby node to which primary currently
it is
connected.

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?

Regards,
Hari Babu
Fujitsu Australia


pg_stat_wal_receiver-to-display-connected-host.patch
Description: Binary data