Re: [HACKERS] proposal: add 'waiting for replication' to pg_stat_activity.state

2016-02-01 Thread Alvaro Herrera
Thomas Munro wrote:

> Thinking of other patches in flight, I think I'd want the proposed
> N-sync standbys feature to be able to explain in more detail what it's
> waiting for (and likewise my causal reads proposal which does that via
> the process title), though I realise that the details of how/where to
> do that are changing in the "replace pg_stat_activity.waiting" thread
> which this patch is waiting on.

Right, interesting thought.  I think that for the time being we should
close this one as returned with feedback, since there's no point in
keeping it open in commitfest.  I encourage the author (and everyone
else) to look at the other patch and help there, which is going to
ultimately achieve the outcome desired with this patch.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] proposal: add 'waiting for replication' to pg_stat_activity.state

2016-01-28 Thread Thomas Munro
On Thu, Dec 3, 2015 at 1:00 PM, Craig Ringer  wrote:
> On 3 December 2015 at 04:22, Julian Schauder 
> wrote:
>
>>
>> I suggest adding a new state to pg_stat_activity.state for backends that
>> are
>>
>> waiting for their synchronous commit to be flushed on the remote host.
>>
>
> Excellent idea. Anything that improves management and visibility into what
> the system is doing like this is really valuable.
>
> I notice that you don't set the 'waiting' flag.  'waiting' is presently
> documented as:
>
>True if this backend is currently waiting on a lock
>
> ... but I'm inclined to just widen its definition and set it here, since we
> most certainly are waiting, and the column isn't named 'waiting_on_a_lock'.
> It shouldn't upset various canned lock monitoring queries people have since
> they generally do an inner join on pg_locks anyway.
>
> There are no test changes in this patch, but that's reasonable because we
> don't currently have a way to automate tests of sync rep.
>
> I've attached a patch with minor wording/formatting changes, but I think I'd
> like to change 'waiting' to true as well. Opinions?

A couple of minor things I noticed:

+   waiting for synchronous replication: The
backend is waiting for its transaction to be flushed on a synchronous
standby.

I wouldn't say "flushed" here.  If you set synchronous_commit =
remote_write, then it's not actually waiting for it to be flushed (and
there may eventually be other levels too).  Maybe just "The backend is
waiting for a response from a synchronous standby"?

+#include 

I think this should use "" instead of <> and should come after
#include "miscadmin.h".

Thinking of other patches in flight, I think I'd want the proposed
N-sync standbys feature to be able to explain in more detail what it's
waiting for (and likewise my causal reads proposal which does that via
the process title), though I realise that the details of how/where to
do that are changing in the "replace pg_stat_activity.waiting" thread
which this patch is waiting on.

-- 
Thomas Munro
http://www.enterprisedb.com


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] proposal: add 'waiting for replication' to pg_stat_activity.state

2016-01-18 Thread Alvaro Herrera
Andres Freund wrote:
> On December 4, 2015 9:48:55 AM GMT+01:00, Craig Ringer 
>  wrote:
> >On 3 December 2015 at 22:58, Amit Kapila 
> >wrote:
> >
> >> On Thu, Dec 3, 2015 at 9:02 AM, Craig Ringer 
> >> wrote:
> 
> >http://www.postgresql.org/message-id/CAA4eK1+=5Ex8-5NRr3u94=_t2p65v0kcjZ5rXddVmkx=lwa...@mail.gmail.com
> >>
> >Good point. I'm not sure this shouldn't set 'waiting' anyway, though.
> 
> No. Waiting corresponds to pg locks -setting it to true for other things 
> would be even more confusing...

Robert's opinion elsewhere is relevant for this patch, and contradicts
Andres' opinion here:
http://www.postgresql.org/message-id/ca+tgmoaj+epoo9qobvfh18f5kjg2taexhq8_-vaywhur-za...@mail.gmail.com

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] proposal: add 'waiting for replication' to pg_stat_activity.state

2015-12-04 Thread Craig Ringer
On 3 December 2015 at 22:58, Amit Kapila  wrote:

> On Thu, Dec 3, 2015 at 9:02 AM, Craig Ringer 
> wrote:
> >
> > On 3 December 2015 at 09:32, Peter Eisentraut  wrote:
> >>
> >> On 12/2/15 7:00 PM, Craig Ringer wrote:
> >> > I notice that you don't set the 'waiting' flag.  'waiting' is
> presently
> >> > documented as:
> >> >
> >> >True if this backend is currently waiting on a
> lock
> >> >
> >> > ... but I'm inclined to just widen its definition and set it here,
> since
> >> > we most certainly are waiting, and the column isn't named
> >> > 'waiting_on_a_lock'. It shouldn't upset various canned lock monitoring
> >> > queries people have since they generally do an inner join on pg_locks
> >> > anyway.
> >>
> >> I'm not so sure about that assumption.
> >
> >
> > Even if it's an outer join, the worst that'll happen is that they'll get
> entries with nulls in pg_locks. I don't think it's worth worrying about too
> much.
> >
>
> That can be one way of dealing with it and another is that we
> keep the current column as it is and add another view related
> wait stats, anyway we need something like that for other purposes
> like lwlock waits etc.  Basically something on lines what we
> is being discussed in thread [1]
>
> [1] -
> http://www.postgresql.org/message-id/CAA4eK1+=5Ex8-5NRr3u94=_t2p65v0kcjZ5rXddVmkx=lwa...@mail.gmail.com
>


