Missing dshash cleanup in pgstat_read_statsfile() after OOM
Hackorum builds and tests every patch posted to the lists, not only commitfest submissions. This is Hackorum's own CI rather than the PostgreSQL project's, and it is still under testing - please report anything that looks wrong.
This thread has been committed, so CI has stopped here. Anything below is the last result it produced.
You can run a PostgreSQL built from this patch straight from Docker, with no checkout and no build:
docker run --rm -p 5432:5432 ghcr.io/hackorum-dev/postgres-patch:t253702psql -h localhost -U postgresBuilt from patchset v1 (message #1), September 07, 2026 at 03:04 PM.
Every patchset is also pushed to a branch of our PostgreSQL fork, so you can check out the same tree CI built. Without a PostgreSQL checkout:
git clone --branch t253702_1 https://github.com/hackorum-dev/postgres.gitIn a checkout you already have, add the fork once:
git remote add hackorum https://github.com/hackorum-dev/postgres.gitthen, for this patchset and every later one:
git fetch hackorum t253702_1 && git checkout t253702_1Patchset v1 (message #1) is on t253702_1
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:
/messages/by-id/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:
/messages/by-id/aLfpyYaQ0g8i4R_m@paquier.xyz
/messages/by-id/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:
/messages/by-id/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
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
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@paquier.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
On Tue, Sep 08, 2026 at 06:34:29AM +0000, Grigorev Jurij wrote:
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.
Yeah.. I'm.. cough.. Aware of that. The annoying thing is that
this touches a very low-level subsystem, but I have to admit that I
have not spent cycles thinking about the possible options here.
--
Michael