Re: Missing dshash cleanup in pgstat_read_statsfile() after OOM

Hi Michael,

Thanks, agreed. I missed the important consequence that ERROR in the
startup process is promoted to FATAL and that shared memory is then
reinitialized. Therefore, the half-initialized entry left by
pgstat_read_statsfile() is not observable by later backends with the
code as it stands on HEAD, and this path cannot explain the ASan crash.

I have since traced the ASan failure to a different path: an error raised
while creating a new DSM segment can escape
dsa_allocate_extended(..., DSA_ALLOC_NO_OOM) after pgstat_init_entry()
has marked the hash entry live but before its body is assigned. I will
report that separately with the logs and a patch.

The pgstat_read_statsfile() change remains a consistency and defensive
cleanup improvement, particularly if the restore ERROR is ever changed
to a WARNING.

I see that you have committed the patch. Thanks for taking care of it;
I'm glad the report was useful :)

Thanks,
Yuriy
________________________________________
От: Michael Paquier <michael(at)paquier(dot)xyz>
Отправлено: 8 сентября 2026 г. 11:35:35
Кому: Григорьев Юрий
Копия: PostgreSQL Hackers
Тема: 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