Re: system views for walsender activity

Hi,

We don't have any statistic views for walsenders in SR's master server
in 9.0, but such views would be useful to monitor and manage standby
servers from the master server. I have two ideas for the solution -
adding a new system view or recycling pg_stat_activity:

1. Add another system view for walsenders, ex. "pg_stat_replication".
It would show pid, remote host, and sent location for each walsender.

2. Make pg_stat_activity to show walsenders. We could use current_query
to display walsender-specific information, like:
=# SELECT * FROM my_stat_activity ;
-[ RECORD 1 ]----+---------------------------------
datid | 16384
<snip>
current_query | SELECT * FROM my_stat_activity ;
-[ RECORD 2 ]----+---------------------------------
datid | 0
datname |
procpid | 4300
usesysid | 10
usename | itagaki
application_name |
client_addr | ::1
client_port | 37710
backend_start | 2010-06-16 16:47:35.646486+09
xact_start |
query_start |
waiting | f
current_query | walsender: sent=0/701AAA8

The attached patch is a WIP codes for the case 2, but it might be
better to design management policy via SQL in 9.1 before such detailed
implementation. Comments welcome.

Regards,
---
Takahiro Itagaki
NTT Open Source Software Center

On Fri, Jun 18, 2010 at 04:33, Takahiro Itagaki
<itagaki(dot)takahiro(at)oss(dot)ntt(dot)co(dot)jp> wrote:
> Hi,
>
> We don't have any statistic views for walsenders in SR's master server
> in 9.0, but such views would be useful to monitor and manage standby
> servers from the master server. I have two ideas for the solution -
> adding a new system view or recycling pg_stat_activity:
>
> 1. Add another system view for walsenders, ex. "pg_stat_replication".
>   It would show pid, remote host, and sent location for each walsender.
>
> 2. Make pg_stat_activity to show walsenders. We could use current_query
>   to display walsender-specific information, like:
>    =# SELECT * FROM my_stat_activity ;
>    -[ RECORD 1 ]----+---------------------------------
>    datid            | 16384
>    <snip>
>    current_query    | SELECT * FROM my_stat_activity ;
>    -[ RECORD 2 ]----+---------------------------------
>    datid            | 0
>    datname          |
>    procpid          | 4300
>    usesysid         | 10
>    usename          | itagaki
>    application_name |
>    client_addr      | ::1
>    client_port      | 37710
>    backend_start    | 2010-06-16 16:47:35.646486+09
>    xact_start       |
>    query_start      |
>    waiting          | f
>    current_query    | walsender: sent=0/701AAA8
>
> The attached patch is a WIP codes for the case 2, but it might be
> better to design management policy via SQL in 9.1 before such detailed
> implementation.  Comments welcome.

I like version 1 much better. It'll be a lot easier for a management
tool to get the data in proper columns and not have to parse it out of
an arbitrary string field.

The downside is that version 1 will require an initdb, and not version
2, right? In that light, #2 is probably the only one we can do for 9.0
(unless we stumble upon other initdb-forcing changes), so maybe we
should do that one as a temporary measure (and if so, note it both in
the documentation and code that it's a temporary thing).

Wouldn't we also need something similar for the receiving end?

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

On Fri, 2010-06-18 at 11:33 +0900, Takahiro Itagaki wrote:

> 1. Add another system view for walsenders, ex. "pg_stat_replication".
> It would show pid, remote host, and sent location for each walsender.

I prefer this option. I consider it an omission that we should correct.

Not sure I understand why you block up access to the WALSendPtr and then
propose re-accessing it in text form a few days later. Whatever else you
do you should revoke the commit that did that.

--
Simon Riggs www.2ndQuadrant.com

On 18/06/10 13:41, Simon Riggs wrote:
> On Fri, 2010-06-18 at 11:33 +0900, Takahiro Itagaki wrote:
>
>> 1. Add another system view for walsenders, ex. "pg_stat_replication".
>> It would show pid, remote host, and sent location for each walsender.
>
> I prefer this option. I consider it an omission that we should correct.
>
> Not sure I understand why you block up access to the WALSendPtr and then
> propose re-accessing it in text form a few days later. Whatever else you
> do you should revoke the commit that did that.

Are you referring to the dead extern declaration of
GetOldestWALSendPointer()? That was indeed dead, the function itself was
already #ifdeffed out. If we go with the pg_state_replication view as
suggested above, it would've been useless anyway because it returns only
one value, not one for each walsender.

Let's discuss what the best possible user interface for the information
would be first, and then decide if we need/want to force an initdb for
that. We have pg_upgrade now, that makes initdb less painful, and if
it's just a new view it might be possible to just add a note in the
release notes that you'll have to run the CREATE VIEW manually if upgrading.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com> writes:
> Let's discuss what the best possible user interface for the information
> would be first, and then decide if we need/want to force an initdb for
> that. We have pg_upgrade now, that makes initdb less painful, and if
> it's just a new view it might be possible to just add a note in the
> release notes that you'll have to run the CREATE VIEW manually if upgrading.

The view would presumably depend on a C-language SRF, which isn't there
either.

I'm of the opinion that this is a 9.1 problem. It needs more thought
than we can put into it now --- one obvious question is what about
monitoring on the slave side? Another is who should be able to see the
data?

regards, tom lane

Magnus Hagander <magnus(at)hagander(dot)net> wrote:

> The downside is that version 1 will require an initdb, and not version
> 2, right?

Unfortunately, 2 also requires initdb because pg_stat_activity will
use LEFT JOIN instead of normal JOIN not to hide rows with databaseid = 0.
All of them are items for 9.1.

Regards,
---
Takahiro Itagaki
NTT Open Source Software Center

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> I'm of the opinion that this is a 9.1 problem. It needs more thought
> than we can put into it now --- one obvious question is what about
> monitoring on the slave side? Another is who should be able to see the
> data?

Sure. We should research user's demands for monitoring and management
of replication. I'll report some voices from users as of this moment:

* Managers often ask DBAs "How long standby servers are behind the master?"
We should provide such methods for DBAs. We have pg_xlog_location()
functions, but they should be improved for:
- The returned values are "xxx/yyy" texts, but more useful information
is the difference of two values. Subtraction functions are required.
- For easier management, the master server should provide not only
sent/flush locations but also received/replayed locations for each
standby servers. Users don't want to access both master and slaves.

* Some developers want to pause and restart replication from the master
server. They're going to use replication for application version
managements. They'll pause all replications, and test their new features
at the master, and restart replication to spread the changes to slaves.

Regards,
---
Takahiro Itagaki
NTT Open Source Software Center

Le 22/06/2010 06:40, Takahiro Itagaki a écrit :
> [...]
> Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
>> I'm of the opinion that this is a 9.1 problem. It needs more thought
>> than we can put into it now --- one obvious question is what about
>> monitoring on the slave side? Another is who should be able to see the
>> data?
>
> Sure. We should research user's demands for monitoring and management
> of replication. I'll report some voices from users as of this moment:
>
> * Managers often ask DBAs "How long standby servers are behind the master?"
> We should provide such methods for DBAs. We have pg_xlog_location()
> functions, but they should be improved for:
> - The returned values are "xxx/yyy" texts, but more useful information
> is the difference of two values. Subtraction functions are required.
> - For easier management, the master server should provide not only
> sent/flush locations but also received/replayed locations for each
> standby servers. Users don't want to access both master and slaves.
>
> * Some developers want to pause and restart replication from the master
> server. They're going to use replication for application version
> managements. They'll pause all replications, and test their new features
> at the master, and restart replication to spread the changes to slaves.
>

I agree on these two.

Something I found lacking when I added support for Hot Standby /
Streaming Replication in pgAdmin (that was a really small patch, there
was not a lot to do) was that one cannot get the actual value of each
recovery.conf parameter. Try a "SHOW primary_conninfo;" and it will
juste reply that primary_conninfo is an unknown parameter. I already
talked about this to Heikki, but didn't get a chance to actually look at
the code.

--
Guillaume
http://www.postgresql.fr
http://dalibo.com

On Tue, 2010-06-22 at 09:54 +0200, Guillaume Lelarge wrote:
> I added support for Hot Standby /
> Streaming Replication in pgAdmin (that was a really small patch, there
> was not a lot to do)

Well done.

Does this mean that pgAdmin has a read only mode now?

What are the details of that support? I couldn't easily see the commits
in the pgadmin list.

--
Simon Riggs www.2ndQuadrant.com

Le 22/06/2010 11:41, Simon Riggs a écrit :
> On Tue, 2010-06-22 at 09:54 +0200, Guillaume Lelarge wrote:
>> I added support for Hot Standby /
>> Streaming Replication in pgAdmin (that was a really small patch, there
>> was not a lot to do)
>
> Well done.
>
> Does this mean that pgAdmin has a read only mode now?
>

Nope, it does not really have one. Though I intend to work on having
pgAdmin more aware of the actual rights of the connected user (allowing
him to get to display the "create table" dialog when we should already
know he cannot is an issue, at least to me).

> What are the details of that support? I couldn't easily see the commits
> in the pgadmin list.
>

Shamely simple : I only added some informations on the server's
properties. See
http://www.pgadmin.org/images/visualtour12/visualtour08.jpg. We only
display the fact that the server is (or isn't) in recovery, and the
result of the two admin functions (receive and replay location). Too bad
the other admin functions aren't there, I could have used them (and hope
to do so in 9.1). Too bad also we cannot know the primary server from a
connection to the slave (that's why I would love to get the value of
primary_conninfo, to found the alias/IP of the primary server).

--
Guillaume
http://www.postgresql.fr
http://dalibo.com

On Tue, 2010-06-22 at 12:19 +0200, Guillaume Lelarge wrote:
> Shamely simple : I only added some informations on the server's
> properties. See
> http://www.pgadmin.org/images/visualtour12/visualtour08.jpg. We only
> display the fact that the server is (or isn't) in recovery, and the
> result of the two admin functions (receive and replay location).

If you store the is-in-Recovery result you could set the .enabled
property of many of the dialog boxes. I think its going to be painful
for people to attempt to submit a DDL command and get an error.

> Too bad the other admin functions aren't there, I could have used them
> (and hope to do so in 9.1). Too bad also we cannot know the primary
> server from a connection to the slave (that's why I would love to get
> the value of
> primary_conninfo, to found the alias/IP of the primary server).

Agreed

--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Training and Services

Le 22/06/2010 12:42, Simon Riggs a écrit :
> On Tue, 2010-06-22 at 12:19 +0200, Guillaume Lelarge wrote:
>> Shamely simple : I only added some informations on the server's
>> properties. See
>> http://www.pgadmin.org/images/visualtour12/visualtour08.jpg. We only
>> display the fact that the server is (or isn't) in recovery, and the
>> result of the two admin functions (receive and replay location).
>
> If you store the is-in-Recovery result you could set the .enabled
> property of many of the dialog boxes. I think its going to be painful
> for people to attempt to submit a DDL command and get an error.
>

That's what I first thought. But it would be weird that we disabled all
the OK button of the dialog properties only for hotstandby servers, but
not when a user doesn't have the permission. At least, that was the
reasonning I had at the time.

>> Too bad the other admin functions aren't there, I could have used them
>> (and hope to do so in 9.1). Too bad also we cannot know the primary
>> server from a connection to the slave (that's why I would love to get
>> the value of
>> primary_conninfo, to found the alias/IP of the primary server).
>
> Agreed
>

:)

--
Guillaume
http://www.postgresql.fr
http://dalibo.com

On Tue, Jun 22, 2010 at 06:18, Takahiro Itagaki
<itagaki(dot)takahiro(at)oss(dot)ntt(dot)co(dot)jp> wrote:
>
> Magnus Hagander <magnus(at)hagander(dot)net> wrote:
>
>> The downside is that version 1 will require an initdb, and not version
>> 2, right?
>
> Unfortunately, 2 also requires initdb because pg_stat_activity will
> use LEFT JOIN instead of normal JOIN not to hide rows with databaseid = 0.
> All of them are items for 9.1.

Did this one end up on the floor?

We definitely need the very basic level for 9.1, and we can always
improve on it later :-) Do you want to keep working on it, or do you
want me to pick it up?

Any of the suggestions that includes the master showing data from the
slaves requires some kind of feedback going in from the slave, making
things a lot more complex (the slave is no longer passive) - let's
leave those for now. (you can use the 2ndquadrant replmgr to get some
of that :P)

I'm not sure it makes much sense to add walsenders to pg_stat_activity
- a lot of the fields would no longer make any sense (statement start?
query start?) - I think we're better off with a separate view for
pg_stat_walsender. It would then only need the columns for procpid,
usesysid, usename, client_addr, client_port, and the WALsender
specific fields.

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

On Tue, Dec 28, 2010 at 21:46, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
>> Unfortunately, 2 also requires initdb because pg_stat_activity will
>> use LEFT JOIN instead of normal JOIN not to hide rows with databaseid = 0.
>> All of them are items for 9.1.
>
> Did this one end up on the floor?
>
> We definitely need the very basic level for 9.1, and we can always
> improve on it later :-) Do you want to keep working on it, or do you
> want me to pick it up?

