From 12eb6bb593fd660ff1283474a694b674b30d1e99 Mon Sep 17 00:00:00 2001 From: Bertrand Drouvot Date: Mon, 30 Mar 2026 17:16:08 +0000 Subject: [PATCH v1] lock statistics: replace per lock type locks with a single lock Currently there is one LWLock per lock type entry: this adds complexity without observable performance benefit. Plus there is a bug with nowait=true: a backend iterating over all entries could successfully flush some entries while skipping others due to contention, then unconditionally reset PendingLockStats and clear have_lockstats, losing the skipped entries permanently. This commit replaces the per entry locks with a single lock protecting all entries and the reset timestamp. This makes the nowait path correct: either the lock is acquired and everything is flushed, or it is not acquired and nothing is modified. This also better matches the intent of the nowait path. As a result, the pgstat_lock_init_shmem_cb(), pgstat_lock_reset_all_cb() and pgstat_lock_snapshot_cb() callbacks are also simplified. Author: Bertrand Drouvot Reported-by: Tomas Vondra Discussion: https://postgr.es/m/1af63e6d-16d5-4d5b-9b03-11472ef1adf9%40vondra.me --- src/backend/utils/activity/pgstat_lock.c | 86 ++++++++---------------- src/include/utils/pgstat_internal.h | 6 +- 2 files changed, 32 insertions(+), 60 deletions(-) 92.0% src/backend/utils/activity/ 7.9% src/include/utils/ diff --git a/src/backend/utils/activity/pgstat_lock.c b/src/backend/utils/activity/pgstat_lock.c index 041f521ce73..aec64f8fb4b 100644 --- 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 97704421a92..8ae3fbcba46 100644 --- a/src/include/utils/pgstat_internal.h +++ b/src/include/utils/pgstat_internal.h @@ -467,10 +467,10 @@ typedef struct PgStatShared_IO typedef struct PgStatShared_Lock { /* - * locks[i] protects stats.stats[i]. locks[0] also protects - * stats.stat_reset_timestamp. + * single lock protecting all entries as well as + * stats->stat_reset_timestamp. */ - LWLock locks[LOCKTAG_LAST_TYPE + 1]; + LWLock lock; PgStat_Lock stats; } PgStatShared_Lock; -- 2.34.1