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

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