Missing dshash cleanup in pgstat_read_statsfile() after OOM

Hi,

While investigating an AddressSanitizer crash in
pgstat_acquire_entry_ref(), I found what looks like an incomplete part of
the OOM cleanup added by 8191e0c16a03.

This is a follow-up to the September 2025 discussion started here:

https://postgr.es/m/CAAi9E7jELo5_-sBENftnc2E8XhW2PKZJWfTC3i2y-GMQd2bcqQ@mail.gmail.com

The original problem was that pgstat_init_entry() could fail while
allocating its DSA object after an entry had already been inserted into
the shared hash. This left a half-initialized entry in the hash.

The discussion considered changing the order of initialization or
having pgstat_init_entry() report the allocation failure to its callers:

https://postgr.es/m/aLfpyYaQ0g8i4R_m@paquier.xyz
https://postgr.es/m/aLlAym4DHW4PM8Gg@paquier.xyz

The latter approach was committed as 8191e0c16a03. pgstat_init_entry()
now uses DSA_ALLOC_NO_OOM and returns NULL when the allocation fails, so
that its caller can remove the newly inserted hash entry before raising
an error.

There are two callers of pgstat_init_entry(). The normal entry creation
path in pgstat_get_entry_ref() performs that cleanup:

shheader = pgstat_init_entry(kind, shhashent);
if (shheader == NULL)
{
...
dshash_delete_entry(pgStatLocal.shared_hash, shhashent);
ereport(ERROR, ...);
}

However, the stats-file restore path in pgstat_read_statsfile() releases
the dshash lock and raises ERROR without deleting the entry:

header = pgstat_init_entry(key.kind, p);
dshash_release_lock(pgStatLocal.shared_hash, p);
if (header == NULL)
elog(ERROR, ...);

At that point, the entry has already been inserted and initialized with
dropped=false, refcount=1 and generation=0, but its body is still
InvalidDsaPointer. After the lock is released, that half-initialized
entry remains in the shared hash rather than being removed on the
allocation failure.

The ERROR-versus-WARNING behavior of pgstat_read_statsfile() was
discussed explicitly:

https://postgr.es/m/aLuH5D1xqHl3TJoA@paquier.xyz

I agree with the decision to keep ERROR here. The issue is only that
the newly inserted hash entry should be removed before raising it, as
already done by the other caller.

The attached patch changes the failure path to:

header = pgstat_init_entry(key.kind, p);
if (header == NULL)
{
dshash_delete_entry(pgStatLocal.shared_hash, p);
elog(ERROR, ...);
}
dshash_release_lock(pgStatLocal.shared_hash, p);

dshash_delete_entry() releases the partition lock itself, so the normal
dshash_release_lock() is reached only after successful initialization.

I originally noticed this while investigating an ASan SEGV where gdb
showed a live shared hash entry with body == InvalidDsaPointer. The
affected entries were shared relation statistics for pg_authid and
pg_database. The resulting NULL from dsa_get_address() was later
dereferenced in pgstat_acquire_entry_ref().

I cannot prove that pgstat_read_statsfile() created those particular
entries. On a clean startup, an ERROR while restoring the stats file
normally prevents the cluster from continuing. The same hash-entry
state could also be left if a process were terminated inside
pgstat_init_entry(), after the entry had been marked live but before its
body was assigned. The normal creation path does remove the entry when
pgstat_init_entry() returns NULL.

The ASan crash was therefore what led me to inspect this invariant,
rather than a reproducer specifically for the stats-file restore path.
Still, pgstat_read_statsfile() leaves the shared hash inconsistent if
pgstat_init_entry() returns NULL, contrary to the cleanup model
introduced by 8191e0c.

The patch is intentionally limited to making the restore caller match
the existing creation path. It does not change the ERROR behavior or
add defensive NULL checks to readers.

Thoughts?

Thanks,
Yuriy Grigoryev