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

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