Re: PostgreSQL 18.4 backend SIGSEGV in pgstat_gc_entry_refs() after caught DSM attach error

On Wed, Aug 05, 2026 at 12:56:09PM +0100, Niall Newman wrote:
> The first is intended as a discussion patch, not a claim that the preferred
> fix belongs at exactly these layers. It:
>
> * makes dsm_attach() undo its descriptor/reference after an OS mapping
> ERROR;
> * completes PgStat_EntryRef fields before releasing the dshash lock;
> * removes the local ref on the explicit pgstats DSA-allocation failure;
> * makes pgstats GC discard incomplete local references; and
> * includes the substance of 16ebc196 for REL_18_STABLE.

Honestly, I think that as presented this patch is a bit of a mess,
because you are doing a lot of things. This has been written by an
agent, I assume? Well, it feels like it got confused..

> The fix patch applies to both the REL_18_4 tag and current REL_18_STABLE. It
> passes git diff --check, compiles successfully on REL_18_STABLE, and passes
> the attached deterministic fault-injection TAP test. The 16ebc196 hunk is
> already present on master and would be omitted there.

There is a grand total of three bugs that you are reporting here, each
one of them with different consequences. In summary, I have the
following:
1) pgstat_acquire_entry_ref() and the assumption of an interrupt
processed can lead to a local refcount being never deflated, which is
as far as I understand a local memory leak. Sure, why not, but that's
minimal for a very narrow window.
2) pgstat_get_entry_ref() and its missed pgstat_release_entry_ref() is
an oversight of 8191e0c16a03, causing a possible NULL pointer
dereference in pgstat_gc_entry_refs() due to the local reference being
around on OOM, and not released.
3) The change in pgstat_gc_entry_refs() is a belt-and-suspender
change, assuming that the state of a NULL shared_stats could be
reached, but it becomes unnecessary once we fix 1), and at some degree
the next point. We could as well go without it.
4) The point behind dsm_attach() is well-received, but pgstats just
faces a consequence of something that we may want to handle better in
the DSA/DSM layer. I don't have a good idea whether what you are
suggesting is a good thing or not, but we could just treat it
separately.
5) While on it, I think that we could do something about the
dshash_find_or_insert() in pgstat_get_entry_ref(), where we could
clean up the local reference if dshash_find_or_insert() returns NULL.
That's content only worth on HEAD, as the problem is unlikely going to
happen in practice, and that's only a local reference.

To summarize, I have fixed 2) with 4069df21beb8 backpatched down to
v15, and did bf80a4c2d238 for 1) on HEAD. 3) is not necessary. 4)
should have its own discussion, as it's a broader change impacting
anything that uses DSA/DSM. And I am planning to apply the attached
for 5) only on HEAD as a follow-up improvement.
--
Michael