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
Back-through: 15

diff --git a/src/backend/utils/activity/pgstat_shmem.c b/src/backend/utils/activity/pgstat_shmem.c
index c1b7ff76b129fb922a3092a68b17ef2c2c456a7d..041c262b9fbae88bc7eec3409084dd959cdb820f 100644 (file)
--- a/src/backend/utils/activity/pgstat_shmem.c
+++ b/src/backend/utils/activity/pgstat_shmem.c
@@ -304,6 +304,11 @@ pgstat_init_entry(PgStat_Kind kind,
     * further if a longer lived reference is needed.
     */
    pg_atomic_init_u32(&shhashent->refcount, 1);
+
+   /*
+    * Initialize "generation" to 0, as freshly created.
+    */
+   pg_atomic_init_u32(&shhashent->generation, 0);
    shhashent->dropped = false;
 
    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)
 
    /* mark as not dropped anymore */
    pg_atomic_fetch_add_u32(&shhashent->refcount, 1);
+
+   /*
+    * Increment "generation", to let any backend with local references know
+    * that what they point to is outdated.
+    */
+   pg_atomic_fetch_add_u32(&shhashent->generation, 1);
    shhashent->dropped = false;
 
    /* reinitialize content */
@@ -367,6 +378,7 @@ pgstat_acquire_entry_ref(PgStat_EntryRef *entry_ref,
 
    entry_ref->shared_stats = shheader;
    entry_ref->shared_entry = shhashent;
+   entry_ref->generation = pg_atomic_read_u32(&shhashent->generation);
 }
 
 /*
@@ -532,7 +544,8 @@ pgstat_get_entry_ref(PgStat_Kind kind, Oid dboid, uint64 objid, bool create,
             * case are replication slot stats, where a new slot can be
             * created with the same index just after dropping. But oid
             * wraparound can lead to other cases as well. We just reset the
-            * stats to their plain state.
+            * stats to their plain state, while incrementing its "generation"
+            * in the shared entry for any remaining local references.
             */
            shheader = pgstat_reinit_entry(kind, shhashent);
            pgstat_acquire_entry_ref(entry_ref, shhashent, shheader);
@@ -599,10 +612,27 @@ pgstat_release_entry_ref(PgStat_HashKey key, PgStat_EntryRef *entry_ref,
            if (!shent)
                elog(ERROR, "could not find just referenced shared stats entry");
 
-           Assert(pg_atomic_read_u32(&entry_ref->shared_entry->refcount) == 0);
-           Assert(entry_ref->shared_entry == shent);
-
-           pgstat_free_entry(shent, NULL);
+           /*
+            * This entry may have been reinitialized while trying to release
+            * it, so double-check that it has not been reused while holding a
+            * lock on its shared entry.
+            */
+           if (pg_atomic_read_u32(&entry_ref->shared_entry->generation) ==
+               entry_ref->generation)
+           {
+               /* Same "generation", so we're OK with the removal */
+               Assert(pg_atomic_read_u32(&entry_ref->shared_entry->refcount) == 0);
+               Assert(entry_ref->shared_entry == shent);
+               pgstat_free_entry(shent, NULL);
+           }
+           else
+           {
+               /*
+                * Shared stats entry has been reinitialized, so do not drop
+                * its shared entry, only release its lock.
+                */
+               dshash_release_lock(pgStatLocal.shared_hash, shent);
+           }
        }
    }
 

diff --git a/src/include/utils/pgstat_internal.h b/src/include/utils/pgstat_internal.h
index 61b2e1f96b259e3572d3e93d8f879ffb5da4b93e..437db06910b4199d6db2f4684332a33864977da9 100644 (file)
--- a/src/include/utils/pgstat_internal.h
+++ b/src/include/utils/pgstat_internal.h
@@ -94,6 +94,19 @@ typedef struct PgStatShared_HashEntry
     */
    pg_atomic_uint32 refcount;
 
+   /*
+    * Counter tracking the number of times the entry has been reused.
+    *
+    * Set to 0 when the entry is created, and incremented by one each time
+    * the shared entry is reinitialized with pgstat_reinit_entry().
+    *
+    * May only be incremented / decremented while holding at least a shared
+    * lock on the dshash partition containing the entry. Like refcount, it
+    * needs to be an atomic variable because multiple backends can increment
+    * the generation with just a shared lock.
+    */
+   pg_atomic_uint32 generation;
+
    /*
     * Pointer to shared stats. The stats entry always starts with
     * PgStatShared_Common, embedded in a larger struct containing the
@@ -133,6 +146,12 @@ typedef struct PgStat_EntryRef
     */
    PgStatShared_Common *shared_stats;
 
+   /*
+    * Copy of PgStatShared_HashEntry->generation, keeping locally track of
+    * the shared stats entry "generation" retrieved (number of times reused).
+    */
+   uint32      generation;
+
    /*
     * Pending statistics data that will need to be flushed to shared memory
     * stats eventually. Each stats kind utilizing pending data defines what