DSA_ALLOC_NO_OOM vs dsm_create ERROR leaving a half-initialized pgstats hash entry

On Tue, Sep 08, 2026 at 09:06:36AM +0000, Grigorev Jurij wrote:

I would rather not go back to PG_TRY/PG_CATCH around
pgstat_init_entry(); that was considered in [2] and dropped in favour
of returning NULL.

I've never been much on favor of TRY/CATCH generally for any kind of
low-level subsystem because it makes the stack manipulations more
complicated, sometimes with static states that need to be kept at
backend level (think threading). Being able to keep track of the
state across

Option (1) could close the pgstats corruption independently of the
lower-level question. Option (2) would make the NO_OOM behavior
consistent for other callers as well.

I can prepare the pgstats patch for (1), or investigate the DSA/DSM
approach first if you think that is the better layer. I can also add a
deterministic failure-injection test for the new-segment path.

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. (2) would show more value if there is a
gazillion of call sites that would like to care about more error
states when doing a DSA/DSM allocation.

I'd still tend to prefer (1) compared to (2) on top of my mind, but
I'm proved wrong a lot, so..
--
Michael

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

On Wed, Sep 09, 2026 at 10:56:34AM +0000, Grigorev Jurij wrote:

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.

Ah. I've missed the typcache.c thing previously. So this TRY/CATCH
pattern where we care about other error types than OOMs exist. Thanks
for pointing it out.

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.

Reading more through the patch.. I'm OK with the extra promise that
it brings: allocate first the chunk, then attempt an insert into the
shared hash table to not polute once we hold a chunk.

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.

Ahh.. You mean that inside the dshash_find_or_insert_extended(), if
dsm_create() itself fails, then we leak a DSA chunk previously
allocated. Yes, that's not a new thing. We could try to plumber
something inside dsm_create() but I take it as a cost/balance issue
because a a TRY/CATCH block is not completely free. On a very
unlikely failure, if I get you right, it means that we just leak some
memory. I'd take that leak over a shared memory state corruption all
the time taking down the cluster. Your patch is still an improvement:
we don't globally maintain an inconsistent shared memory state
anymore.

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().

I've pondered about that. A DSA allocation error while having
inconsistent data in the shared hash table means pollution across the
whole cluster. I think that this warrants a backpatch for the same
reason as 8191e0c16a03: it is not limited to a backend-level static
state. One allocation error can bring the whole cluster down. That's
not cool. If you can produce some patches down to v15, that would
speed up my work looking at all these branches, for sure.

In the stats read path, dshash_find_or_insert_extended() combined with
pgstat_alloc_entry_body() feels kind of nice. On OOM, we get nicer
reports. Under other failures, ERRORs are upgraded to FATAL. Not
perfect as it would lack context, still OK. By the way, we don't
really need to care about this code path if we get a failure due to
the previous argument, as a ERROR->FATAL just brings the server down
when the stats are read, taking down shared memory while on it.
Accomodating the stats read path with the redesign of
pgstat_init_entry() makes sense to me anyway: we want callers to give
a pre-allocated DSA chunk, let the caller deal with any cleanup errors
during the DSA allocation.

+       /*
+        * 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);

Hmm. This still leaves a local entry_ref if pgstat_alloc_entry_body()
itself fails. Compared to the case of a corrupted shmem area. I think
that I can live with that. And if I'm reading that right, the backend
reference that may still be around self-heals on re-entry if a backend
tries to insert again the same entry?

+ LWLockInitialize(&shheader->lock, LWTRANCHE_PGSTATS_DATA);

-	/* Link the new entry from the hash entry. */
+	pg_atomic_init_u32(&shhashent->refcount, 1);
[...] 
-	LWLockInitialize(&shheader->lock, LWTRANCHE_PGSTATS_DATA);

Why is this LWLockInitialize() moved around?

+                       dsa_free(pgStatLocal.dsa, chunk);
+                       dshash_release_lock(pgStatLocal.shared_hash, p);

In the "don't allow duplicate entries" case of
pgstat_read_statsfile(), doing a dsa_free() while holding the dshash
lock is just wasteful. There should be no concurrent activity in this
code path, which is OK in practice; that's just wasteful.
--
Michael

On Thu, Sep 17, 2026 at 11:48:26AM +0900, Michael Paquier wrote:

