Re: Missing dshash cleanup in pgstat_read_statsfile() after OOM

On Mon, Sep 07, 2026 at 02:52:59PM +0000, Grigorev Jurij wrote:
> 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.

Well, this addition is not really necessary. When we read the stats
from disk, we are in the startup process and an ERROR would upgrade to
a FATAL, and shared memory gets reinitialized. So it is not possible
to observe a half-baked entry as far as I know for this path, at least
as coded on HEAD.

> 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.

That points to a different issue to me. If you see something going
on, please feel free, but I have not heard of other similar reports
yet.

> 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.

Saying that, there is I guess an argument of consistency with the
entry creation path. If someone has the idea to change the ERROR to a
WARNING, then the post-read cleanup doing all the discard would see a
half-baked entry, due to pgstat_drop_entry_internal() in
pgstat_reset_after_failure -> pgstat_drop_all_entries() if I follow
that right. That would be surprising, still that's not something
worth more than a HEAD-only change.
--
Michael