postgres.git / summary / log / commit / refs
commit d9de60c5e47a7f7a5dcee050d119bda08ea85d55
Author: Michael Paquier <michael@paquier.xyz>
Date: Wed Sep 23 23:48:56 2026 +0000
Allocate pgstats entry body before insert into shared hashtable
8191e0c16a03 made pgstat_init_entry() return NULL on DSA allocation
failure so callers could delete a just-inserted hash entry. That works
when dsa_allocate_extended(DSA_ALLOC_NO_OOM) returns InvalidDsaPointer,
but not when the creation of a new DSM segment raises ERROR.
Under this type of failure, pgstat_init_entry() would mark the hash
entry live before allocating the body and cause an inconsistent state in
shared memory. The next backend trying to acquire the existing,
partially-initialized entry in pgstat_acquire_entry_ref() would crash,
taking down the server. Some of the public reports mentioned ENOSPC on
POSIX shared memory as one initial cause.
This commit refactors the pgstats entry initialization to happen in two
phases:
- Allocate the DSA chunk.
- Insert the hash entry with a valid segment.
If a concurrent insert wins, the allocation of the first phase needs an
extra set of dsa_free() calls (pulling back a DSM into its free list).
As this changes the signature of pgstat_init_entry(), no backpatch is
done.
Author: Yuriy Grigoryev <ju.grigorev@ftdata.ru>
Author: Michael Paquier <michael@paquier.xyz>
Discussion: https://postgr.es/m/ddc3ecfb01ce4e9698b23cc59767f016@localhost.localdomain
src/backend/utils/activity/pgstat.c | 46 ++++++++++++++------
src/backend/utils/activity/pgstat_shmem.c | 72 ++++++++++++++++++-------------
src/include/utils/pgstat_internal.h | 4 +-
3 files changed, 76 insertions(+), 46 deletions(-)
diff --git a/src/backend/utils/activity/pgstat.c b/src/backend/utils/activity/pgstat.c
index 5177f880f70..6dd13ab9dec 100644
--- a/src/backend/utils/activity/pgstat.c
+++ b/src/backend/utils/activity/pgstat.c
@@ -2007,6 +2007,7 @@ pgstat_read_statsfile(void)
PgStatShared_HashEntry *p;
PgStatShared_Common *header;
const PgStat_KindInfo *kind_info = NULL;
+ dsa_pointer chunk;
CHECK_FOR_INTERRUPTS();
@@ -2095,33 +2096,50 @@ pgstat_read_statsfile(void)
* This intentionally doesn't use pgstat_get_entry_ref() -
* putting all stats into checkpointer's
* pgStatEntryRefHash would be wasted effort and memory.
+ *
+ * Allocate the DSA body before inserting the hash entry.
*/
- p = dshash_find_or_insert(pgStatLocal.shared_hash, &key, &found);
-
- /* don't allow duplicate entries */
- if (found)
+ chunk = pgstat_alloc_entry_body(key.kind);
+ if (chunk == InvalidDsaPointer)
{
- dshash_release_lock(pgStatLocal.shared_hash, p);
- elog(WARNING, "found duplicate stats entry %u/%u/%" PRIu64 " of type %c",
+ /*
+ * It would be tempting to switch this ERROR to a
+ * WARNING, but it would mean that all the statistics
+ * are discarded when the environment fails on OOM.
+ */
+ elog(ERROR, "could not allocate entry %u/%u/%" PRIu64 " of type %c",
key.kind, key.dboid,
key.objid, t);
- goto error;
}
- header = pgstat_init_entry(key.kind, p);
- if (header == NULL)
+ p = dshash_find_or_insert_extended(pgStatLocal.shared_hash,
+ &key, &found,
+ DSHASH_INSERT_NO_OOM);
+ if (!p)
{
- dshash_delete_entry(pgStatLocal.shared_hash, p);
+ dsa_free(pgStatLocal.dsa, chunk);
/*
- * It would be tempting to switch this ERROR to a
- * WARNING, but it would mean that all the statistics
- * are discarded when the environment fails on OOM.
+ * for the same reason as previously, ERROR not
+ * WARNING
*/
- elog(ERROR, "could not allocate entry %u/%u/%" PRIu64 " of type %c",
+ elog(ERROR, "could not insert entry %u/%u/%" PRIu64 " of type %c",
key.kind, key.dboid,
key.objid, t);
}
+
+ /* don't allow duplicate entries */
+ if (found)
+ {
+ dshash_release_lock(pgStatLocal.shared_hash, p);
+ dsa_free(pgStatLocal.dsa, chunk);
+ elog(WARNING, "found duplicate stats entry %u/%u/%" PRIu64 " of type %c",
+ key.kind, key.dboid,
+ key.objid, t);
+ goto error;
+ }
+
+ header = pgstat_init_entry(key.kind, p, chunk);
dshash_release_lock(pgStatLocal.shared_hash, p);
if (!read_chunk(fpin,
diff --git a/src/backend/utils/activity/pgstat_shmem.c b/src/backend/utils/activity/pgstat_shmem.c
index 5feed5f842a..1858d68f918 100644
--- a/src/backend/utils/activity/pgstat_shmem.c
+++ b/src/backend/utils/activity/pgstat_shmem.c
@@ -303,21 +303,36 @@ pgstat_detach_shmem(void)
*/
/*
- * Initialize entry newly-created.
+ * Allocate the DSA body for a new variable-numbered pgstats entry.
*
- * Returns NULL in the event of an allocation failure, so as callers can
- * take cleanup actions as the entry initialized is already inserted in the
- * shared hashtable.
+ * Returns InvalidDsaPointer in the event of an allocation failure.
+ */
+dsa_pointer
+pgstat_alloc_entry_body(PgStat_Kind kind)
+{
+ const PgStat_KindInfo *kind_info = pgstat_get_kind_info(kind);
+
+ return dsa_allocate_extended(pgStatLocal.dsa,
+ kind_info->shared_size,
+ DSA_ALLOC_ZERO | DSA_ALLOC_NO_OOM);
+}
+
+/*
+ * Initialize variable-numbered pgstats entry.
+ *
+ * "chunk" must be a valid pointer, allocated previously by
+ * pgstat_alloc_entry_body().
*/
PgStatShared_Common *
pgstat_init_entry(PgStat_Kind kind,
- PgStatShared_HashEntry *shhashent)
+ PgStatShared_HashEntry *shhashent,
+ dsa_pointer chunk)
{
- /* Create new stats entry. */
- dsa_pointer chunk;
PgStatShared_Common *shheader;
const PgStat_KindInfo *kind_info = pgstat_get_kind_info(kind);
+ Assert(DsaPointerIsValid(chunk));
+
/*
* Initialize refcount to 1, marking it as valid / not dropped. The entry
* can't be freed before the initialization because it can't be found as
@@ -332,12 +347,6 @@ pgstat_init_entry(PgStat_Kind kind,
pg_atomic_init_u32(&shhashent->generation, 0);
shhashent->dropped = false;
- chunk = dsa_allocate_extended(pgStatLocal.dsa,
- kind_info->shared_size,
- DSA_ALLOC_ZERO | DSA_ALLOC_NO_OOM);
- if (chunk == InvalidDsaPointer)
- return NULL;
-
shheader = dsa_get_address(pgStatLocal.dsa, chunk);
shheader->magic = 0xdeadbeef;
@@ -545,6 +554,19 @@ pgstat_get_entry_ref(PgStat_Kind kind, Oid dboid, uint64 objid, bool create,
if (create && !shhashent)
{
bool shfound;
+ dsa_pointer chunk;
+
+ /* Allocate the stats body before inserting a hash entry. */
+ chunk = pgstat_alloc_entry_body(kind);
+ if (chunk == InvalidDsaPointer)
+ {
+ pgstat_release_entry_ref(key, entry_ref, false);
+ ereport(ERROR,
+ (errcode(ERRCODE_OUT_OF_MEMORY),
+ errmsg("out of memory"),
+ errdetail("Failed while allocating entry %u/%u/%" PRIu64 ".",
+ key.kind, key.dboid, key.objid)));
+ }
/*
* It's possible that somebody created the entry since the above
@@ -556,6 +578,8 @@ pgstat_get_entry_ref(PgStat_Kind kind, Oid dboid, uint64 objid, bool create,
DSHASH_INSERT_NO_OOM);
if (!shhashent)
{
+ dsa_free(pgStatLocal.dsa, chunk);
+
/*
* Clean up the local reference when failing insert into the
* shared hashtable.
@@ -570,24 +594,7 @@ pgstat_get_entry_ref(PgStat_Kind kind, Oid dboid, uint64 objid, bool create,
if (!shfound)
{
- shheader = pgstat_init_entry(kind, shhashent);
- if (shheader == NULL)
- {
- /*
- * Failed the allocation of a new entry, so clean up both the
- * local reference and the shared hashtable before giving up.
- * Clean the local state first, since releasing the dshash
- * lock can process a pending interrupt.
- */
- pgstat_release_entry_ref(key, entry_ref, false);
- dshash_delete_entry(pgStatLocal.shared_hash, shhashent);
-
- ereport(ERROR,
- (errcode(ERRCODE_OUT_OF_MEMORY),
- errmsg("out of memory"),
- errdetail("Failed while allocating entry %u/%u/%" PRIu64 ".",
- key.kind, key.dboid, key.objid)));
- }
+ shheader = pgstat_init_entry(kind, shhashent, chunk);
pgstat_acquire_entry_ref(entry_ref, shhashent, shheader);
if (created_entry != NULL)
@@ -595,6 +602,9 @@ pgstat_get_entry_ref(PgStat_Kind kind, Oid dboid, uint64 objid, bool create,
return entry_ref;
}
+
+ /* Concurrent insert won; drop the unused body. */
+ dsa_free(pgStatLocal.dsa, chunk);
}
if (!shhashent)
diff --git a/src/include/utils/pgstat_internal.h b/src/include/utils/pgstat_internal.h
index 14369e59a1c..201e57279b1 100644
--- a/src/include/utils/pgstat_internal.h
+++ b/src/include/utils/pgstat_internal.h
@@ -839,8 +839,10 @@ extern void pgstat_reset_matching_entries(bool (*do_reset) (PgStatShared_HashEnt
TimestampTz ts);
extern void pgstat_request_entry_refs_gc(void);
+extern dsa_pointer pgstat_alloc_entry_body(PgStat_Kind kind);
extern PgStatShared_Common *pgstat_init_entry(PgStat_Kind kind,
- PgStatShared_HashEntry *shhashent);
+ PgStatShared_HashEntry *shhashent,
+ dsa_pointer chunk);
/*