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
signature.asc
Description: PGP signature