OK, I'll work for it.

> I'm not sure it makes much sense to add walsenders to pg_stat_activity
> - a lot of the fields would no longer make any sense (statement start?
> query start?) - I think we're better off with a separate view for
> pg_stat_walsender. It would then only need the columns for procpid,
> usesysid, usename, client_addr, client_port, and the WALsender
> specific fields.

+1 for the separate view. "backend_start" (or replication_start?)
might be also reasonable.

--
Itagaki Takahiro

On Tue, Dec 28, 2010 at 14:14, Itagaki Takahiro
<itagaki(dot)takahiro(at)gmail(dot)com> wrote:
> On Tue, Dec 28, 2010 at 21:46, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
>>> Unfortunately, 2 also requires initdb because pg_stat_activity will
>>> use LEFT JOIN instead of normal JOIN not to hide rows with databaseid = 0.
>>> All of them are items for 9.1.
>>
>> Did this one end up on the floor?
>>
>> We definitely need the very basic level for 9.1, and we can always
>> improve on it later :-) Do you want to keep working on it, or do you
>> want me to pick it up?
>
> OK, I'll work for it.

Great.

>> I'm not sure it makes much sense to add walsenders to pg_stat_activity
>> - a lot of the fields would no longer make any sense (statement start?
>> query start?) - I think we're better off with a separate view for
>> pg_stat_walsender. It would then only need the columns for procpid,
>> usesysid, usename, client_addr, client_port, and the WALsender
>> specific fields.
>
> +1 for the separate view. "backend_start" (or replication_start?)
> might be also reasonable.

Yeah, agreed. backend_start is probably the best one -
replication_start may be considered conceptually different if the
connection was dropped and reconnected. backend_start is more
explicit.

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

On Tue, Dec 28, 2010 at 22:17, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
>>> We definitely need the very basic level for 9.1, and we can always
>>> improve on it later :-)
>
>>> pg_stat_walsender. It would then only need the columns for procpid,
>>> usesysid, usename, client_addr, client_port, and the WALsender
>>> specific fields.
> Yeah, agreed. backend_start is probably the best one

Here are patches for pg_stat_walsender.
I split the feature into two pieces:

* get_host_and_port.patch
It separates host and port formatter as a subroutine from pg_stat_activity.
In addition, make pg_stat_get_backend_client_addr/port() functions to
use the subroutine to reduce duplicated codes.

* pg_stat_walsender.patch
It adds pg_stat_walsender system view. It has subset columns of
pg_stat_activity and only one additional WAL sender specific information
via WALSndStatus(). I named the column "sending location" because
standby servers might not have received the WAL record; if we had
synchronous replication, a new "sent location" wold be added.
But the naming is still an open question. Comments welcome.

There is O(max_wal_senders^2) complexity in the view, But I think
it is not so serious problem because we can expect max_wal_senders
is 10 or so at most.

CREATE VIEW pg_stat_walsender AS
SELECT
S.procpid,
S.usesysid,
U.rolname AS usename,
S.client_addr,
S.client_port,
S.backend_start,
S.xlog_sending
FROM pg_stat_get_walsender(NULL) AS S, pg_authid U
WHERE S.usesysid = U.oid;

