Обсуждение: Waits monitoring

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

On Thu, Jul 9, 2015 at 1:52 AM, Ildus Kurbangaliev
<i.kurbangaliev@postgrespro.ru> wrote:
> Hello.
>
> Currently, PostgreSQL offers many metrics for monitoring. However, detailed
> monitoring of waits is still not supported yet. Such monitoring would
> let dba know how long backend waited for particular event and therefore
> identify
> bottlenecks. This functionality is very useful, especially for highload
> databases. Metric for waits monitoring are provided by many popular
> commercial
> DBMS. We currently have requests of this feature from companies migrating to
> PostgreSQL from commercial DBMS. Thus, I think it would be nice for
> PostgreSQL
> to have it too.

Yes, It is good have such wait monitoring views in PostgreSQL.
you can add this patch to the open commitfest.

Regards,
Hari Babu
Fujitsu Australia

On Thu, Jul 9, 2015 at 2:12 PM, Haribabu Kommi <kommi.haribabu@gmail.com> wrote:
> On Thu, Jul 9, 2015 at 1:52 AM, Ildus Kurbangaliev
> <i.kurbangaliev@postgrespro.ru> wrote:
>> Hello.
>>
>> Currently, PostgreSQL offers many metrics for monitoring. However, detailed
>> monitoring of waits is still not supported yet. Such monitoring would
>> let dba know how long backend waited for particular event and therefore
>> identify
>> bottlenecks. This functionality is very useful, especially for highload
>> databases. Metric for waits monitoring are provided by many popular
>> commercial
>> DBMS. We currently have requests of this feature from companies migrating to
>> PostgreSQL from commercial DBMS. Thus, I think it would be nice for
>> PostgreSQL
>> to have it too.
>
> Yes, It is good have such wait monitoring views in PostgreSQL.
> you can add this patch to the open commitfest.

Robert and Amit proposed very similar idea and its patch is now being
reviewed in the current CommitFest. So I think that you should attend
that discussion rather than starting new one.

http://www.postgresql.org/message-id/CA+TgmoYd3GTz2_mJfUHF+RPe-bCy75ytJeKVv9x-o+SonCGApw@mail.gmail.com

Regards,

-- 
Fujii Masao

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?

Greetings,

Andres Freund

On Mon, 7 Sep 2015 08:58:15 +0530
Amit Kapila <amit.kapila16@gmail.com> wrote:

> On Sun, Sep 6, 2015 at 5:58 PM, Andres Freund <andres@anarazel.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

On Mon, Sep 7, 2015 at 7:33 AM, Ildus Kurbangaliev
<i.kurbangaliev@postgrespro.ru> wrote:
>> > Ildus, could you please review Amit & Robert's patch?
>> [1] -
>> http://www.postgresql.org/message-id/CAA4eK1KdeC1Tm5ya9gkV85Vtn4qqsRxzKJrU-tu70G_tL1xkFA@mail.gmail.com
> 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].

Great - let's try to deal with [1] first, then.

Does anyone wish to object to me committing that part?

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

Hi,

> Great - let's try to deal with [1] first, then.
> 
> Does anyone wish to object to me committing that part?

I have no objection to commiting this, too. But some fix would be
needed.

===
Generated lwlocknames.[ch] don't have header comment because
generate-lwlocknames.pl writes them into wrong place.

lmgr/Makefile looks to have some mistakes.
- lwlocknames.c is not generated from (or using) lwlocknames.c  so the entry "lwlocknames.c: lwlocknames.h" doesn't
looksto  be appropriate.
 
- maintainer-clean in lmgr/Makefile forgets to remove lwlocknames.c.

lwlock.c now includes lwlockname.c so such entry would be
needed. (But might not be needed because it is naturally
generated along with .h)

Perhaps uncommenting in pg_config_manual.h is left alone.
(This is not included in the diff below)


regards,


At Tue, 8 Sep 2015 13:01:23 -0400, Robert Haas <robertmhaas@gmail.com> wrote in
<CA+TgmoaKyg2p5ni_HeVxp-7U7AJ3B9k3rqPshmcBA6Zp1HE2iQ@mail.gmail.com>
> On Mon, Sep 7, 2015 at 7:33 AM, Ildus Kurbangaliev
> <i.kurbangaliev@postgrespro.ru> wrote:
> >> > Ildus, could you please review Amit & Robert's patch?
> >> [1] -
> >> http://www.postgresql.org/message-id/CAA4eK1KdeC1Tm5ya9gkV85Vtn4qqsRxzKJrU-tu70G_tL1xkFA@mail.gmail.com
> > 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].
> 
> Great - let's try to deal with [1] first, then.
> 
> Does anyone wish to object to me committing that part?


========================
--- generate-lwlocknames.pl.org    2015-09-10 15:46:02.291079423 +0900
+++ generate-lwlocknames.pl    2015-09-10 15:53:22.177903045 +0900
@@ -8,5 +8,6 @@
-print
-  "/* autogenerated from src/backend/storage/lmgr/lwlocknames.txt, do not edit */\n";
-print "/* there is deliberately not an #ifndef LWLOCKNAMES_H here */\n";
+my $head_comment_c = 
+    "/* autogenerated from src/backend/storage/lmgr/lwlocknames.txt, do not edit */\n\n";
+my $head_comment_h = $head_comment_c .
+  "/* there is deliberately not an #ifndef LWLOCKNAMES_H here */\n\n";
@@ -23,2 +24,6 @@
+# Write the caution comment at the beginning of the files
+print H $head_comment_h;
+print C $head_comment_c;
+print C "static char *MainLWLockNames[] = {";

--- Makefile.org    2015-09-10 16:16:38.610660526 +0900
+++ Makefile    2015-09-10 16:39:09.718916378 +0900
@@ -26,6 +26,6 @@
-# see explanation in ../../parser/Makefile
-lwlocknames.c: lwlocknames.h ;
+lwlock.c: lwlocknames.c
-lwlocknames.h,lwlocknames.c: $(top_srcdir)/src/backend/storage/lmgr/lwlocknames.txt generate-lwlocknames.pl
+# see explanation in ../../parser/Makefile
+lwlocknames.h lwlocknames.c: $(top_srcdir)/src/backend/storage/lmgr/lwlocknames.txt generate-lwlocknames.pl    $(PERL)
$(srcdir)/generate-lwlocknames.pl$<
 
@@ -39,2 +39,2 @@maintainer-clean: clean
-    rm -f lwlocknames.h
+    rm -f lwlocknames.[ch]

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On Thu, Sep 10, 2015 at 3:43 AM, Kyotaro HORIGUCHI
<horiguchi.kyotaro@lab.ntt.co.jp> wrote:
> Generated lwlocknames.[ch] don't have header comment because
> generate-lwlocknames.pl writes them into wrong place.
>
> lmgr/Makefile looks to have some mistakes.

Fixed.

>  - lwlocknames.c is not generated from (or using) lwlocknames.c
>    so the entry "lwlocknames.c: lwlocknames.h" doesn't looks to
>    be appropriate.

I think that's a pretty standard way of handling a case where a single
command generates multiple files.

>  - maintainer-clean in lmgr/Makefile forgets to remove lwlocknames.c.

Fixed.

> Perhaps uncommenting in pg_config_manual.h is left alone.
> (This is not included in the diff below)

Fixed.

And committed.  Thanks for the review, let's see what the buildfarm thinks.

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