Re: RFC: replace pg_stat_activity.waiting with something more descriptive

Please forgive me to resend this message for some too-sad
misspellings.

# "Waiting for heavy weight locks" is somewhat confusing to spell..

===
Hello,

At Tue, 7 Jul 2015 16:27:38 +0900, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote in <CAHGQGwEJwov8YwvmbbWps3Rba6kF1yf7qL3S==Oy4D=gq9YNsQ(at)mail(dot)gmail(dot)com>
> Each backend reports its event when trying to take a lock. But
> the reported event is never reset until next event is reported.
> Is this OK? This means that the wait_event column keeps showing
> the *last* event while a backend is in idle state, for example.
> So, shouldn't we reset the reported event or report another one
> when releasing the lock?

It seems so but pg_stat_activity.waiting would indicate whether
the event is lasting. However, .waiting reflects only the status
of heavy-weight locks. It would be quite misleading.

I think that pg_stat_activity.wait_event sould be linked to
.waiting then .wait_event should be restricted to heavy weight
locks if the meaning of .waiting cannot not be changed.

On the other hand, we need to have as many wait events as
Itagaki-san's patch did so pg_stat_activity might be the wrong
place for full-spec wait_event.

> +read_string_from_waitevent(uint8 wait_event)
>
> The performance of this function looks poor because its worst case
> is O(n): n is the number of all the events that we are trying to track.
> Also in pg_stat_activity, this function is called per backend.
> Can't we retrieve the event name by using wait event ID as an index
> of WaitEventTypeMap array?

+1

regards,

At Tue, 7 Jul 2015 16:27:38 +0900, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote in <CAHGQGwEJwov8YwvmbbWps3Rba6kF1yf7qL3S==Oy4D=gq9YNsQ(at)mail(dot)gmail(dot)com>
> On Tue, Jun 30, 2015 at 10:30 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > On Fri, Jun 26, 2015 at 6:26 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> >>
> >> On Thu, Jun 25, 2015 at 11:57 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
> >> wrote:
> >> >> >> 3. Add new view 'pg_stat_wait_event' with following info:
> >> >> >> pid - process id of this backend
> >> >> >> waiting - true for any form of wait, false otherwise
> >> >> >> wait_event_type - Heavy Weight Lock, Light Weight Lock, I/O wait,
> >> >> >> etc
> >> >> >> wait_event - Lock (Relation), Lock (Relation Extension), etc
> >> >> I am pretty unconvinced that it's a good idea to try to split up the
> >> >> wait event into two columns. I'm only proposing ~20 wait states, so
> >> >> there's something like 5 bits of information here. Spreading that
> >> >> over two text columns is a lot, and note that Amit's text would
> >> >> basically recapitulate the contents of the first column in the second
> >> >> one, which I cannot get excited about.
> >> > There is an advantage in splitting the columns which is if
> >> > wait_event_type
> >> > column indicates Heavy Weight Lock, then user can go and check further
> >> > details in pg_locks, I think he can do that even by seeing wait_event
> >> > column, but that might not be as straightforward as with wait_event_type
> >> > column.
> >>
> >> It's just a matter of writing event_type LIKE 'Lock %' instead of
> >> event_type = 'Lock'.
> >>
> >
> > Yes that way it can be done and may be that is not inconvenient for user,
> > but then there is other type of information which user might need like what
> > distinct resources on which wait is possible, which again he can easily find
> > with
> > different event_type column. I think some more discussion is required before
> > we
> > could freeze the user interface for this feature, but in the meantime I have
> > prepared an initial patch by adding a new column wait_event in
> > pg_stat_activity.
> > For now, I have added the support for Heavy-Weight locks, Light-Weight locks
> > [1]
> > and Buffer Cleanup Lock. I could add for other types (spin lock delay
> > sleep, IO,
> > network IO, etc.) if there is no objection in the approach used in patch to
> > implement
> > this feature.
> >
> > [1] For LWLocks, currently I have used wait_event as OtherLock for locks
> > other than NUM_FIXED_LWLOCKS (Refer function NumLWLocks to see all
> > type of LWLocks). The reason is that there is no straight forward way to
> > get
> > the id (lockid) of such locks as for some of those (like shared_buffers,
> > MaxBackends) the number of locks will depend on run-time configuration
> > parameters. I think if we want to handle those then we could either do some
> > math to find out the lockid based on runtime values of these parameters or
> > we could add tag in LWLock structure (which indicates the lock type) and
> > fill it during Lock initialization or may be some other better way to do it.
> >
> > I have still not added documentation and have not changed anything for
> > waiting column in pg_stat_activity as I think before that we need to
> > finalize
> > the user interface. Apart from that as mentioned above still wait for
> > some event types (like IO, netwrok IO, etc.) is not added and also I think
> > separate function/'s (like we have for existing ones
> > pg_stat_get_backend_waiting)
> > will be required which again depends upon user interface.
>
> Yes, we need to discuss what events to track. As I suggested upthread,
> Itagaki-san's patch would be helpful to think that. He proposed to track
> not only "wait event" like locking and I/O operation but also CPU events
> like query parsing, planning, and etc. I think that tracking even CPU
> events would be useful to analyze the performance problem. For example,
> if pg_stat_activity reports many times that a large majority of backends
> are doing QUERY PLANNING, DBA can think that it might be possible
> cause of performance bottleneck and try to check whether the application
> uses prepared statements properly.
>
> Here are some review comments on the patch:
>
> When I played around the patch, the test of make check failed.
>
> Each backend reports its event when trying to take a lock. But
> the reported event is never reset until next event is reported.
> Is this OK? This means that the wait_event column keeps showing
> the *last* event while a backend is in idle state, for example.
> So, shouldn't we reset the reported event or report another one
> when releasing the lock?
>
> +read_string_from_waitevent(uint8 wait_event)
>
> The performance of this function looks poor because its worst case
> is O(n): n is the number of all the events that we are trying to track.
> Also in pg_stat_activity, this function is called per backend.
> Can't we retrieve the event name by using wait event ID as an index
> of WaitEventTypeMap array?
>
> Regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

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