+       /*
+        * 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);

Hmm. This still leaves a local entry_ref if pgstat_alloc_entry_body()
itself fails. Compared to the case of a corrupted shmem area. I think
that I can live with that. And if I'm reading that right, the backend
reference that may still be around self-heals on re-entry if a backend
tries to insert again the same entry?

Ah.  4069df21beb8 points exactly at that case.  Perhaps we should
extend pgstat_gc_entry_refs() so as it is able to handle gracefully a
partial reference then?  I would imagine something like that, that
forces a release of the local entry if we don't have a shared_entry,
as of:
--- a/src/backend/utils/activity/pgstat_shmem.c
+++ b/src/backend/utils/activity/pgstat_shmem.c
@@ -839,6 +839,15 @@ pgstat_gc_entry_refs(void)
         Assert(!entry_ref->shared_stats ||
                entry_ref->shared_stats->magic == 0xdeadbeef);

+        /* NULL shared_entry marks a partial reference */
+        if (entry_ref->shared_entry == NULL)
+        {
+            Assert(entry_ref->shared_stats == NULL);
+            Assert(entry_ref->pending == NULL);
+            pgstat_release_entry_ref(ent->key, entry_ref, false);
+            continue;
+        }

What do you think about the attached? That would be an independent
safety measure.
--
Michael

Hi Michael,

Thanks for the review!

Hmm. This still leaves a local entry_ref if
pgstat_alloc_entry_body() itself fails. Compared to the case of a
corrupted shmem area. I think that I can live with that. And if I'm
reading that right, the backend reference that may still be around
self-heals on re-entry if a backend tries to insert again the same
entry?

There are two failure cases here. If pgstat_alloc_entry_body() returns
InvalidDsaPointer, the attached patch releases the local entry_ref before
raising ERROR. If dsm_create() raises ERROR from inside the allocation,
that cleanup is bypassed and the partial local reference remains.

Yes, a later lookup of the same key can reuse it:
pgstat_get_entry_ref_cached() treats shared_stats == NULL as a cache miss
and retries initialization. However, pgstat_gc_entry_refs() can run
before such a retry and currently dereferences shared_entry
unconditionally.

The check you proposed in the follow-up [1]/messages/by-id/aqtiKTvl519bu8-V@paquier.xyz looks right to me. A partial
reference at this point cannot have shared_stats or pending data, and
pgstat_release_entry_ref() can remove it without touching shared state.
It handles the backend-local partial reference, while this patch prevents
the same failure from leaving an inconsistent shared hash entry. I agree
that the two changes should remain independent, as you suggested.

Why is this LWLockInitialize() moved around?

There was no intended semantic change there; it was just unnecessary
movement while splitting allocation from initialization. v2 restores
LWLockInitialize() to its original position.

And for the duplicate stats-file entry path:

doing a dsa_free() while holding the dshash lock is just wasteful.

Agreed. v2 releases the dshash lock before freeing the unused chunk in
that path.

The attached v2 is rebased, and contains those review changes.
It passes a full assertions-enabled build and all 239 core regression tests.

I have prepared and build-tested versions for the supported branches
down to REL_15_STABLE, but I am not attaching them yet so that the HEAD
change can be reviewed first. I can send them once the shape of this
patch is agreed.

Do you think this warrants a deterministic test?

A low-level injection point in make_new_segment() would reproduce the
actual escaped ERROR, but the test would also need to force this
particular DSA area to allocate a new segment. A pgstats-specific
injection point immediately before the body allocation would be much
more deterministic, but it would model the ERROR rather than exercise
dsm_create() itself.

I left test-only instrumentation out of v2 for now. I can add one of
these approaches if you think the extra coverage is worth it.

Regards,
Yuriy

[1]: /messages/by-id/aqtiKTvl519bu8-V@paquier.xyz

On Thu, Sep 17, 2026 at 06:19:19AM +0000, Grigorev Jurij wrote:

The check you proposed in the follow-up [1] looks right to me. A partial
reference at this point cannot have shared_stats or pending data, and
pgstat_release_entry_ref() can remove it without touching shared state.
It handles the backend-local partial reference, while this patch prevents
the same failure from leaving an inconsistent shared hash entry. I agree
that the two changes should remain independent, as you suggested.

My second patch is an extra defense.. I guess that we should just do
that first.

I have prepared and build-tested versions for the supported branches
down to REL_15_STABLE, but I am not attaching them yet so that the HEAD
change can be reviewed first. I can send them once the shape of this
patch is agreed.

Something that I did not consider yet is how much we should worry
about the ABI change of pgstat_init_entry().. I could see nothing in
the open that uses it, but the risk seems non-zero. Perhaps we should
just limit that on HEAD.

Do you think this warrants a deterministic test?

Nah. That feels like a waste of test cycles for what's already a very
narrow case.
--
Michael

Agreed on all three points!

The two changes touch separate parts of pgstat_shmem.c, so the current
HEAD patch should apply cleanly on top of the defensive change. I do
not expect any conflicts, but I will rebase and resend it if needed.

And yeah, regarding the ABI concern, even though pgstat_init_entry()
is declared in pgstat_internal.h and I found no external users, changing
its signature in stable branches does not seem worth the compatibility
risk. I therefore agree that this patch should be limited to HEAD.

And agreed, no injection test for this narrow path!

Thanks,
Yuriy