--
Itagaki Takahiro

On Tue, 2011-01-04 at 15:51 +0900, Itagaki Takahiro wrote:
> On Tue, Dec 28, 2010 at 22:17, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
> >>> We definitely need the very basic level for 9.1, and we can always
> >>> improve on it later :-)
> >
> >>> pg_stat_walsender. It would then only need the columns for procpid,
> >>> usesysid, usename, client_addr, client_port, and the WALsender
> >>> specific fields.
> > Yeah, agreed. backend_start is probably the best one
>
> Here are patches for pg_stat_walsender.
> I split the feature into two pieces:
>
> * get_host_and_port.patch
> It separates host and port formatter as a subroutine from pg_stat_activity.
> In addition, make pg_stat_get_backend_client_addr/port() functions to
> use the subroutine to reduce duplicated codes.
>
> * pg_stat_walsender.patch
> It adds pg_stat_walsender system view. It has subset columns of
> pg_stat_activity and only one additional WAL sender specific information
> via WALSndStatus(). I named the column "sending location" because
> standby servers might not have received the WAL record; if we had
> synchronous replication, a new "sent location" wold be added.
> But the naming is still an open question. Comments welcome.
>
> There is O(max_wal_senders^2) complexity in the view, But I think
> it is not so serious problem because we can expect max_wal_senders
> is 10 or so at most.
>
> CREATE VIEW pg_stat_walsender AS
> SELECT
> S.procpid,
> S.usesysid,
> U.rolname AS usename,
> S.client_addr,
> S.client_port,
> S.backend_start,
> S.xlog_sending
> FROM pg_stat_get_walsender(NULL) AS S, pg_authid U
> WHERE S.usesysid = U.oid;

Just seen you started working on this again. Very good.

I enclose some snippets of code I was working on, which I am removing
from my patch in favour of your work as a separate commit.

The way I coded it was a new SRF that joins to the existing
pg_stat_activity. So no initdb required, and this can also easily be
included as an external module for 9.0.

Please notice also that my coding of the new SRF does not have the O^2
issue you mention, which I was keen to avoid.

The sent pointer is needed whether or not we have sync rep. We should
also include application name, since the user may set that in the
standby for all the same reasons it is set elsewhere.

Small point: please lets not call this pg_stat_walsender?
pg_stat_replication_sent and pg_stat_replication_received would be
easier for normal humans to understand.

I would very much appreciate it if one of you could complete something
here and commit in the next few days. That would then allow me to extend
the view with sync rep specific info for monitoring and patch testing.

--
Simon Riggs http://www.2ndQuadrant.com/books/
PostgreSQL Development, 24x7 Support, Training and Services

On Tue, Jan 4, 2011 at 12:48 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> The sent pointer is needed whether or not we have sync rep. We should
> also include application name, since the user may set that in the
> standby for all the same reasons it is set elsewhere.
>
> Small point: please lets not call this pg_stat_walsender?
> pg_stat_replication_sent and pg_stat_replication_received would be
> easier for normal humans to understand.

Eh... I may be showing my status as a non-normal human, but I know
exactly what pg_stat_walsender is (it's the view that shows you the
status of the WAL senders you've allowed by configuring
max_wal_senders>0) but I have no idea what pg_stat_replication_sent
and pg_stat_replication_received are supposed to be. Why two views?
*scratches head in confusion*

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

> Eh... I may be showing my status as a non-normal human, but I know
> exactly what pg_stat_walsender is (it's the view that shows you the
> status of the WAL senders you've allowed by configuring
> max_wal_senders>0) but I have no idea what pg_stat_replication_sent
> and pg_stat_replication_received are supposed to be. Why two views?
> *scratches head in confusion*

How about one view, called "pg_stat_replication"? This would be clear
even to newbies, which none of the other view names would ...

--
-- Josh Berkus
PostgreSQL Experts Inc.
http://www.pgexperts.com

On Tue, 2011-01-04 at 10:50 -0800, Josh Berkus wrote:
> > Eh... I may be showing my status as a non-normal human, but I know
> > exactly what pg_stat_walsender is (it's the view that shows you the
> > status of the WAL senders you've allowed by configuring
> > max_wal_senders>0) but I have no idea what pg_stat_replication_sent
> > and pg_stat_replication_received are supposed to be. Why two views?
> > *scratches head in confusion*
>
> How about one view, called "pg_stat_replication"? This would be clear
> even to newbies, which none of the other view names would ...

hmmm I think "pg_stat_standby" might be more relevant but I definitely
agree something more newbie appropriate is in order.

Joshua D. Drake

>
> --
> -- Josh Berkus
> PostgreSQL Experts Inc.
> http://www.pgexperts.com
>

--
PostgreSQL.org Major Contributor
Command Prompt, Inc: http://www.commandprompt.com/ - 509.416.6579
Consulting, Training, Support, Custom Development, Engineering
http://twitter.com/cmdpromptinc | http://identi.ca/commandprompt

> hmmm I think "pg_stat_standby" might be more relevant but I definitely
> agree something more newbie appropriate is in order.

I'd be fine with that name, too.

--
-- Josh Berkus
PostgreSQL Experts Inc.
http://www.pgexperts.com

On Tue, Jan 4, 2011 at 20:28, Josh Berkus <josh(at)agliodbs(dot)com> wrote:
>
>> hmmm I think "pg_stat_standby" might be more relevant but I definitely
>> agree something more newbie appropriate is in order.
>
> I'd be fine with that name, too.

That seems kind of backwards though - given that the view only
contains data on the master...

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

On Tue, Jan 4, 2011 at 2:31 PM, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
> On Tue, Jan 4, 2011 at 20:28, Josh Berkus <josh(at)agliodbs(dot)com> wrote:
>>
>>> hmmm I think "pg_stat_standby" might be more relevant but I definitely
>>> agree something more newbie appropriate is in order.
>>
>> I'd be fine with that name, too.
>
> That seems kind of backwards though - given that the view only
> contains data on the master...

I think pg_stat_replication is better than pg_stat_standby, but I'm
still not convinced we shouldn't go with the obvious
pg_stat_walsenders.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 04.01.2011 21:43, Robert Haas wrote:
> On Tue, Jan 4, 2011 at 2:31 PM, Magnus Hagander<magnus(at)hagander(dot)net> wrote:
>> On Tue, Jan 4, 2011 at 20:28, Josh Berkus<josh(at)agliodbs(dot)com> wrote:
>>>
>>>> hmmm I think "pg_stat_standby" might be more relevant but I definitely
>>>> agree something more newbie appropriate is in order.
>>>
>>> I'd be fine with that name, too.
>>
>> That seems kind of backwards though - given that the view only
>> contains data on the master...
>
> I think pg_stat_replication is better than pg_stat_standby, but I'm
> still not convinced we shouldn't go with the obvious
> pg_stat_walsenders.

How about pg_stat_replication_activity? If I understood correctly, the
view is similar to pg_stat_activity, but displays information about
connected standbys rather than regular backends. It's a bit long name,
though.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

On Tue, Jan 04, 2011 at 10:50:12AM -0800, Josh Berkus wrote:
>
> > Eh... I may be showing my status as a non-normal human, but I know
> > exactly what pg_stat_walsender is (it's the view that shows you the
> > status of the WAL senders you've allowed by configuring
> > max_wal_senders>0) but I have no idea what pg_stat_replication_sent
> > and pg_stat_replication_received are supposed to be. Why two views?
> > *scratches head in confusion*
>
> How about one view, called "pg_stat_replication"? This would be clear
> even to newbies, which none of the other view names would ...

Wait. We can't do that. We'd be breaking a decades-old tradition of
terrible names! ;)

Cheers,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

On Wed, Jan 5, 2011 at 04:56, Heikki Linnakangas
<heikki(dot)linnakangas(at)enterprisedb(dot)com> wrote:
>> I think pg_stat_replication is better than pg_stat_standby, but I'm
>> still not convinced we shouldn't go with the obvious
>> pg_stat_walsenders.
>
> How about pg_stat_replication_activity? If I understood correctly, the view
> is similar to pg_stat_activity, but displays information about connected
> standbys rather than regular backends. It's a bit long name, though.

