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

On Fri, Jul 24, 2015 at 12:31 AM, Ildus Kurbangaliev <
i(dot)kurbangaliev(at)postgrespro(dot)ru> wrote:

> Hello.
> I’ve changed the previous patch. `group` field in LWLock is removed, so
> the size of LWLock will not increase.
> Instead of the `group` field I've created new tranches for LWLocks from
> MainLWLocksArray.

@@ -448,6 +549,30 @@ CreateLWLocks(void)
+ /* Initialized tranche id for buffer mapping lwlocks */
+ for (id = 0; id < NUM_BUFFER_PARTITIONS; id++)
+ {
+ LWLock *lock;
+ lock = &MainLWLockArray[BUFFER_MAPPING_LWLOCK_OFFSET + id].lock;
+ lock->tranche = LW_BUFFER_MAPPING;
+ }
+
+ /* Initialized tranche id for lock manager lwlocks */
+ for (id = 0; id < NUM_LOCK_PARTITIONS; id++)
+ {
+ LWLock *lock;
+ lock = &MainLWLockArray[LOCK_MANAGER_LWLOCK_OFFSET + id].lock;
+ lock->tranche = LW_LOCK_MANAGER;
+ }
+
+ /* Initialized tranche id for predicate lock manager lwlocks */
+ for (id = 0; id < NUM_PREDICATELOCK_PARTITIONS; id++)
+ {
+ LWLock *lock;
+ lock = &MainLWLockArray[PREDICATELOCK_MANAGER_LWLOCK_OFFSET + id].lock;
+ lock->tranche = LW_PREDICATELOCK_MANAGER;
+ }

I don't think we need to separately set tranche for above locks, we
can easily identify the same by lockid as is done in my patch.

2.
+const char *
+pgstat_get_wait_event_name(uint8 classId, uint8 eventId)
{
..
}

I don't understand why a single WaitEventType enum is not sufficient
as in the patch provided by me and why we need to divide it into
separate structures for each kind of wait, I find that way easily
understandable.

3.
+void
+pgstat_report_wait_end()
{
}

Till now, what has been discussed is that we will not clear the
waiting event untill next wait, rather we will change waiting
as true or false, so this also doesn't seem to be inline with
whats being discussed.

In general, I think if we want to extend the patch for more wait types,
then lets do it in the way I have defined them initially as I haven't
heard anything bad in this thread about the way it has bee initially
designed.

"I don't mind doing it some other way as you are proposing or some
one else proposes, but the point is that we should get sufficient
buy-in for going in one or the other way, just going ahead and modifying
it do the things in different way doesn't appear to be a good way to
proceed".

Also I think before extending we should be careful in adding such
stats collection, example if we have to add new stats collection in
below path as is done in patch, then we need to measure performance
for read-only cases when data-doesn't fit in shared buffers as I think
this will be used heavily in such a path:

@@ -674,6 +675,8 @@ mdread(SMgrRelation reln, ForkNumber forknum,
BlockNumber blocknum,
int nbytes;
MdfdVec *v;

+ pgstat_report_wait_start(WAIT_IO, WAIT_IO_READ);
+
TRACE_POSTGRESQL_SMGR_MD_READ_START(forknum, blocknum,
reln->smgr_rnode.node.spcNode,
reln->smgr_rnode.node.dbNode,
@@ -702,6 +705,8 @@ mdread(SMgrRelation reln, ForkNumber forknum,
BlockNumber blocknum,
nbytes,
BLCKSZ);

+ pgstat_report_wait_end();
+

I have added stats collection at minimal places so that we minimize such
impacts and adding at more places could be convenient if they are proven
to be safe performance impact-wise.

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