Good point. I'm not sure this shouldn't set 'waiting' anyway, though.


-- 
 Craig Ringer   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services


Re: [HACKERS] proposal: add 'waiting for replication' to pg_stat_activity.state

2015-12-04 Thread Andres Freund
On December 4, 2015 9:48:55 AM GMT+01:00, Craig Ringer  
wrote:
>On 3 December 2015 at 22:58, Amit Kapila 
>wrote:
>
>> On Thu, Dec 3, 2015 at 9:02 AM, Craig Ringer 
>> wrote:

>http://www.postgresql.org/message-id/CAA4eK1+=5Ex8-5NRr3u94=_t2p65v0kcjZ5rXddVmkx=lwa...@mail.gmail.com
>>
>Good point. I'm not sure this shouldn't set 'waiting' anyway, though.

No. Waiting corresponds to pg locks -setting it to true for other things would 
be even more confusing...

Andres

--- 
Please excuse brevity and formatting - I am writing this on my mobile phone.


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] proposal: add 'waiting for replication' to pg_stat_activity.state

2015-12-03 Thread Amit Kapila
On Thu, Dec 3, 2015 at 9:02 AM, Craig Ringer  wrote:
>
> On 3 December 2015 at 09:32, Peter Eisentraut  wrote:
>>
>> On 12/2/15 7:00 PM, Craig Ringer wrote:
>> > I notice that you don't set the 'waiting' flag.  'waiting' is presently
>> > documented as:
>> >
>> >True if this backend is currently waiting on a
lock
>> >
>> > ... but I'm inclined to just widen its definition and set it here,
since
>> > we most certainly are waiting, and the column isn't named
>> > 'waiting_on_a_lock'. It shouldn't upset various canned lock monitoring
>> > queries people have since they generally do an inner join on pg_locks
>> > anyway.
>>
>> I'm not so sure about that assumption.
>
>
> Even if it's an outer join, the worst that'll happen is that they'll get
entries with nulls in pg_locks. I don't think it's worth worrying about too
much.
>

That can be one way of dealing with it and another is that we
keep the current column as it is and add another view related
wait stats, anyway we need something like that for other purposes
like lwlock waits etc.  Basically something on lines what we
is being discussed in thread [1]

[1] -
http://www.postgresql.org/message-id/CAA4eK1+=5Ex8-5NRr3u94=_t2p65v0kcjZ5rXddVmkx=lwa...@mail.gmail.com


With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com


Re: [HACKERS] proposal: add 'waiting for replication' to pg_stat_activity.state

2015-12-02 Thread Craig Ringer
On 3 December 2015 at 04:22, Julian Schauder 
wrote:


> I suggest adding a new state to pg_stat_activity.state for backends that
> are
>
waiting for their synchronous commit to be flushed on the remote host.
>
>
Excellent idea. Anything that improves management and visibility into what
the system is doing like this is really valuable.

I notice that you don't set the 'waiting' flag.  'waiting' is presently
documented as:

   True if this backend is currently waiting on a lock

... but I'm inclined to just widen its definition and set it here, since we
most certainly are waiting, and the column isn't named 'waiting_on_a_lock'.
It shouldn't upset various canned lock monitoring queries people have since
they generally do an inner join on pg_locks anyway.

There are no test changes in this patch, but that's reasonable because we
don't currently have a way to automate tests of sync rep.

I've attached a patch with minor wording/formatting changes, but I think
I'd like to change 'waiting' to true as well. Opinions?

-- 
 Craig Ringer   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services
From e0bcab8ec199e92aaf51ac732275f1a2a5f1eb9f Mon Sep 17 00:00:00 2001
From: Craig Ringer 
Date: Thu, 3 Dec 2015 07:57:59 +0800
Subject: [PATCH] Add 'waiting for replication' state to pg_stat_activity

---
 doc/src/sgml/monitoring.sgml|  5 +
 src/backend/replication/syncrep.c   | 14 +-
 src/backend/utils/adt/pgstatfuncs.c |  3 +++
 src/include/pgstat.h|  1 +
 4 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index e64b7ef..458ae0f 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -642,6 +642,11 @@ postgres   27093  0.0  0.0  30096  2752 ?Ss   11:34   0:00 postgres: ser
  
  
   
