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] 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] https://postgr.es/m/aqtiKTvl519bu8-V%40paquier.xyz
v2-0001-Allocate-pgstats-entry-body-before-shared-hash-insert.patch
Description: v2-0001-Allocate-pgstats-entry-body-before-shared-hash-insert.patch
