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

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