Use single LWLock for lock statistics in pgstats

Previously, one LWLock was used for each lock type, adding complexity
without an observable performance benefit as data is gathered only for
paths involving lock waits, at least currently.  This commit replaces
the per-type set of LWLocks with a single LWLock protecting the stats
data of all the lock types, like the stats kinds for SLRU or WAL.  A
good chunk of the callbacks get simpler thanks to this change.

The previous approach also had one bug in the flush callback when nowait
was called with "true": a backend iterating over all entries could
successfully flush some entries while skipping others due to contention,
then unconditionally reset the pending data.  This would cause some
stats data loss.

Oversight in 4019f725f5d4.

Reported-by: Tomas Vondra <tomas@vondra.me>
Author: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Discussion: https://postgr.es/m/1af63e6d-16d5-4d5b-9b03-11472ef1adf9@vondra.me

diff --git a/src/backend/utils/activity/pgstat_lock.c b/src/backend/utils/activity/pgstat_lock.c
index 041f521ce737012fad90ace32f72885ebdaf7eb8..aec64f8fb4b63cf4ef67c0e1b1466274bafd5174 100644 (file)
--- a/src/backend/utils/activity/pgstat_lock.c
+++ b/src/backend/utils/activity/pgstat_lock.c
@@ -50,99 +50,71 @@ pgstat_lock_flush(bool nowait)
 bool
 pgstat_lock_flush_cb(bool nowait)
 {
-       LWLock     *lcktype_lock;
-       PgStat_LockEntry *lck_shstats;
-       bool            lock_not_acquired = false;
+       LWLock     *lckstat_lock;
+       PgStatShared_Lock *shstats;
 
        if (!have_lockstats)
                return false;
 
+       shstats = &pgStatLocal.shmem->lock;
+       lckstat_lock = &shstats->lock;
+
+       if (!nowait)
+               LWLockAcquire(lckstat_lock, LW_EXCLUSIVE);
+       else if (!LWLockConditionalAcquire(lckstat_lock, LW_EXCLUSIVE))
+               return true;
+
        for (int i = 0; i <= LOCKTAG_LAST_TYPE; i++)
        {
-               lcktype_lock = &pgStatLocal.shmem->lock.locks[i];
-               lck_shstats =
-                       &pgStatLocal.shmem->lock.stats.stats[i];
-
-               if (!nowait)
-                       LWLockAcquire(lcktype_lock, LW_EXCLUSIVE);
-               else if (!LWLockConditionalAcquire(lcktype_lock, LW_EXCLUSIVE))
-               {
-                       lock_not_acquired = true;
-                       continue;
-               }
-
 #define LOCKSTAT_ACC(fld) \
-       (lck_shstats->fld += PendingLockStats.stats[i].fld)
+       (shstats->stats.stats[i].fld += PendingLockStats.stats[i].fld)
                LOCKSTAT_ACC(waits);
                LOCKSTAT_ACC(wait_time);
                LOCKSTAT_ACC(fastpath_exceeded);
 #undef LOCKSTAT_ACC
-
-               LWLockRelease(lcktype_lock);
        }
 
-       memset(&PendingLockStats, 0, sizeof(PendingLockStats));
+       LWLockRelease(lckstat_lock);
 
+       memset(&PendingLockStats, 0, sizeof(PendingLockStats));
        have_lockstats = false;
 
-       return lock_not_acquired;
+       return false;
 }
 
-
 void
 pgstat_lock_init_shmem_cb(void *stats)
 {
        PgStatShared_Lock *stat_shmem = (PgStatShared_Lock *) stats;
 
-       for (int i = 0; i <= LOCKTAG_LAST_TYPE; i++)
-               LWLockInitialize(&stat_shmem->locks[i], LWTRANCHE_PGSTATS_DATA);
+       LWLockInitialize(&stat_shmem->lock, LWTRANCHE_PGSTATS_DATA);
 }
 
 void
 pgstat_lock_reset_all_cb(TimestampTz ts)
 {
-       for (int i = 0; i <= LOCKTAG_LAST_TYPE; i++)
-       {
-               LWLock     *lcktype_lock = &pgStatLocal.shmem->lock.locks[i];
-               PgStat_LockEntry *lck_shstats = &pgStatLocal.shmem->lock.stats.stats[i];
+       LWLock     *lckstat_lock = &pgStatLocal.shmem->lock.lock;
 
-               LWLockAcquire(lcktype_lock, LW_EXCLUSIVE);
+       LWLockAcquire(lckstat_lock, LW_EXCLUSIVE);
 
-               /*
-                * Use the lock in the first lock type PgStat_LockEntry to protect the
-                * reset timestamp as well.
-                */
-               if (i == 0)
-                       pgStatLocal.shmem->lock.stats.stat_reset_timestamp = ts;
+       pgStatLocal.shmem->lock.stats.stat_reset_timestamp = ts;
 
-               memset(lck_shstats, 0, sizeof(*lck_shstats));
-               LWLockRelease(lcktype_lock);
-       }
+       memset(pgStatLocal.shmem->lock.stats.stats, 0,
+                  sizeof(pgStatLocal.shmem->lock.stats.stats));
+
+       LWLockRelease(lckstat_lock);
 }
 
 void
 pgstat_lock_snapshot_cb(void)
 {
-       for (int i = 0; i <= LOCKTAG_LAST_TYPE; i++)
-       {
-               LWLock     *lcktype_lock = &pgStatLocal.shmem->lock.locks[i];
-               PgStat_LockEntry *lck_shstats = &pgStatLocal.shmem->lock.stats.stats[i];
-               PgStat_LockEntry *lck_snap = &pgStatLocal.snapshot.lock.stats[i];
-
-               LWLockAcquire(lcktype_lock, LW_SHARED);
-
-               /*
-                * Use the lock in the first lock type PgStat_LockEntry to protect the
-                * reset timestamp as well.
-                */
-               if (i == 0)
-                       pgStatLocal.snapshot.lock.stat_reset_timestamp =
-                               pgStatLocal.shmem->lock.stats.stat_reset_timestamp;
-
-               /* using struct assignment due to better type safety */
-               *lck_snap = *lck_shstats;
-               LWLockRelease(lcktype_lock);
-       }
+       LWLock     *lckstat_lock = &pgStatLocal.shmem->lock.lock;
+
+       LWLockAcquire(lckstat_lock, LW_SHARED);
+
+       pgStatLocal.snapshot.lock = pgStatLocal.shmem->lock.stats;
+
+       LWLockRelease(lckstat_lock);
 }
 
 /*

diff --git a/src/include/utils/pgstat_internal.h b/src/include/utils/pgstat_internal.h
index eed4c6b359c6cea306aa4f86c22b4c9f69b0b57e..c0496f2c69c8dd9ac4ba10f278f20f63c52e8abc 100644 (file)
--- a/src/include/utils/pgstat_internal.h
+++ b/src/include/utils/pgstat_internal.h
@@ -466,11 +466,8 @@ typedef struct PgStatShared_IO
 
 typedef struct PgStatShared_Lock
 {
-       /*
-        * locks[i] protects stats.stats[i]. locks[0] also protects
-        * stats.stat_reset_timestamp.
-        */
-       LWLock          locks[LOCKTAG_LAST_TYPE + 1];
+       /* lock protects ->stats */
+       LWLock          lock;
        PgStat_Lock stats;
 } PgStatShared_Lock;