Re: RFC: replace pg_stat_activity.waiting with something more descriptive - Mailing list pgsql-hackers

On 4 March 2016 at 04:05, Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Wed, Feb 24, 2016 at 7:14 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>>
>> On Mon, Feb 22, 2016 at 10:05 AM, Amit Kapila <amit.kapila16@gmail.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.

I've attached a revised version of the patch with the following corrections:

+         <para>
+          <literal>LWLockTranche</>: The backend is waiting for any one of a
+          group of locks with similar function.  The <literal>wait_event</>
+          name for this type of wait identifies the general purpose of locks
+          in that group.
+         </para>

s/with similar/with a similar/

+        <row>
+         <entry><literal>ControlFileLock</></entry>
+         <entry>A server process is waiting to read or update the control file
+         or creation of a new WAL log file.</entry>
+        </row>

As the L in WAL stands for "log" anyway, I think the extra "log" word
can be removed.

+        <row>
+         <entry><literal>RelCacheInitLock</></entry>
+         <entry>A server process is waiting to read or write to relation cache
+         initialization file.</entry>
+        </row>

s/to relation/to the relation/

+        <row>
+         <entry><literal>BtreeVacuumLock</></entry>
+          <entry>A server process is waiting to read or update vacuum related
+          information for Btree index.</entry>
+        </row>

s/vacuum related/vacuum-related/
s/for Btree/for a Btree/

+        <row>
+         <entry><literal>AutovacuumLock</></entry>
+         <entry>A server process which could be autovacuum worker is waiting to
+         update or read current state of autovacuum workers.</entry>
+        </row>

s/could be autovacuum/could be that an autovacuum/
s/read current/read the current/

(discussed with Amit offline about other sources of wait, and he
suggested autovacuum launcher, so I've added that in too)

+        <row>
+         <entry><literal>AutovacuumScheduleLock</></entry>
+         <entry>A server process is waiting on another process to ensure that
+         the table it has selected for vacuum still needs vacuum.
+         </entry>
+        </row>

s/for vacuum/for a vacuum/
s/still needs vacuum/still needs vacuuming/

+        <row>
+         <entry><literal>SyncScanLock</></entry>
+         <entry>A server process is waiting to get the start location of scan
+         on table for synchronized scans.</entry>
+        </row>

s/of scan/of a scan/
s/on table/on a table/

+        <row>
+         <entry><literal>SerializableFinishedListLock</></entry>
+         <entry>A server process is waiting to access list of finished
+         serializable transactions.</entry>
+        </row>

s/to access list/to access the list/

+        <row>
+         <entry><literal>SerializablePredicateLockListLock</></entry>
+         <entry>A server process is waiting to perform operation on list of
+         locks held by serializable transactions.</entry>
+        </row>

s/perform operation/perform an operation/
s/on list/on a list/

+        <row>
+         <entry><literal>AutoFileLock</></entry>
+         <entry>A server process is waiting to update
<filename>postgresql.auto.conf</>
+         file.</entry>
+        </row>

s/to update/to update the/

+        <row>
+         <entry><literal>CommitTsLock</></entry>
+         <entry>A server process is waiting to read or update the last value
+         set for transaction timestamp.</entry>
+        </row>

s/for transaction/for the transaction/

+        <row>
+         <entry><literal>clog</></entry>
+         <entry>A server process is waiting on any one of the clog buffer locks
+         to read or write the clog page in pg_clog subdirectory.</entry>
+        </row>

s/page in/page in the/

The 6 rows that follow that one could apply this same correction.

+        <row>
+         <entry><literal>wal_insert</></entry>
+         <entry>A server process is waiting on any one of the wal_insert locks
+         to write data in wal buffers.</entry>
+        </row>

s/data in/data into/

+        <row>
+         <entry><literal>buffer_content</></entry>
+         <entry>A server process is waiting on any one of the buffer_content
+         locks to read or write data in shared buffers.</entry>
+        </row>

s/data in/data into/

+        <row>
+         <entry><literal>buffer_io</></entry>
+         <entry>A server process is waiting on any one of the buffer_io locks
+         to allow other process to complete the I/O on buffer.</entry>
+        </row>

s/allow other process/allow another process/

+        <row>
+         <entry><literal>buffer_mapping</></entry>
+         <entry>A server process is waiting on any one of the buffer_mapping
+         locks to associate a data block with buffer in buffer pool.</entry>
+        </row>

s/with buffer/with a buffer/
s/in buffer pool/in the buffer pool/

+        <row>
+         <entry><literal>lock_manager</></entry>
+         <entry>A server process is waiting on any one of the
lock_manager locks
+         to add or examine locks for backends or waiting on joining/exiting
+         parallel group to perform parallel query.</entry>
+        </row>

s/exiting parallel/exiting a parallel/
s/perform parallel/perform a parallel/

+        <row>
+         <entry><literal>relation</></entry>
+         <entry>A server process is waiting to acquire lock on a
relation.</entry>
+        </row>

s/acquire lock/acquire a lock/

+        <row>
+         <entry><literal>tuple</></entry>
+         <entry>A server process is waiting to acquire a lock on tuple.</entry>
+        </row>

s/on tuple/on a tuple/

+        <row>
+         <entry><literal>virtualxid</></entry>
+         <entry>A server process is waiting to acquire virtual xid
lock.</entry>
+        </row>

s/acquire virtual/acquire a virtual/

The 5 rows that follow the above one can apply a similar substitution
(i.e. s/acquire/acquire a/ )

+    <para>
+     For tranches registered by extensions, the name is specified by extension
+     and the same will be displayed as <structfield>wait_event</>.  It is quite
+     possible that user has registered tranche in one of the backends (by
+     having allocation in dynamic shared memory) in which case other backends
+     won't have that information, so we display <literal>extension</> for such
+     cases.
+    </para>

s/and the same will/and this will/
s/has registered tranche/has registered the tranche/

Regards

Thom