The view currently discussed is for *master* servers. We might have some
views for replication activity in *standby* servers. So, I'd like to
choose consistent and symmetric names for them -- for example,
pg_stat_replication_master and pg_stat_replication_standby.
I've expected they will be pg_stat_wal_[senders|receivers]
when I was writing the patch, but any other better names welcome.

However, we have "max_wal_senders" GUC parameter. So, users still
need to know what "wal_senders" is.

--
Itagaki Takahiro

On Wed, Jan 5, 2011 at 02:32, Itagaki Takahiro
<itagaki(dot)takahiro(at)gmail(dot)com> wrote:
> On Wed, Jan 5, 2011 at 04:56, Heikki Linnakangas
> <heikki(dot)linnakangas(at)enterprisedb(dot)com> wrote:
>>> I think pg_stat_replication is better than pg_stat_standby, but I'm
>>> still not convinced we shouldn't go with the obvious
>>> pg_stat_walsenders.
>>
>> How about pg_stat_replication_activity? If I understood correctly, the view
>> is similar to pg_stat_activity, but displays information about connected
>> standbys rather than regular backends. It's a bit long name, though.
>
> The view currently discussed is for *master* servers. We might have some
> views for replication activity in *standby* servers. So, I'd like to
> choose consistent and symmetric names for them -- for example,
> pg_stat_replication_master and pg_stat_replication_standby.
> I've expected they will be pg_stat_wal_[senders|receivers]
> when I was writing the patch, but any other better names welcome.
>
> However, we have "max_wal_senders" GUC parameter. So, users still
> need to know what "wal_senders" is.

An example to compare with could be pg_stat_bgwriter - that's also one
the really expects you to know some internals. Now, it so happens that
it's a very *bad* example, since it contains a bunch of information
that's *not* actually about the bgwriter these days :-)

But from that perspective, is it likely to ever contain anyting
*other* than walsender information? Given that it's keyed by the
process id of a walsender, I don't expect it would. Whereas a
pg_stat_replication or such could equally be expected to contain
information about other ways of replication - like the file based
modes or even slony.

+1 for pg_stat_walsender or pg_stat_walsender_activity

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

On Wed, Jan 5, 2011 at 02:48, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> The way I coded it was a new SRF that joins to the existing
> pg_stat_activity. So no initdb required, and this can also easily be
> included as an external module for 9.0.
>
> Please notice also that my coding of the new SRF does not have the O^2
> issue you mention, which I was keen to avoid.

Yeah, using SQL JOIN to avoid O(n^2) is a good idea. My only concern is
that pg_stat_get_activity(NULL) might return rows that are not actually
used. Is it an ignorable overhead?

> We should
> also include application name, since the user may set that in the
> standby for all the same reasons it is set elsewhere.

Ah, we can use application_name to name each wal senders.

> Small point: please lets not call this pg_stat_walsender?
> pg_stat_replication_sent and pg_stat_replication_received would be
> easier for normal humans to understand.

A list of proposed view names for replication master server:
- pg_stat_replication
- pg_stat_replication_sent
- pg_stat_standby
- pg_stat_walsender
- pg_stat_walsender_activity

We have some functions for standby server activity
(pg_last_xlog_[receive|replay]_[location|timestamp])
but could have a view for them:
- pg_stat_replication_received
- pg_stat_walreceiver

"pg_stat_replication" and "pg_stat_standby" might not be good names
when we have a view for standby server because the names are not
clear for master server. But if we will have a view only on master,
"pg_stat_replication" seems to be the most understandable name.

> I would very much appreciate it if one of you could complete something
> here and commit in the next few days. That would then allow me to extend
> the view with sync rep specific info for monitoring and patch testing.

What will we name xlog locations that have been received? We call
xlog locations sent to standby as "sentPtr". If we have sync rep,
we will have two locations for each wal sender. For example,
we can call them "sent_location" and "sync_location".

--
Itagaki Takahiro

On Fri, 2011-01-07 at 12:13 +0900, Itagaki Takahiro wrote:

> "pg_stat_replication" seems to be the most understandable name.
>
> > I would very much appreciate it if one of you could complete something
> > here and commit in the next few days. That would then allow me to extend
> > the view with sync rep specific info for monitoring and patch testing.

Please go with whatever you think best for now. I'm sure people will ask
for different names later anyway. Let's get this committed soon, to
reduce later patch conflicts. Thanks.

> What will we name xlog locations that have been received? We call
> xlog locations sent to standby as "sentPtr". If we have sync rep,
> we will have two locations for each wal sender. For example,
> we can call them "sent_location" and "sync_location".

Please add sent_location, I will add others.

--
Simon Riggs http://www.2ndQuadrant.com/books/
PostgreSQL Development, 24x7 Support, Training and Services

On Fri, Jan 7, 2011 at 19:20, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>> "pg_stat_replication" seems to be the most understandable name.
>
> Please go with whatever you think best for now. I'm sure people will ask
> for different names later anyway. Let's get this committed soon, to
> reduce later patch conflicts. Thanks.
>
> Please add sent_location, I will add others.

OK, I added a view named s "pg_stat_replication". The view is basically
based on Simon's patch, but I just skipped unused WalSnd entreis in
WalSndCtl rather than return NULLs. The applied patch attached.

I expect we will have two views for master and standby servers:

* pg_stat_replication
Activity of wal senders in master server.
* pg_stat_standby (not yet)
Activity of a wal receiver and a recovery process in standby servers.

I didn't use pg_stat_wal_sender/receiver as their names because standby
activity in slaves could contain not only a wal receiver but also a
recovery process.

--
Itagaki Takahiro

On Fri, Jan 7, 2011 at 12:42, Itagaki Takahiro
<itagaki(dot)takahiro(at)gmail(dot)com> wrote:
> On Fri, Jan 7, 2011 at 19:20, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>>> "pg_stat_replication" seems to be the most understandable name.
>>
>> Please go with whatever you think best for now. I'm sure people will ask
>> for different names later anyway. Let's get this committed soon, to
>> reduce later patch conflicts. Thanks.
>>
>> Please add sent_location, I will add others.
>
> OK, I added a view named s "pg_stat_replication". The view is basically
> based on Simon's patch, but I just skipped unused WalSnd entreis in
> WalSndCtl rather than return NULLs. The applied patch attached.
>
> I expect we will have two views for master and standby servers:
>
>  * pg_stat_replication
>       Activity of wal senders in master server.
>  * pg_stat_standby (not yet)
>       Activity of a wal receiver and a recovery process in standby servers.

Just to keep the bikeshedding up, should it in this case not be
pg_stat_replication_master and pg_stat_replication_standby or such?
Replication applies to both master and slave...

> I didn't use pg_stat_wal_sender/receiver as their names because standby
> activity in slaves could contain not only a wal receiver but also a
> recovery process.

Good point.

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

On Fri, Jan 7, 2011 at 21:48, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
>>  * pg_stat_replication
>>  * pg_stat_standby (not yet)
>
> Just to keep the bikeshedding up, should it in this case not be
> pg_stat_replication_master and pg_stat_replication_standby or such?
> Replication applies to both master and slave...

The reason I didn't use term "master" is that pg_stat_replication is
information of *standby* servers on master server. Of course,
wal senders are processes in the master, but users probably think
they are the location standby servers receives.

I forgot to update SGML for the view. I'll do it soon.
Thanks for the heads-up.

--
Itagaki Takahiro

On Fri, Jan 7, 2011 at 8:09 AM, Itagaki Takahiro
<itagaki(dot)takahiro(at)gmail(dot)com> wrote:
> On Fri, Jan 7, 2011 at 21:48, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
>>>  * pg_stat_replication
>>>  * pg_stat_standby (not yet)
>>
>> Just to keep the bikeshedding up, should it in this case not be
>> pg_stat_replication_master and pg_stat_replication_standby or such?
>> Replication applies to both master and slave...
>
> The reason I didn't use term "master" is that pg_stat_replication is
> information of *standby* servers on master server. Of course,
> wal senders are processes in the master, but users probably think
> they are the location standby servers receives.

To my way of thinking, pg_stat_walsender and pg_stat_walreceiver would
be more clear than pg_stat_replication_master and
pg_stat_replication_slave.

