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

On Thu, Dec 3, 2015 at 1:00 PM, Craig Ringer <craig(at)2ndquadrant(dot)com> wrote:
> On 3 December 2015 at 04:22, Julian Schauder <julian(dot)schauder(at)credativ(dot)de>
> 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:
>
> <entry>True if this backend is currently waiting on a lock</entry>
>
> ... 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:

+ <literal>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 <pgstat.h>

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