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

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,

--
Fujii Masao