However, my way of thinking is of course not the only way of thinking.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

> To my way of thinking, pg_stat_walsender and pg_stat_walreceiver would
> be more clear than pg_stat_replication_master and
> pg_stat_replication_slave.

Let's commit it so that some of us can get a look at the data it
contains, and then we can fix the name during beta.

--
-- Josh Berkus
PostgreSQL Experts Inc.
http://www.pgexperts.com

On Fri, Jan 7, 2011 at 19:46, Josh Berkus <josh(at)agliodbs(dot)com> wrote:
>
>> To my way of thinking, pg_stat_walsender and pg_stat_walreceiver would
>> be more clear than pg_stat_replication_master and
>> pg_stat_replication_slave.
>
> Let's commit it so that some of us can get a look at the data it
> contains, and then we can fix the name during beta.

We try to avoid inidb-requiring changes (like renaming a system
object...) during beta.

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

On Fri, 2011-01-07 at 19:48 +0100, Magnus Hagander wrote:
> On Fri, Jan 7, 2011 at 19:46, Josh Berkus <josh(at)agliodbs(dot)com> wrote:
> >
> >> To my way of thinking, pg_stat_walsender and pg_stat_walreceiver would
> >> be more clear than pg_stat_replication_master and
> >> pg_stat_replication_slave.
> >
> > Let's commit it so that some of us can get a look at the data it
> > contains, and then we can fix the name during beta.
>
> We try to avoid inidb-requiring changes (like renaming a system
> object...) during beta.

Why?

--
Simon Riggs http://www.2ndQuadrant.com/books/
PostgreSQL Development, 24x7 Support, Training and Services

Simon Riggs <simon(at)2ndQuadrant(dot)com> writes:
> On Fri, 2011-01-07 at 19:48 +0100, Magnus Hagander wrote:
>> We try to avoid inidb-requiring changes (like renaming a system
>> object...) during beta.

> Why?

So that beta testers won't be forced to do a dump and reload.

When and if pg_upgrade is actually 100% trustworthy, maybe the desire
to avoid initdb's after beta1 will vanish completely; but for now it's
still a good thing to avoid.

regards, tom lane

On Fri, Jan 7, 2011 at 20:51, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Simon Riggs <simon(at)2ndQuadrant(dot)com> writes:
>> On Fri, 2011-01-07 at 19:48 +0100, Magnus Hagander wrote:
>>> We try to avoid inidb-requiring changes (like renaming a system
>>> object...) during beta.
>
>> Why?
>
> So that beta testers won't be forced to do a dump and reload.
>
> When and if pg_upgrade is actually 100% trustworthy, maybe the desire
> to avoid initdb's after beta1 will vanish completely; but for now it's
> still a good thing to avoid.

I think it's still a good thing to avoid. It's at that point no longer
as necessary, but it's still best if we can avoid to make those
changes after beta when possible.

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

On Fri, 2011-01-07 at 14:51 -0500, Tom Lane wrote:
> Simon Riggs <simon(at)2ndQuadrant(dot)com> writes:
> > On Fri, 2011-01-07 at 19:48 +0100, Magnus Hagander wrote:
> >> We try to avoid inidb-requiring changes (like renaming a system
> >> object...) during beta.
>
> > Why?
>
> So that beta testers won't be forced to do a dump and reload.
>
> When and if pg_upgrade is actually 100% trustworthy, maybe the desire
> to avoid initdb's after beta1 will vanish completely; but for now it's
> still a good thing to avoid.

Then before we go beta, we should confirm the names.

IIRC that is not until at least 15 Feb, perhaps 15 Mar.

ISTM there will be much discussion on a range of things, just as there
was last year and no doubt will be every year.

--
Simon Riggs http://www.2ndQuadrant.com/books/
PostgreSQL Development, 24x7 Support, Training and Services

On Fri, Jan 7, 2011 at 1:46 PM, Josh Berkus <josh(at)agliodbs(dot)com> wrote:
>
>> To my way of thinking, pg_stat_walsender and pg_stat_walreceiver would
>> be more clear than pg_stat_replication_master and
>> pg_stat_replication_slave.
>
> Let's commit it so that some of us can get a look at the data it
> contains, and then we can fix the name during beta.

Well, the first half is committed, under the name pg_stat_replication.
So go look at that, for starters...

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Fri, Jan 7, 2011 at 22:21, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Fri, Jan 7, 2011 at 1:46 PM, Josh Berkus <josh(at)agliodbs(dot)com> wrote:
>>
>>> To my way of thinking, pg_stat_walsender and pg_stat_walreceiver would
>>> be more clear than pg_stat_replication_master and
>>> pg_stat_replication_slave.
>>
>> Let's commit it so that some of us can get a look at the data it
>> contains, and then we can fix the name during beta.
>
> Well, the first half is committed, under the name pg_stat_replication.
>  So go look at that, for starters...

One thing I noticed is that it gives an interesting property to my
patch for streaming base backups - they now show up in
pg_stat_replication, with a streaming location of 0/0.

If the view is named pg_stat_replication, we probably want to filter
that out somehow. But then, do we want a separate view listing the
walsenders that are busy sending base backups?

For that matter, do we want an indication that separates a walsender
not sending data from one sending that happens to be at location 0/0?
Most will leave 0/0 really quickly, but a walsender can be idle (not
received a command yet), or it can be running IDENTIFY_SYSTEM for
example.

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

On Sun, 2011-01-09 at 12:52 +0100, Magnus Hagander wrote:

> One thing I noticed is that it gives an interesting property to my
> patch for streaming base backups - they now show up in
> pg_stat_replication, with a streaming location of 0/0.
>
> If the view is named pg_stat_replication, we probably want to filter
> that out somehow. But then, do we want a separate view listing the
> walsenders that are busy sending base backups?
>
> For that matter, do we want an indication that separates a walsender
> not sending data from one sending that happens to be at location 0/0?
> Most will leave 0/0 really quickly, but a walsender can be idle (not
> received a command yet), or it can be running IDENTIFY_SYSTEM for
> example.

I think we need a status enum. ('BACKUP', 'CATCHUP', 'STREAM') for the 3
phases of replication.

--
Simon Riggs http://www.2ndQuadrant.com/books/
PostgreSQL Development, 24x7 Support, Training and Services

On Sun, Jan 9, 2011 at 15:53, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> On Sun, 2011-01-09 at 12:52 +0100, Magnus Hagander wrote:
>
>> One thing I noticed is that it gives an interesting property to my
>> patch for streaming base backups - they now show up in
>> pg_stat_replication, with a streaming location of 0/0.
>>
>> If the view is named pg_stat_replication, we probably want to filter
>> that out somehow. But then, do we want a separate view listing the
>> walsenders that are busy sending base backups?
>>
>> For that matter, do we want an indication that separates a walsender
>> not sending data from one sending that happens to be at location 0/0?
>> Most will leave 0/0 really quickly, but a walsender can be idle (not
>> received a command yet), or it can be running IDENTIFY_SYSTEM for
>> example.
>
> I think we need a status enum. ('BACKUP', 'CATCHUP', 'STREAM') for the 3
> phases of replication.

That seems reasonable. But if we keep BACKUP in there, should we
really have it called pg_stat_replication? (yeah, I know, I'm not
giving up :P)

(You'd need a 4th mode for WAITING or so, to indicate it's waiting for
a command)

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

On Mon, 2011-01-10 at 15:20 +0100, Magnus Hagander wrote:
> On Sun, Jan 9, 2011 at 15:53, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> > On Sun, 2011-01-09 at 12:52 +0100, Magnus Hagander wrote:
> >
> >> One thing I noticed is that it gives an interesting property to my
> >> patch for streaming base backups - they now show up in
> >> pg_stat_replication, with a streaming location of 0/0.
> >>
> >> If the view is named pg_stat_replication, we probably want to filter
> >> that out somehow. But then, do we want a separate view listing the
> >> walsenders that are busy sending base backups?
> >>
> >> For that matter, do we want an indication that separates a walsender
> >> not sending data from one sending that happens to be at location 0/0?
> >> Most will leave 0/0 really quickly, but a walsender can be idle (not
> >> received a command yet), or it can be running IDENTIFY_SYSTEM for
> >> example.
> >
> > I think we need a status enum. ('BACKUP', 'CATCHUP', 'STREAM') for the 3
> > phases of replication.
>
> That seems reasonable. But if we keep BACKUP in there, should we
> really have it called pg_stat_replication? (yeah, I know, I'm not
> giving up :P)
>
> (You'd need a 4th mode for WAITING or so, to indicate it's waiting for
> a command)

