Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
Skip to content

Commit 818119a

Browse files
committed
Fix race conditions with drop of reused pgstats entries
This fixes a set of race conditions with cumulative statistics where a shared stats entry could be dropped while it should still be valid in the event when it is reused: an entry may refer to a different object but requires the same hash key. This can happen with various stats kinds, like: - Replication slots that compute internally an index number, for different slot names. - Stats kinds that use an OID in the object key, where a wraparound causes the same key to be used if an OID is used for the same object. - As of PostgreSQL 18, custom pgstats kinds could also be an issue, depending on their implementation. This issue is fixed by introducing a counter called "generation" in the shared entries via PgStatShared_HashEntry, initialized at 0 when an entry is created and incremented when the same entry is reused, to avoid concurrent issues on drop because of other backends still holding a reference to it. This "generation" is copied to the local copy that a backend holds when looking at an object, then cross-checked with the shared entry to make sure that the entry is not dropped even if its "refcount" justifies that if it has been reused. This problem could show up when a backend shuts down and needs to discard any entries it still holds, causing statistics to be removed when they should not, or even an assertion failure. Another report involved a failure in a standby after an OID wraparound, where the startup process would FATAL on a "can only drop stats once", stopping recovery abruptly. The buildfarm has been sporadically complaining about the problem, as well, but the window is hard to reach with the in-core tests. Note that the issue can be reproduced easily by adding a sleep before dshash_find() in pgstat_release_entry_ref() to enlarge the problematic window while repeating test_decoding's isolation test oldest_xmin a couple of times, for example, as pointed out by Alexander Lakhin. Reported-by: Alexander Lakhin, Peter Smith Author: Kyotaro Horiguchi, Michael Paquier Reviewed-by: Bertrand Drouvot Discussion: https://postgr.es/m/CAA4eK1KxuMVyAryz_Vk5yq3ejgKYcL6F45Hj9ZnMNBS-g+PuZg@mail.gmail.com Discussion: https://postgr.es/m/17947-b9554521ad963c9c@postgresql.org Backpatch-through: 15
1 parent 5b00786 commit 818119a

File tree

2 files changed

+54
-5
lines changed

2 files changed

+54
-5
lines changed

src/backend/utils/activity/pgstat_shmem.c

+35-5
Original file line numberDiff line numberDiff line change
@@ -304,6 +304,11 @@ pgstat_init_entry(PgStat_Kind kind,
304304
* further if a longer lived reference is needed.
305305
*/
306306
pg_atomic_init_u32(&shhashent->refcount, 1);
307+
308+
/*
309+
* Initialize "generation" to 0, as freshly created.
310+
*/
311+
pg_atomic_init_u32(&shhashent->generation, 0);
307312
shhashent->dropped = false;
308313

309314
chunk = dsa_allocate0(pgStatLocal.dsa, pgstat_get_kind_info(kind)->shared_size);
@@ -327,6 +332,12 @@ pgstat_reinit_entry(PgStat_Kind kind, PgStatShared_HashEntry *shhashent)
327332

328333
/* mark as not dropped anymore */
329334
pg_atomic_fetch_add_u32(&shhashent->refcount, 1);
335+
336+
/*
337+
* Increment "generation", to let any backend with local references know
338+
* that what they point to is outdated.
339+
*/
340+
pg_atomic_fetch_add_u32(&shhashent->generation, 1);
330341
shhashent->dropped = false;
331342

332343
/* reinitialize content */
@@ -367,6 +378,7 @@ pgstat_acquire_entry_ref(PgStat_EntryRef *entry_ref,
367378

368379
entry_ref->shared_stats = shheader;
369380
entry_ref->shared_entry = shhashent;
381+
entry_ref->generation = pg_atomic_read_u32(&shhashent->generation);
370382
}
371383

372384
/*
@@ -532,7 +544,8 @@ pgstat_get_entry_ref(PgStat_Kind kind, Oid dboid, uint64 objid, bool create,
532544
* case are replication slot stats, where a new slot can be
533545
* created with the same index just after dropping. But oid
534546
* wraparound can lead to other cases as well. We just reset the
535-
* stats to their plain state.
547+
* stats to their plain state, while incrementing its "generation"
548+
* in the shared entry for any remaining local references.
536549
*/
537550
shheader = pgstat_reinit_entry(kind, shhashent);
538551
pgstat_acquire_entry_ref(entry_ref, shhashent, shheader);
@@ -599,10 +612,27 @@ pgstat_release_entry_ref(PgStat_HashKey key, PgStat_EntryRef *entry_ref,
599612
if (!shent)
600613
elog(ERROR, "could not find just referenced shared stats entry");
601614

602-
Assert(pg_atomic_read_u32(&entry_ref->shared_entry->refcount) == 0);
603-
Assert(entry_ref->shared_entry == shent);
604-
605-
pgstat_free_entry(shent, NULL);
615+
/*
616+
* This entry may have been reinitialized while trying to release
617+
* it, so double-check that it has not been reused while holding a
618+
* lock on its shared entry.
619+
*/
620+
if (pg_atomic_read_u32(&entry_ref->shared_entry->generation) ==
621+
entry_ref->generation)
622+
{
623+
/* Same "generation", so we're OK with the removal */
624+
Assert(pg_atomic_read_u32(&entry_ref->shared_entry->refcount) == 0);
625+
Assert(entry_ref->shared_entry == shent);
626+
pgstat_free_entry(shent, NULL);
627+
}
628+
else
629+
{
630+
/*
631+
* Shared stats entry has been reinitialized, so do not drop
632+
* its shared entry, only release its lock.
633+
*/
634+
dshash_release_lock(pgStatLocal.shared_hash, shent);
635+
}
606636
}
607637
}
608638

src/include/utils/pgstat_internal.h

+19
Original file line numberDiff line numberDiff line change
@@ -94,6 +94,19 @@ typedef struct PgStatShared_HashEntry
9494
*/
9595
pg_atomic_uint32 refcount;
9696

97+
/*
98+
* Counter tracking the number of times the entry has been reused.
99+
*
100+
* Set to 0 when the entry is created, and incremented by one each time
101+
* the shared entry is reinitialized with pgstat_reinit_entry().
102+
*
103+
* May only be incremented / decremented while holding at least a shared
104+
* lock on the dshash partition containing the entry. Like refcount, it
105+
* needs to be an atomic variable because multiple backends can increment
106+
* the generation with just a shared lock.
107+
*/
108+
pg_atomic_uint32 generation;
109+
97110
/*
98111
* Pointer to shared stats. The stats entry always starts with
99112
* PgStatShared_Common, embedded in a larger struct containing the
@@ -133,6 +146,12 @@ typedef struct PgStat_EntryRef
133146
*/
134147
PgStatShared_Common *shared_stats;
135148

149+
/*
150+
* Copy of PgStatShared_HashEntry->generation, keeping locally track of
151+
* the shared stats entry "generation" retrieved (number of times reused).
152+
*/
153+
uint32 generation;
154+
136155
/*
137156
* Pending statistics data that will need to be flushed to shared memory
138157
* stats eventually. Each stats kind utilizing pending data defines what

0 commit comments

Comments
 (0)