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 <[email protected]>
Reviewed-by: "Drouvot, Bertrand" <[email protected]>
Reviewed-by: Andres Freund <[email protected]>
Author: Kyotaro Horiguchi <[email protected]>
Discussion: https://postgr.es/m/CAM-w4HMYkM_DkYhWtUGV+qE_rrBxKOzOF0+5faozxO3vXrc9wA@mail.gmail.com
Backpatch: 15-

diff --git a/src/backend/utils/activity/pgstat.c b/src/backend/utils/activity/pgstat.c
index 88e5dd1b2b72b2f81718d9b65d32c324462196d2..6224c498c21d0ee129907c7f730270e99e4a6fcd 100644 (file)
--- 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 89060ef29a01610c1af7ab2e5ae400e803cb9d74..ac9891868847f29508178bf026ad03ac4183c2c5 100644 (file)
--- 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 9303d05427f116c67f313c0a87f1dfba6dc4de84..901d2041d66a89ea89c8e113883454fb8d9f007a 100644 (file)
--- 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);