That's something different.

The 3 phases are more concrete.

BACKUP --> CATCHUP <---> STREAM

When you connect you either do BACKUP or CATCHUP. Once in CATCHUP mode
you never issue a BACKUP. Once we have caught up we move to STREAM. That
has nothing to do with idle/active.

--
Simon Riggs http://www.2ndQuadrant.com/books/
PostgreSQL Development, 24x7 Support, Training and Services

On 10.01.2011 16:49, Simon Riggs wrote:
> On Mon, 2011-01-10 at 15:20 +0100, Magnus Hagander wrote:
>> On Sun, Jan 9, 2011 at 15:53, Simon Riggs<simon(at)2ndquadrant(dot)com> wrote:
>>> On Sun, 2011-01-09 at 12:52 +0100, Magnus Hagander wrote:
>>>
>>>> One thing I noticed is that it gives an interesting property to my
>>>> patch for streaming base backups - they now show up in
>>>> pg_stat_replication, with a streaming location of 0/0.
>>>>
>>>> If the view is named pg_stat_replication, we probably want to filter
>>>> that out somehow. But then, do we want a separate view listing the
>>>> walsenders that are busy sending base backups?
>>>>
>>>> For that matter, do we want an indication that separates a walsender
>>>> not sending data from one sending that happens to be at location 0/0?
>>>> Most will leave 0/0 really quickly, but a walsender can be idle (not
>>>> received a command yet), or it can be running IDENTIFY_SYSTEM for
>>>> example.
>>>
>>> I think we need a status enum. ('BACKUP', 'CATCHUP', 'STREAM') for the 3
>>> phases of replication.
>>
>> That seems reasonable. But if we keep BACKUP in there, should we
>> really have it called pg_stat_replication? (yeah, I know, I'm not
>> giving up :P)
>>
>> (You'd need a 4th mode for WAITING or so, to indicate it's waiting for
>> a command)
>
> That's something different.
>
> The 3 phases are more concrete.
>
> BACKUP --> CATCHUP<---> STREAM
>
> When you connect you either do BACKUP or CATCHUP. Once in CATCHUP mode
> you never issue a BACKUP. Once we have caught up we move to STREAM. That
> has nothing to do with idle/active.

So how does a walsender that's waiting for a command from the client
show up? Surely it's not in "catchup" mode yet?

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

On Mon, 2011-01-10 at 17:05 +0200, Heikki Linnakangas wrote:
> On 10.01.2011 16:49, Simon Riggs wrote:
> > On Mon, 2011-01-10 at 15:20 +0100, Magnus Hagander wrote:
> >> On Sun, Jan 9, 2011 at 15:53, Simon Riggs<simon(at)2ndquadrant(dot)com> wrote:
> >>> On Sun, 2011-01-09 at 12:52 +0100, Magnus Hagander wrote:
> >>>
> >>>> One thing I noticed is that it gives an interesting property to my
> >>>> patch for streaming base backups - they now show up in
> >>>> pg_stat_replication, with a streaming location of 0/0.
> >>>>
> >>>> If the view is named pg_stat_replication, we probably want to filter
> >>>> that out somehow. But then, do we want a separate view listing the
> >>>> walsenders that are busy sending base backups?
> >>>>
> >>>> For that matter, do we want an indication that separates a walsender
> >>>> not sending data from one sending that happens to be at location 0/0?
> >>>> Most will leave 0/0 really quickly, but a walsender can be idle (not
> >>>> received a command yet), or it can be running IDENTIFY_SYSTEM for
> >>>> example.
> >>>
> >>> I think we need a status enum. ('BACKUP', 'CATCHUP', 'STREAM') for the 3
> >>> phases of replication.
> >>
> >> That seems reasonable. But if we keep BACKUP in there, should we
> >> really have it called pg_stat_replication? (yeah, I know, I'm not
> >> giving up :P)
> >>
> >> (You'd need a 4th mode for WAITING or so, to indicate it's waiting for
> >> a command)
> >
> > That's something different.
> >
> > The 3 phases are more concrete.
> >
> > BACKUP --> CATCHUP<---> STREAM
> >
> > When you connect you either do BACKUP or CATCHUP. Once in CATCHUP mode
> > you never issue a BACKUP. Once we have caught up we move to STREAM. That
> > has nothing to do with idle/active.
>
> So how does a walsender that's waiting for a command from the client
> show up? Surely it's not in "catchup" mode yet?

There is a trivial state between connect and first command. If you think
that is worth publishing, feel free. STARTING?

--
Simon Riggs http://www.2ndQuadrant.com/books/
PostgreSQL Development, 24x7 Support, Training and Services

Em 10-01-2011 12:05, Heikki Linnakangas escreveu:
> So how does a walsender that's waiting for a command from the client
> show up? Surely it's not in "catchup" mode yet?
>
It is kind of "initializing catchup". I think it is not worth representing
those short lifespan states (in normal scenarios).

--
Euler Taveira de Oliveira
http://www.timbira.com/

On Mon, Jan 10, 2011 at 16:41, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> On Mon, 2011-01-10 at 17:05 +0200, Heikki Linnakangas wrote:
>> On 10.01.2011 16:49, Simon Riggs wrote:
>> > On Mon, 2011-01-10 at 15:20 +0100, Magnus Hagander wrote:
>> >> On Sun, Jan 9, 2011 at 15:53, Simon Riggs<simon(at)2ndquadrant(dot)com>  wrote:
>> >>> On Sun, 2011-01-09 at 12:52 +0100, Magnus Hagander wrote:
>> >>>
>> >>>> One thing I noticed is that it gives an interesting property to my
>> >>>> patch for streaming base backups - they now show up in
>> >>>> pg_stat_replication, with a streaming location of 0/0.
>> >>>>
>> >>>> If the view is named pg_stat_replication, we probably want to filter
>> >>>> that out somehow. But then, do we want a separate view listing the
>> >>>> walsenders that are busy sending base backups?
>> >>>>
>> >>>> For that matter, do we want an indication that separates a walsender
>> >>>> not sending data from one sending that happens to be at location 0/0?
>> >>>> Most will leave 0/0 really quickly, but a walsender can be idle (not
>> >>>> received a command yet), or it can be running IDENTIFY_SYSTEM for
>> >>>> example.
>> >>>
>> >>> I think we need a status enum. ('BACKUP', 'CATCHUP', 'STREAM') for the 3
>> >>> phases of replication.
>> >>
>> >> That seems reasonable. But if we keep BACKUP in there, should we
>> >> really have it called pg_stat_replication? (yeah, I know, I'm not
>> >> giving up :P)
>> >>
>> >> (You'd need a 4th mode for WAITING or so, to indicate it's waiting for
>> >> a command)
>> >
>> > That's something different.
>> >
>> > The 3 phases are more concrete.
>> >
>> > BACKUP -->  CATCHUP<--->  STREAM
>> >
>> > When you connect you either do BACKUP or CATCHUP. Once in CATCHUP mode
>> > you never issue a BACKUP. Once we have caught up we move to STREAM. That
>> > has nothing to do with idle/active.
>>
>> So how does a walsender that's waiting for a command from the client
>> show up? Surely it's not in "catchup" mode yet?
>
> There is a trivial state between connect and first command. If you think
> that is worth publishing, feel free. STARTING?

If we don't publish it, it'll implicitly be in one of the others..
Unless we say NULL, of course, but I definitely prefer STARTING then.

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

On Mon, Jan 10, 2011 at 16:48, Euler Taveira de Oliveira
<euler(at)timbira(dot)com> wrote:
> Em 10-01-2011 12:05, Heikki Linnakangas escreveu:
>>
>> So how does a walsender that's waiting for a command from the client
>> show up? Surely it's not in "catchup" mode yet?
>>
> It is kind of "initializing catchup". I think it is not worth representing
> those short lifespan states (in normal scenarios).

