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

On Tue, Jul 7, 2015 at 12:57 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>
> On Tue, Jun 30, 2015 at 10:30 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
wrote:
> > 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.
>

I think as part of this patch, we are trying to capture events where we
need to wait, extending the scope to include CPU events might duplicate
the tracing events (DTRACE) we already have in PostgreSQL and it might
degrade performance in certain cases. If we try to capture events only
during waits, then there is almost no chance of having any visible
performance
impact. I suggest for an initial implementation, lets track Heavy weight
locks,
Light Weight Locks and IO events and then we can add more events if we think
those are important.

> Here are some review comments on the patch:
>
> When I played around the patch, the test of make check failed.
>

Yes, I still need to update some of the tests according to updates in
pg_stat_activity, but the main reason I have not done so is that the user
interface needs some more discussion.

> 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?
>

As pointed by Kyotaro-San, 'waiting' column will indicate whether it
is still waiting on the event specified in wait_event column.
Currently waiting is updated only for Heavy Weight locks, if there is
no objection to use it for other wait_events (aka break backward
compatibility
for that parameter), then I can update it for other events as well, do you
have any better suggestions for the same?

> +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?
>

Yes, we can do that way.

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