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-gmqd2b...@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/[email protected] https://postgr.es/m/[email protected] 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/[email protected] 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
0001-Clean-up-pgstats-hash-entry-after-restore-OOM.patch
Description: 0001-Clean-up-pgstats-hash-entry-after-restore-OOM.patch
