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

On Wed, Feb 24, 2016 at 7:14 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
> On Mon, Feb 22, 2016 at 10:05 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
wrote:
> >> I wouldn't bother tinkering with it at this point. The value isn't
> >> going to be recorded on disk anywhere, so it will be easy to change
> >> the way it's computed in the future if we ever need to do that.
> >>
> >
> > Okay. Find the rebased patch attached with this mail. I have moved
> > this patch to upcoming CF.
>
> I would call the functions pgstat_report_wait_start() and
> pgstat_report_wait_end() instead of pgstat_report_start_waiting() and
> pgstat_report_end_waiting().
>

Changed as per suggestion and made these functions inline.

> I think pgstat_get_wait_event_type should not return HWLock, a term
> that appears nowhere in our source tree at present. How about just
> "Lock"?
>

Changed as per suggestion.

> I think the wait event types should be documented - and the wait
> events too, perhaps.
>

As discussed upthread, I have added documentation for all the possible wait
events and an example. Some of the LWLocks like AsyncQueueLock and
AsyncCtlLock are used for quite similar purpose, so I have kept there
explanation as same.

> Maybe it's worth having separate wait event type names for lwlocks and
> lwlock tranches. We could report LWLockNamed and LWLockTranche and
> document the difference: "LWLockNamed indicates that the backend is
> waiting for a specific, named LWLock. The event name is the name of
> that lock. LWLockTranche indicates that the backend is waiting for
> any one of a group of locks with similar function. The event name
> identifies the general purpose of locks in that group."
>

Changed as per suggestion.

> There's no requirement that every session have every tranche
> registered. I think we should consider displaying "extension" for any
> tranche that's not built-in, or at least for tranches that are not
> registered (rather than "unknown wait event").
>
> + if (lock->tranche == 0 && lockId < NUM_INDIVIDUAL_LWLOCKS)
>
> Isn't the second part automatically true at this point?
>

Fixed.

> The changes to LockBufferForCleanup() don't look right to me. Waiting
> for a buffer pin might be a reasonable thing to define as a wait
> event, but it shouldn't reported as if we were waiting on the LWLock
> itself.
>

As discussed upthread, added a new wait event BufferPin for this case.

> What happens if an error is thrown while we're in a wait?
>

As discussed upthread, added in AbortTransaction and from where
ever LWLockReleaseAll() gets called, point to note is that we can call this
function only when we are sure there is no further possibility of wait on
LWLock.

> Does this patch hurt performance?
>

Performance tests are underway.

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