+   waiting for synchronous replication: The backend is waiting for its transaction to be flushed on a synchronous standby.
+  
+ 
+ 
+  
idle: The backend is waiting for a new client command.
   
  
diff --git a/src/backend/replication/syncrep.c b/src/backend/replication/syncrep.c
index 325239d..f2bf5e1 100644
--- a/src/backend/replication/syncrep.c
+++ b/src/backend/replication/syncrep.c
@@ -45,7 +45,7 @@
 #include "postgres.h"
 
 #include 
-
+#include 
 #include "access/xact.h"
 #include "miscadmin.h"
 #include "replication/syncrep.h"
@@ -153,6 +153,18 @@ SyncRepWaitForLSN(XLogRecPtr XactCommitLSN)
 	}
 
 	/*
+	 * Alter 'state' in pg_stat before entering the loop.
+	 *
+	 * As with updating the ps display it is safe to assume that we'll wait
+	 * at least for a short time so we just set the state without bothering
+	 * to check if we're really going to have to wait for the standby.
+	 *
+	 * We don't set the 'waiting' flag because that's documented as waiting
+	 * on a lock.
+	 */
+	pgstat_report_activity(STATE_WAITINGFORREPLICATION,NULL);
+
+	/*
 	 * Wait for specified LSN to be confirmed.
 	 *
 	 * Each proc has its own wait latch, so we perform a normal latch
diff --git a/src/backend/utils/adt/pgstatfuncs.c b/src/backend/utils/adt/pgstatfuncs.c
index f7c9bf6..84d67e0 100644
--- a/src/backend/utils/adt/pgstatfuncs.c
+++ b/src/backend/utils/adt/pgstatfuncs.c
@@ -663,6 +663,9 @@ pg_stat_get_activity(PG_FUNCTION_ARGS)
 case STATE_IDLEINTRANSACTION_ABORTED:
 	values[4] = CStringGetTextDatum("idle in transaction (aborted)");
 	break;
+case STATE_WAITINGFORREPLICATION:
+	values[4] = CStringGetTextDatum("waiting for synchronous replication");
+	break;
 case STATE_DISABLED:
 	values[4] = CStringGetTextDatum("disabled");
 	break;
diff --git a/src/include/pgstat.h b/src/include/pgstat.h
index 9ecc163..ab1befc 100644
--- a/src/include/pgstat.h
+++ b/src/include/pgstat.h
@@ -692,6 +692,7 @@ typedef enum BackendState
 	STATE_IDLEINTRANSACTION,
 	STATE_FASTPATH,
 	STATE_IDLEINTRANSACTION_ABORTED,
+	STATE_WAITINGFORREPLICATION,
 	STATE_DISABLED
 } BackendState;
 
-- 
2.1.0


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] proposal: add 'waiting for replication' to pg_stat_activity.state

2015-12-02 Thread Craig Ringer
On 3 December 2015 at 04:22, Julian Schauder 
wrote:

>
> I suggest adding a new state to pg_stat_activity.state for backends that
> are
> waiting for their synchronous commit to be flushed on the remote host.
> I chose 'waiting for synchronous replication' for now.
>
>
I've added this to the next CF:

https://commitfest.postgresql.org/8/436/


-- 
 Craig Ringer   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services


Re: [HACKERS] proposal: add 'waiting for replication' to pg_stat_activity.state

2015-12-02 Thread Craig Ringer
On 3 December 2015 at 09:32, Peter Eisentraut  wrote:

> On 12/2/15 7:00 PM, Craig Ringer wrote:
> > I notice that you don't set the 'waiting' flag.  'waiting' is presently
> > documented as:
> >
> >True if this backend is currently waiting on a lock
> >
> > ... but I'm inclined to just widen its definition and set it here, since
> > we most certainly are waiting, and the column isn't named
> > 'waiting_on_a_lock'. It shouldn't upset various canned lock monitoring
> > queries people have since they generally do an inner join on pg_locks
> > anyway.
>
> I'm not so sure about that assumption.
>

Even if it's an outer join, the worst that'll happen is that they'll get
entries with nulls in pg_locks. I don't think it's worth worrying about too
much.

We could always mitigate it by adding a pg_lock_status view to the system
catalogs with a decent canned query over pg_stat_activity and pg_locks, so
people can stop copying & pasting from the wiki or using buggy homebrew
queries ;)

-- 
 Craig Ringer   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services


Re: [HACKERS] proposal: add 'waiting for replication' to pg_stat_activity.state

2015-12-02 Thread Peter Eisentraut
On 12/2/15 7:00 PM, Craig Ringer wrote:
> I notice that you don't set the 'waiting' flag.  'waiting' is presently
> documented as:
> 
>True if this backend is currently waiting on a lock
> 
> ... but I'm inclined to just widen its definition and set it here, since
> we most certainly are waiting, and the column isn't named
> 'waiting_on_a_lock'. It shouldn't upset various canned lock monitoring
> queries people have since they generally do an inner join on pg_locks
> anyway.

I'm not so sure about that assumption.



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers