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 <[email protected]> Отправлено: 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
