Re: Waits monitoring

On Mon, 7 Sep 2015 08:58:15 +0530
Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:

> On Sun, Sep 6, 2015 at 5:58 PM, Andres Freund <andres(at)anarazel(dot)de>
> wrote:
> >
> > On 2015-09-04 23:44:21 +0100, Simon Riggs wrote:
> > > I see the need for both current wait information and for
> > > cumulative historical detail.
> > >
> > > I'm willing to wait before reviewing this, but not for more than
> > > 1 more
> CF.
> > >
> > > Andres, please decide whether we should punt to next CF now,
> > > based upon other developments. Thanks
> >
> > I think we can do some of the work concurrently - the whole lwlock
> > infrastructure piece is rather independent and what currently most
> > of the arguments are about. I agree that the actual interface will
> > need to be coordinated.
> >
> > Ildus, could you please review Amit & Robert's patch?
> >
>
> Are you talking about patch where I have fixed few issues in Robert's
> patch [1] or the original patch [3] written by me.
>
> IIUC, this is somewhat different than what Ildus is doing in his
> latest patch [2].
>
> Sorry, but I think there is some confusion about that patch [1]
> development. Let me try to summarize what I think has happened and
> why I feel there is some confusion. Initially Robert has proposed
> the idea of having a column in pg_stat_activity for wait_event on
> hackers and then I wrote an initial patch so that we can discuss the
> same in a more meaningful way and wanted to extend that patch based
> on consensus and what any other patch like Waits monitoring would
> need from it. In-between Ildus has proposed
> Waits monitoring patch and also started hacking the other
> pg_stat_activity patch and that was the starting point of confusion.
> Now I think that the current
> situation is there's one patch [1] of Robert (with some fixes by
> myself) for standardising
> LWLock stuff, so that we can build pg_stat_activity patch on top of
> it and then
> a patch [2] from Ildus for doing something similar but I think it
> hasn't used Robert's
> patch.
>
> I think the intention of having multiple people develop same patch is
> to get the work done faster, but I think here it is causing confusion
> and I feel that
> is one reason the patch is getting dragged as well. Let me know your
> thoughts
> about what is the best way to proceed?
>
> [1] -
> http://www.postgresql.org/message-id/CAA4eK1KdeC1Tm5ya9gkV85Vtn4qqsRxzKJrU-tu70G_tL1xkFA@mail.gmail.com
> [2] -
> http://www.postgresql.org/message-id/3F71DA37-A17B-4961-9908-016E6323E612@postgrespro.ru
> [3] -
> http://www.postgresql.org/message-id/CAA4eK1Kt2e6XhViGisR5o1aC9NfO0j2wTb8N0ggD1_JkLdeKdQ@mail.gmail.com
>
> P.S. - This mail is not to point anything wrong with any particular
> individual,
> rather about the development of a particular patch getting haphazard
> because of some confusion. I am not sure that this is the right
> thread to write about
> it, but as it has been asked here to review the patch in other related
> thread,
> so I have mentioned my thoughts on the same.

About [1] and [2]. They are slightly conflicting, but if [1] is
going to be committed I can easily use it in [2]. And it will simplify
my patch, so I have no objections here. And the same time [3] can be
significantly simplified and improved on top of [1] and [2].

For example this code from [3]:

static void
LWLockReportStat(LWLock *lock)
{
int lockid;
const char *tranche_name;

tranche_name = T_NAME(lock);

if (strcmp(tranche_name, "main") == 0)
{
lockid = T_ID(lock);

if (lockid < NUM_INDIVIDUAL_LWLOCKS)
pgstat_report_wait_event(LWLockTypeToWaitEvent(lockid));
else if (lockid >= BUFFER_MAPPING_LWLOCK_OFFSET &&
lockid < LOCK_MANAGER_LWLOCK_OFFSET)
pgstat_report_wait_event(WaitEvent_BufferMappingLock);
else if (lockid >= LOCK_MANAGER_LWLOCK_OFFSET &&
lockid <
PREDICATELOCK_MANAGER_LWLOCK_OFFSET)
pgstat_report_wait_event(WaitEvent_LockManagerLock);
else if (lockid >=
PREDICATELOCK_MANAGER_LWLOCK_OFFSET&& lockid <
NUM_FIXED_LWLOCKS)
pgstat_report_wait_event(WaitEvent_PredicateManagerLock);
else pgstat_report_wait_event(WaitEvent_OtherLWLock); }
else if (strcmp(tranche_name, "WALInsertLocks") == 0)
pgstat_report_wait_event(WaitEvent_WALInsertLocks);
else if (strcmp(tranche_name, "ReplicationOrigins") == 0)
pgstat_report_wait_event(WaitEvent_ReplicationOrigins);
else
pgstat_report_wait_event(WaitEvent_OtherTrancheLWLock);
}

can be changed to something like:

#define LWLOCK_WAIT_ID(lock) \
(lock->tranche == 0? T_ID(lock) : lock->tranche +
NUM_INDIVIDUAL_LWLOCKS)

static void
LWLockReportStat(LWLock *lock)
{
int offset = <pgstat offset for lwlocks>;
pgstat_report_wait_event(offset + LWLOCK_WAIT_ID(lock));
}

So it will almost not affect to performance of LWLockAcquire.

----
Ildus Kurbangaliev
Postgres Professional: http://www.postgrespro.com
<http://www.postgrespro.com/> The Russian Postgres Company