agora inbox for pgsql-hackers@postgresql.org
help / color / mirror / Atom feedFrom: Grigorev Jurij <ju.grigorev@ftdata.ru>
To: Michael Paquier <michael@paquier.xyz>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: DSA_ALLOC_NO_OOM vs dsm_create ERROR leaving a half-initialized pgstats hash entry
Date: Wed, 9 Sep 2026 10:56:34 +0000
Message-ID: <a72c99d5b1e448b191a89e30ecaf3e8a@localhost.localdomain> (raw)
In-Reply-To: <aqCfwe5-EIiJXlIu@paquier.xyz>
References: <ddc3ecfb01ce4e9698b23cc59767f016@localhost.localdomain>
<aqCfwe5-EIiJXlIu@paquier.xyz>
On Tue, Sep 08, 2026 at 11:52:33PM +0000, Michael Paquier wrote:
> My question regarding (1) vs (2) would be: do we have other
> sub-systems that display patterns similar to pgstats when it comes to
> the DSA/DSM failing? If pgstats is the only one, (1) sounds like a
> solution good enough for me.
I checked all in-tree DSA_ALLOC_NO_OOM and DSHASH_INSERT_NO_OOM call
sites, plus the comparable two-phase cases in typcache.c and async.c.
pgstats is the only one that both publishes its surrounding object
before allocating the DSA body and assumes the body is always valid.
With one exception, discussed below, they all allocate before
publishing. In dshash, insert_into_bucket() allocates the item before
linking it into a bucket, resize() allocates the new bucket array
before replacing the old one, and dshash_create() does not publish the
table until its buckets exist. pgsa_set_advice_string() in
contrib/pg_stash_advice allocates the advice string, then inserts with
DSHASH_INSERT_NO_OOM and frees the string if that returns NULL, which
is exactly the ordering the patch gives pgstats.
find_or_make_matching_shared_tupledesc() in typcache.c copies the
TupleDesc into DSA first, with a PG_TRY() around the insertion to free
it on error.
The exception, and the closest case to pgstats, is async.c:
PrepareTableEntriesForListen() inserts a channel entry with
listenersArray == InvalidDsaPointer and allocates afterwards. But that
incomplete state is supported by design -- numListeners stays zero, so
consumers do not access any array elements, and a later call retries
the allocation. pgstats has no such tolerance: the existing-entry path
dereferences body unconditionally, which is why the escaped error
becomes a SEGV instead of a retry.
So option (1) looks sufficient for this crash. Patch attached.
It allocates the DSA body before inserting the shared hash entry;
pgstat_init_entry() now receives a valid chunk and no longer allocates.
Both callers use dshash_find_or_insert_extended() with
DSHASH_INSERT_NO_OOM and free the preallocated body if insertion
returns NULL or finds an existing entry.
One residual case remains: dshash insertion can itself raise ERROR from
dsm_create() despite DSHASH_INSERT_NO_OOM, in which case the
preallocated chunk is not reclaimed. pg_stash_advice and
dshash_create() have analogous unreachable-allocation cases. No
inconsistent pgstats entry is published, but closing the leak would
require either exception cleanup or the lower-level NO_OOM change. I
left PG_TRY/PG_CATCH out based on your comments; typcache shows how it
could be used if such cleanup is preferred.
8191e0c was backpatched through 15 for the same class of corruption, so
this path may deserve the same treatment. If the approach looks right,
I can prepare back-branch versions and add a deterministic test using
an injection point in make_new_segment() before dsm_create().
Thanks,
Yuriy Grigoryev=
Attachments:
[application/octet-stream] 0001-Allocate-pgstats-entry-body-before-shared-hash-insert.patch (10.1K, ../a72c99d5b1e448b191a89e30ecaf3e8a@localhost.localdomain/2-0001-Allocate-pgstats-entry-body-before-shared-hash-insert.patch)
download | inline diff:
From d06792297a82fb0a0a1d8dbb1f4064c324cb485a Mon Sep 17 00:00:00 2001
From: Yuriy Grigoryev <ju.grigorev@ftdata.ru>
Date: Tue, 8 Sep 2026 13:30:18 +0700
Subject: [PATCH] Allocate pgstats entry body before shared hash insert
8191e0c 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 creating a new DSM segment raises ERROR (for example ENOSPC
on posix shared memory).
pgstat_init_entry() marked the hash entry live before allocating the
body. If dsm_create() failed after the insert, error unwind released
the dshash lock but left a visible entry with body == InvalidDsaPointer.
The next backend crashed in pgstat_acquire_entry_ref().
Split allocation from hash initialization: allocate the DSA chunk first,
insert the hash entry only with a valid body, and free the chunk if a
concurrent insert wins or the insert returns NULL. Both callers use
dshash_find_or_insert_extended() with DSHASH_INSERT_NO_OOM so that path
can free the preallocated body.
Discussion: https://postgr.es/m/ddc3ecfb01ce4e9698b23cc59767f016@localhost.localdomain
---
src/backend/utils/activity/pgstat.c | 45 ++++++----
src/backend/utils/activity/pgstat_shmem.c | 105 ++++++++++++----------
src/include/utils/pgstat_internal.h | 4 +-
3 files changed, 91 insertions(+), 63 deletions(-)
diff --git a/src/backend/utils/activity/pgstat.c b/src/backend/utils/activity/pgstat.c
index 5177f88..3f4fe60 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,12 +2096,39 @@ 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,
+ * so a dsm_create failure cannot leave a half-initialized
+ * shared entry behind.
*/
- p = dshash_find_or_insert(pgStatLocal.shared_hash, &key, &found);
+ chunk = pgstat_alloc_entry_body(key.kind);
+ if (chunk == InvalidDsaPointer)
+ {
+ /*
+ * 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);
+ }
+
+ p = dshash_find_or_insert_extended(pgStatLocal.shared_hash,
+ &key, &found,
+ DSHASH_INSERT_NO_OOM);
+ if (!p)
+ {
+ dsa_free(pgStatLocal.dsa, chunk);
+ 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)
{
+ dsa_free(pgStatLocal.dsa, chunk);
dshash_release_lock(pgStatLocal.shared_hash, p);
elog(WARNING, "found duplicate stats entry %u/%u/%" PRIu64 " of type %c",
key.kind, key.dboid,
@@ -2108,20 +2136,7 @@ pgstat_read_statsfile(void)
goto error;
}
- header = pgstat_init_entry(key.kind, p);
- if (header == NULL)
- {
- dshash_delete_entry(pgStatLocal.shared_hash, p);
-
- /*
- * 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);
- }
+ 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 d8ac9d6..bf886b4 100644
--- a/src/backend/utils/activity/pgstat_shmem.c
+++ b/src/backend/utils/activity/pgstat_shmem.c
@@ -303,53 +303,58 @@ 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 if the allocation fails without throwing. Call
+ * this before inserting a hash entry: dsa_allocate_extended() can still raise
+ * ERROR when creating a new DSM segment (for example ENOSPC), and doing that
+ * after the insert would leave a live hash entry with body ==
+ * InvalidDsaPointer.
+ */
+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 a newly-inserted hash entry around an already-allocated DSA
+ * body.
+ *
+ * The caller must hold the dshash partition lock. The entry cannot be found
+ * by other backends until that lock is released, so it is safe to publish
+ * refcount/dropped/body here. Caller needs to increment the refcount further
+ * if a longer-lived reference is needed.
+ *
+ * chunk must be a valid pointer from 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);
- /*
- * 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
- * long as we hold the dshash partition lock. Caller needs to increase
- * 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_allocate_extended(pgStatLocal.dsa,
- kind_info->shared_size,
- DSA_ALLOC_ZERO | DSA_ALLOC_NO_OOM);
- if (chunk == InvalidDsaPointer)
- return NULL;
+ Assert(DsaPointerIsValid(chunk));
shheader = dsa_get_address(pgStatLocal.dsa, chunk);
shheader->magic = 0xdeadbeef;
+ LWLockInitialize(&shheader->lock, LWTRANCHE_PGSTATS_DATA);
- /* Link the new entry from the hash entry. */
+ pg_atomic_init_u32(&shhashent->refcount, 1);
+ pg_atomic_init_u32(&shhashent->generation, 0);
+ shhashent->dropped = false;
shhashent->body = chunk;
/* Increment entry count, if required. */
if (kind_info->track_entry_count)
pg_atomic_fetch_add_u64(&pgStatLocal.shmem->entry_counts[kind - 1], 1);
- LWLockInitialize(&shheader->lock, LWTRANCHE_PGSTATS_DATA);
-
return shheader;
}
@@ -545,6 +550,24 @@ 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. Creating a
+ * new DSA segment can raise ERROR (e.g. ENOSPC on posix shm); doing
+ * that after the insert would leave a live hash entry with an
+ * invalid body.
+ */
+ 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 +579,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 +595,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 +603,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 14369e5..201e572 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);
/*
--
2.50.1 (Apple Git-155)
view thread (10+ messages) latest in thread
Message-ID: <a72c99d5b1e448b191a89e30ecaf3e8a@localhost.localdomain>
Permalink: ../a72c99d5b1e448b191a89e30ecaf3e8a@localhost.localdomain/
Also on: postgresql.org/message-id/a72c99d5b1e448b191a89e30ecaf3e8a@localhost.localdomain
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: ju.grigorev@ftdata.ru, michael@paquier.xyz, pgsql-hackers@lists.postgresql.org
Subject: Re: DSA_ALLOC_NO_OOM vs dsm_create ERROR leaving a half-initialized pgstats hash entry
In-Reply-To: <a72c99d5b1e448b191a89e30ecaf3e8a@localhost.localdomain>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox