From cd063344fb801a90a40923a5b8aefe4eb8d80762 Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Mon, 22 Aug 2022 20:16:50 -0700
Subject: [PATCH] pgstat: Acquire lock when reading variable-numbered stats
Somewhere during the development of the patch acquiring a lock during read
access to variable-numbered stats got lost. The missing lock acquisition won't
cause corruption, but can lead to reading torn values when accessing
stats. Add the missing lock acquisitions.
Reported-by: Greg Stark <stark@mit.edu>
Reviewed-by: "Drouvot, Bertrand" <bdrouvot@amazon.com>
Reviewed-by: Andres Freund <andres@anarazel.de>
Author: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Discussion: https://postgr.es/m/CAM-w4HMYkM_DkYhWtUGV+qE_rrBxKOzOF0+5faozxO3vXrc9wA@mail.gmail.com
Backpatch: 15-
---
src/backend/utils/activity/pgstat.c | 9 +++++++++
src/backend/utils/activity/pgstat_shmem.c | 16 ++++++++++++++++
src/include/utils/pgstat_internal.h | 1 +
3 files changed, 26 insertions(+)
diff --git a/src/backend/utils/activity/pgstat.c b/src/backend/utils/activity/pgstat.c
index 88e5dd1b2b7..6224c498c21 100644
--- a/src/backend/utils/activity/pgstat.c
+++ b/src/backend/utils/activity/pgstat.c
@@ -844,9 +844,12 @@ pgstat_fetch_entry(PgStat_Kind kind, Oid dboid, Oid objoid)
else
stats_data = MemoryContextAlloc(pgStatLocal.snapshot.context,
kind_info->shared_data_len);
+
+ pgstat_lock_entry_shared(entry_ref, false);
memcpy(stats_data,
pgstat_get_entry_data(kind, entry_ref->shared_stats),
kind_info->shared_data_len);
+ pgstat_unlock_entry(entry_ref);
if (pgstat_fetch_consistency > PGSTAT_FETCH_CONSISTENCY_NONE)
{
@@ -983,9 +986,15 @@ pgstat_build_snapshot(void)
entry->data = MemoryContextAlloc(pgStatLocal.snapshot.context,
kind_info->shared_size);
+ /*
+ * Acquire the LWLock directly instead of using
+ * pg_stat_lock_entry_shared() which requires a reference.
+ */
+ LWLockAcquire(&stats_data->lock, LW_SHARED);
memcpy(entry->data,
pgstat_get_entry_data(kind, stats_data),
kind_info->shared_size);
+ LWLockRelease(&stats_data->lock);
}
dshash_seq_term(&hstat);
diff --git a/src/backend/utils/activity/pgstat_shmem.c b/src/backend/utils/activity/pgstat_shmem.c
index 89060ef29a0..ac989186884 100644
--- a/src/backend/utils/activity/pgstat_shmem.c
+++ b/src/backend/utils/activity/pgstat_shmem.c
@@ -579,6 +579,22 @@ pgstat_lock_entry(PgStat_EntryRef *entry_ref, bool nowait)
return true;
}
+/*
+ * Separate from pgstat_lock_entry() as most callers will need to lock
+ * exclusively.
+ */
+bool
+pgstat_lock_entry_shared(PgStat_EntryRef *entry_ref, bool nowait)
+{
+ LWLock *lock = &entry_ref->shared_stats->lock;
+
+ if (nowait)
+ return LWLockConditionalAcquire(lock, LW_SHARED);
+
+ LWLockAcquire(lock, LW_SHARED);
+ return true;
+}
+
void
pgstat_unlock_entry(PgStat_EntryRef *entry_ref)
{
diff --git a/src/include/utils/pgstat_internal.h b/src/include/utils/pgstat_internal.h
index 9303d05427f..901d2041d66 100644
--- a/src/include/utils/pgstat_internal.h
+++ b/src/include/utils/pgstat_internal.h
@@ -581,6 +581,7 @@ extern void pgstat_detach_shmem(void);
extern PgStat_EntryRef *pgstat_get_entry_ref(PgStat_Kind kind, Oid dboid, Oid objoid,
bool create, bool *found);
extern bool pgstat_lock_entry(PgStat_EntryRef *entry_ref, bool nowait);
+extern bool pgstat_lock_entry_shared(PgStat_EntryRef *entry_ref, bool nowait);
extern void pgstat_unlock_entry(PgStat_EntryRef *entry_ref);
extern bool pgstat_drop_entry(PgStat_Kind kind, Oid dboid, Oid objoid);
extern void pgstat_drop_all_entries(void);