From: Michael Paquier <michael@paquier.xyz>
Date: Mon, 6 Apr 2026 05:01:04 +0000 (+0900)
Subject: Use single LWLock for lock statistics in pgstats
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=404a17c155ac6e80d990fb0625569a3854f7090d;p=thirdparty%2Fpostgresql.git

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 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 eed4c6b359c..c0496f2c69c 100644
--- 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;