Skip to content

Commit

Permalink
pgstat: Acquire lock when reading variable-numbered stats
Browse files Browse the repository at this point in the history
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-
  • Loading branch information
anarazel committed Aug 23, 2022
1 parent a2caf18 commit 045ec34
Show file tree
Hide file tree
Showing 3 changed files with 26 additions and 0 deletions.
9 changes: 9 additions & 0 deletions src/backend/utils/activity/pgstat.c
Expand Up @@ -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)
{
Expand Down Expand Up @@ -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);

Expand Down
16 changes: 16 additions & 0 deletions src/backend/utils/activity/pgstat_shmem.c
Expand Up @@ -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)
{
Expand Down
1 change: 1 addition & 0 deletions src/include/utils/pgstat_internal.h
Expand Up @@ -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);
Expand Down

0 comments on commit 045ec34

Please sign in to comment.