True, but it's quite important to detect and diagnose the abnormal ones...

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

On Mon, Jan 10, 2011 at 10:41 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>> >>> I think we need a status enum. ('BACKUP', 'CATCHUP', 'STREAM') for the 3
>> >>> phases of replication.
>> >>
>> >> That seems reasonable. But if we keep BACKUP in there, should we
>> >> really have it called pg_stat_replication? (yeah, I know, I'm not
>> >> giving up :P)
>> >>
>> >> (You'd need a 4th mode for WAITING or so, to indicate it's waiting for
>> >> a command)
>> >
>> > That's something different.
>> >
>> > The 3 phases are more concrete.
>> >
>> > BACKUP -->  CATCHUP<--->  STREAM
>> >
>> > When you connect you either do BACKUP or CATCHUP. Once in CATCHUP mode
>> > you never issue a BACKUP. Once we have caught up we move to STREAM. That
>> > has nothing to do with idle/active.
>>
>> So how does a walsender that's waiting for a command from the client
>> show up? Surely it's not in "catchup" mode yet?
>
> There is a trivial state between connect and first command. If you think
> that is worth publishing, feel free. STARTING?

I think it's worth publishing. STARTING would be OK, or maybe STARTUP
to parallel the other two -UP states.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Tue, Jan 11, 2011 at 02:24, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Mon, Jan 10, 2011 at 10:41 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>>> >>> I think we need a status enum. ('BACKUP', 'CATCHUP', 'STREAM') for the 3
>>> >>> phases of replication.
>>> >>
>>> >> That seems reasonable. But if we keep BACKUP in there, should we
>>> >> really have it called pg_stat_replication? (yeah, I know, I'm not
>>> >> giving up :P)
>>> >>
>>> >> (You'd need a 4th mode for WAITING or so, to indicate it's waiting for
>>> >> a command)
>>> >
>>> > That's something different.
>>> >
>>> > The 3 phases are more concrete.
>>> >
>>> > BACKUP -->  CATCHUP<--->  STREAM
>>> >
>>> > When you connect you either do BACKUP or CATCHUP. Once in CATCHUP mode
>>> > you never issue a BACKUP. Once we have caught up we move to STREAM. That
>>> > has nothing to do with idle/active.
>>>
>>> So how does a walsender that's waiting for a command from the client
>>> show up? Surely it's not in "catchup" mode yet?
>>
>> There is a trivial state between connect and first command. If you think
>> that is worth publishing, feel free. STARTING?
>
> I think it's worth publishing.  STARTING would be OK, or maybe STARTUP
> to parallel the other two -UP states.

Here's a patch for this. I chose IDLE, because that's what we call
other backends that are waiting for commands...

Does this seem correct?

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

On Tue, Jan 11, 2011 at 5:28 AM, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
> Does this seem correct?

It looks reasonable, except that I the way you've chosen to capitalize
the wal sender states makes me want to shoot myself.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Tue, 2011-01-11 at 11:28 +0100, Magnus Hagander wrote:
> >>> >>
> >>> >> (You'd need a 4th mode for WAITING or so, to indicate it's waiting for
> >>> >> a command)
> >>> >
> >>> > That's something different.
> >>> >
> >>> > The 3 phases are more concrete.
> >>> >
> >>> > BACKUP --> CATCHUP<---> STREAM
> >>> >
> >>> > When you connect you either do BACKUP or CATCHUP. Once in CATCHUP mode
> >>> > you never issue a BACKUP. Once we have caught up we move to STREAM. That
> >>> > has nothing to do with idle/active.
> >>>
> >>> So how does a walsender that's waiting for a command from the client
> >>> show up? Surely it's not in "catchup" mode yet?
> >>
> >> There is a trivial state between connect and first command. If you think
> >> that is worth publishing, feel free. STARTING?
> >
> > I think it's worth publishing. STARTING would be OK, or maybe STARTUP
> > to parallel the other two -UP states.
>
> Here's a patch for this. I chose IDLE, because that's what we call
> other backends that are waiting for commands...
>
> Does this seem correct?

No

You can be "idle" yet in STREAMING mode. What mode we are in has nothing
to do with idle/active. Either STARTING/STARTUP/NULL but not IDLE.

If you want that as well, then we need a second column, but personally
it's too much information and its too hard to say what it actually
means. For example, with sync rep, the WALSender might be idle, yet
there might yet be backends waiting for a reply.

--
Simon Riggs http://www.2ndQuadrant.com/books/
PostgreSQL Development, 24x7 Support, Training and Services

On Tue, Jan 11, 2011 at 12:17, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Tue, Jan 11, 2011 at 5:28 AM, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
>> Does this seem correct?
>
> It looks reasonable, except that I the way you've chosen to capitalize
> the wal sender states makes me want to shoot myself.

Hah, I was waiting for that. I can certainly change that :-)

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

On Tue, Jan 11, 2011 at 12:23, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> On Tue, 2011-01-11 at 11:28 +0100, Magnus Hagander wrote:
>> >>> >>
>> >>> >> (You'd need a 4th mode for WAITING or so, to indicate it's waiting for
>> >>> >> a command)
>> >>> >
>> >>> > That's something different.
>> >>> >
>> >>> > The 3 phases are more concrete.
>> >>> >
>> >>> > BACKUP -->  CATCHUP<--->  STREAM
>> >>> >
>> >>> > When you connect you either do BACKUP or CATCHUP. Once in CATCHUP mode
>> >>> > you never issue a BACKUP. Once we have caught up we move to STREAM. That
>> >>> > has nothing to do with idle/active.
>> >>>
>> >>> So how does a walsender that's waiting for a command from the client
>> >>> show up? Surely it's not in "catchup" mode yet?
>> >>
>> >> There is a trivial state between connect and first command. If you think
>> >> that is worth publishing, feel free. STARTING?
>> >
>> > I think it's worth publishing.  STARTING would be OK, or maybe STARTUP
>> > to parallel the other two -UP states.
>>
>> Here's a patch for this. I chose IDLE, because that's what we call
>> other backends that are waiting for commands...
>>
>> Does this seem correct?
>
> No
>
> You can be "idle" yet in STREAMING mode. What mode we are in has nothing
> to do with idle/active. Either STARTING/STARTUP/NULL but not IDLE.
>
> If you want that as well, then we need a second column, but personally
> it's too much information and its too hard to say what it actually
> means. For example, with sync rep, the WALSender might be idle, yet
> there might yet be backends waiting for a reply.

That's a good point.

Just to be clear, you're objecting to the *name* of the state, right,
not how/where it's set? In particular, how the catchup/streaming
things are set?

I agree that using a second column for it is unnecessary.

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

On Tue, 2011-01-11 at 12:41 +0100, Magnus Hagander wrote:

> Just to be clear, you're objecting to the *name* of the state, right,
> not how/where it's set?

Yes

> In particular, how the catchup/streaming
> things are set?

You've set it in the right places.

I would personally constrain the state transitions, so that we can't
ever make illegal changes, such as CATCHUP -> BACKUP.

I would also check the state locally before grabbing the spinlock, as is
typical in other xlog code. Continually resetting shared state to
exactly what it is already seems strange, to me. If we make the rule
that the state can only be changed by the WALSender itself, it won't
need to grab spinlock. If you don't like that, a local variable works.

Also, mixing upper camel case and uppercase for the STATe NamES looKS
weIRD. Uppercase makes them look more clearly like enum/states as used
elsewhere in similar code.

--
Simon Riggs http://www.2ndQuadrant.com/books/
PostgreSQL Development, 24x7 Support, Training and Services

On Tue, Jan 11, 2011 at 12:58, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> On Tue, 2011-01-11 at 12:41 +0100, Magnus Hagander wrote:
>
>> Just to be clear, you're objecting to the *name* of the state, right,
>> not how/where it's set?
>
> Yes
>
>> In particular, how the catchup/streaming
>> things are set?
>
> You've set it in the right places.
>
> I would personally constrain the state transitions, so that we can't
> ever make illegal changes, such as CATCHUP -> BACKUP.

Well, once we enter the walsender loop we can never get out of it, so
it simply cannot happen...

> I would also check the state locally before grabbing the spinlock, as is
> typical in other xlog code. Continually resetting shared state to
> exactly what it is already seems strange, to me. If we make the rule
> that the state can only be changed by the WALSender itself, it won't
> need to grab spinlock. If you don't like that, a local variable works.

Hmm. I don't see why anybody other than the walsender should change
it, so yeah. You mean I can just drop the spinlock calls completely,
and then do an
if (walsnd->state != state)
walsnd->state = state;

? Do I need to keep the volatile pointer, or should I drop that as well?

> Also, mixing upper camel case and uppercase for the STATe NamES looKS
> weIRD. Uppercase makes them look more clearly like enum/states as used
> elsewhere in similar code.

Yeah, I'll change that.

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

On Tue, 2011-01-11 at 13:04 +0100, Magnus Hagander wrote:
> On Tue, Jan 11, 2011 at 12:58, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> > On Tue, 2011-01-11 at 12:41 +0100, Magnus Hagander wrote:
> >
> >> Just to be clear, you're objecting to the *name* of the state, right,
> >> not how/where it's set?
> >
> > Yes
> >
> >> In particular, how the catchup/streaming
> >> things are set?
> >
> > You've set it in the right places.
> >
> > I would personally constrain the state transitions, so that we can't
> > ever make illegal changes, such as CATCHUP -> BACKUP.
>
> Well, once we enter the walsender loop we can never get out of it, so
> it simply cannot happen...

Accidentally it can, so I would like to protect against that.

Putting those checks in are like Asserts(), they help document what is
and is not possible by design.

> > I would also check the state locally before grabbing the spinlock, as is
> > typical in other xlog code. Continually resetting shared state to
> > exactly what it is already seems strange, to me. If we make the rule
> > that the state can only be changed by the WALSender itself, it won't
> > need to grab spinlock. If you don't like that, a local variable works.
>
> Hmm. I don't see why anybody other than the walsender should change
> it, so yeah. You mean I can just drop the spinlock calls completely,
> and then do an
> if (walsnd->state != state)
> walsnd->state = state;
>
>
> ? Do I need to keep the volatile pointer, or should I drop that as well?

No, do this at top

if (walsnd->state == state)
return;

Keep spinlocks when actually setting it.

> > Also, mixing upper camel case and uppercase for the STATe NamES looKS
> > weIRD. Uppercase makes them look more clearly like enum/states as used
> > elsewhere in similar code.
>
> Yeah, I'll change that.
>

--
Simon Riggs http://www.2ndQuadrant.com/books/
PostgreSQL Development, 24x7 Support, Training and Services

On Tue, Jan 11, 2011 at 13:18, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> On Tue, 2011-01-11 at 13:04 +0100, Magnus Hagander wrote:
>> On Tue, Jan 11, 2011 at 12:58, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>> > On Tue, 2011-01-11 at 12:41 +0100, Magnus Hagander wrote:
>> >
>> >> Just to be clear, you're objecting to the *name* of the state, right,
>> >> not how/where it's set?
>> >
>> > Yes
>> >
>> >> In particular, how the catchup/streaming
>> >> things are set?
>> >
>> > You've set it in the right places.
>> >
>> > I would personally constrain the state transitions, so that we can't
>> > ever make illegal changes, such as CATCHUP -> BACKUP.
>>
>> Well, once we enter the walsender loop we can never get out of it, so
>> it simply cannot happen...
>
> Accidentally it can, so I would like to protect against that.
>
> Putting those checks in are like Asserts(), they help document what is
> and is not possible by design.
>
>> > I would also check the state locally before grabbing the spinlock, as is
>> > typical in other xlog code. Continually resetting shared state to
>> > exactly what it is already seems strange, to me. If we make the rule
>> > that the state can only be changed by the WALSender itself, it won't
>> > need to grab spinlock. If you don't like that, a local variable works.
>>
>> Hmm. I don't see why anybody other than the walsender should change
>> it, so yeah. You mean I can just drop the spinlock calls completely,
>> and then do an
>> if (walsnd->state != state)
>>    walsnd->state = state;
>>
>>
>> ? Do I need to keep the volatile pointer, or should I drop that as well?
>
> No, do this at top
>
> if (walsnd->state == state)
>  return;
>
> Keep spinlocks when actually setting it.

Aha. Thanks for the pointers, pfa a new version.

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

On Tue, Jan 11, 2011 at 7:24 AM, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
>> No, do this at top
>>
>> if (walsnd->state == state)
>>  return;
>>
>> Keep spinlocks when actually setting it.

I think this is safe...

> Aha. Thanks for the pointers, pfa a new version.

...but I think you also need to take the spinlock when reading the value.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Tue, 11 Jan 2011 13:24:33 +0100
Magnus Hagander <magnus(at)hagander(dot)net> wrote:
> Aha. Thanks for the pointers, pfa a new version.

Changing pg_stat replication view would require to fix regression test
"rule". Please find attached patch.

Regards,
--
Shigeru Hanada

On 01/11/2011 10:24 PM, Shigeru HANADA wrote:
> On Tue, 11 Jan 2011 13:24:33 +0100
> Magnus Hagander<magnus(at)hagander(dot)net> wrote:
>> Aha. Thanks for the pointers, pfa a new version.
>
> Changing pg_stat replication view would require to fix regression test
> "rule". Please find attached patch.
>
>

I have just committed a fix for this.

cheers

andrew

On Wed, Jan 12, 2011 at 03:03, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Tue, Jan 11, 2011 at 7:24 AM, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
>>> No, do this at top
>>>
>>> if (walsnd->state == state)
>>>  return;
>>>
>>> Keep spinlocks when actually setting it.
>
> I think this is safe...
>
>> Aha. Thanks for the pointers, pfa a new version.
>
> ...but I think you also need to take the spinlock when reading the value.

Even when it can only ever be set by one process (the owning
walsender), and the variable is atomic (as it should be, since it's a
single enum/int)?

Anyway, it should be as simple as copying it out to a local variable
when it's already in the spinlock and then use that, right?

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

On Thu, Jan 13, 2011 at 11:08 AM, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
> On Wed, Jan 12, 2011 at 03:03, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> On Tue, Jan 11, 2011 at 7:24 AM, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
>>>> No, do this at top
>>>>
>>>> if (walsnd->state == state)
>>>>  return;
>>>>
>>>> Keep spinlocks when actually setting it.
>>
>> I think this is safe...
>>
>>> Aha. Thanks for the pointers, pfa a new version.
>>
>> ...but I think you also need to take the spinlock when reading the value.
>
> Even when it can only ever be set by one process (the owning
> walsender), and the variable is atomic (as it should be, since it's a
> single enum/int)?

The fact that it can only be modified by one process makes it safe for
*that process* to read it without taking the lock, but another process
that wants to read it still needs the lock, I believe - otherwise you
might get a slightly stale value. That's probably not a *huge* deal
in this case, but I think it'd be better to get it right because
people tend to copy these sorts of things elsewhere, and it'd be bad
if it got copied into some place more critical.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Thu, Jan 13, 2011 at 18:43, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Thu, Jan 13, 2011 at 11:08 AM, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
>> On Wed, Jan 12, 2011 at 03:03, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>> On Tue, Jan 11, 2011 at 7:24 AM, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
>>>>> No, do this at top
>>>>>
>>>>> if (walsnd->state == state)
>>>>>  return;
>>>>>
>>>>> Keep spinlocks when actually setting it.
>>>
>>> I think this is safe...
>>>
>>>> Aha. Thanks for the pointers, pfa a new version.
>>>
>>> ...but I think you also need to take the spinlock when reading the value.
>>
>> Even when it can only ever be set by one process (the owning
>> walsender), and the variable is atomic (as it should be, since it's a
>> single enum/int)?
>
> The fact that it can only be modified by one process makes it safe for
> *that process* to read it without taking the lock, but another process
> that wants to read it still needs the lock, I believe - otherwise you
> might get a slightly stale value.  That's probably not a *huge* deal
> in this case, but I think it'd be better to get it right because
> people tend to copy these sorts of things elsewhere, and it'd be bad
> if it got copied into some place more critical.

ok, thanks for the pointers - fix applied.

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/