On Wed, Sep 03, 2025 at 10:39:04PM +0100, Mikhail Kot wrote: > The issue is not only in pgstat_init_entry(). Currently it errors on OOM but > this doesn't prevent us from calling pgstat_lock_entry() through > pgstat_get_entry_ref() which accesses a non-initialized lock.
Spent more time on that, finally. So your argument is that this leads
to an inconsistent state in the hash table because the dshash API is
designed to force a new entry creation if it cannot be found.
- shheader = pgstat_init_entry(kind, shhashent);
+ PG_TRY();
+ {
+ shheader = pgstat_init_entry(kind, shhashent);
+ }
+ PG_CATCH();
+ {
+ dshash_delete_entry(pgStatLocal.shared_hash, shhashent);
+ PG_RE_THROW();
+ }
+ PG_END_TRY();
Hmm. There are a couple of extra errors that can be reached, through
get_segment_by_index() or dsa_get_address() for example, but these
point to scenarios that should never happen or programming errors when
using DSAs.
> Here's the second version of the patch. Now we remove inserted hash entry
> on OOM which would prevent accessing the entry
There are only two callers of pgstat_init_entry(), so I am wondering
if a better long-term thing would be to track this behavior in
pgstat_init_entry(), and let the callers deal with the cleanup
consequences, rather than have the callers of pgstat_init_entry()
guess that they may need to do something with a TRY/CATCH block. I
doubt that the number of callers of pgstat_init_entry() will raise,
but people like doing fancy things.
In pgstat_read_statsfile(), an interesting side effect is the
possibility to issue a soft error. I have never seen anybody complain
about an OOM from the startup process when reading the stats file,
TBH, still prioritizing availability is an interesting take when
reading the stats file when facing a DSA allocation failure.
In order to reproduce one failure pattern, you can use the attached
0002, then use this sequence to emulate the OOM path and the dshash
table inconsistency (install src/test/modules/injection_points first):
create extension injection_points;
select injection_points_attach('pgstat-init-entry-oom', 'notice');
-- SQL query able to create fresh pgstats entry
-- ERROR with patch 0001, crash on HEAD.
Note that none of that seems worth a backpatch, we have an history of
treating unlikely-going-to-happen errors like OOMs as HEAD-only
improvements. This one is of the same class.
--
Michael
From 6fe33b6446f1c57ca7570f742f39f844e36a7c23 Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Thu, 4 Sep 2025 16:27:51 +0900
Subject: [PATCH v3 1/2] Fix OOM failure handling with pgstats entry creations
---
src/backend/utils/activity/pgstat.c | 7 ++++++
src/backend/utils/activity/pgstat_shmem.c | 27 ++++++++++++++++++++++-
2 files changed, 33 insertions(+), 1 deletion(-)
diff --git a/src/backend/utils/activity/pgstat.c b/src/backend/utils/activity/pgstat.c
index ffb5b8cce344..8cba6e9cdf8c 100644
--- a/src/backend/utils/activity/pgstat.c
+++ b/src/backend/utils/activity/pgstat.c
@@ -1975,6 +1975,13 @@ pgstat_read_statsfile(void)
header = pgstat_init_entry(key.kind, p);
dshash_release_lock(pgStatLocal.shared_hash, p);
+ if (header == NULL)
+ {
+ elog(WARNING, "could not allocate entry %u/%u/%" PRIu64 " of type %c",
+ key.kind, key.dboid,
+ key.objid, t);
+ goto error;
+ }
if (!read_chunk(fpin,
pgstat_get_entry_data(key.kind, header),
diff --git a/src/backend/utils/activity/pgstat_shmem.c b/src/backend/utils/activity/pgstat_shmem.c
index 62de34744536..8f8b57e8ee8a 100644
--- a/src/backend/utils/activity/pgstat_shmem.c
+++ b/src/backend/utils/activity/pgstat_shmem.c
@@ -289,6 +289,12 @@ pgstat_detach_shmem(void)
* ------------------------------------------------------------
*/
+/*
+ * Initialize entry newly-created.
+ *
+ * Returns NULL in the event of an allocation failure, giving the possibility
+ * for callers to deal with any cleanup actions.
+ */
PgStatShared_Common *
pgstat_init_entry(PgStat_Kind kind,
PgStatShared_HashEntry *shhashent)
@@ -311,7 +317,12 @@ pgstat_init_entry(PgStat_Kind kind,
pg_atomic_init_u32(&shhashent->generation, 0);
shhashent->dropped = false;
- chunk = dsa_allocate0(pgStatLocal.dsa, pgstat_get_kind_info(kind)->shared_size);
+ chunk = dsa_allocate_extended(pgStatLocal.dsa,
+ pgstat_get_kind_info(kind)->shared_size,
+ DSA_ALLOC_ZERO | DSA_ALLOC_NO_OOM);
+ if (chunk == InvalidDsaPointer)
+ return NULL;
+
shheader = dsa_get_address(pgStatLocal.dsa, chunk);
shheader->magic = 0xdeadbeef;
@@ -509,6 +520,20 @@ pgstat_get_entry_ref(PgStat_Kind kind, Oid dboid, uint64 objid, bool create,
if (!shfound)
{
shheader = pgstat_init_entry(kind, shhashent);
+ if (shheader == NULL)
+ {
+ /*
+ * Failed the allocation of a new entry, so clean up the shared
+ * hashtable before giving up.
+ */
+ dshash_delete_entry(pgStatLocal.shared_hash, shhashent);
+
+ ereport(ERROR,
+ (errcode(ERRCODE_OUT_OF_MEMORY),
+ errmsg("out of memory"),
+ errdetail("Failed while allocating entry %u/%u/%" PRIu64 " of kind %u.",
+ key.kind, key.dboid, key.objid, kind)));
+ }
pgstat_acquire_entry_ref(entry_ref, shhashent, shheader);
if (created_entry != NULL)
--
2.51.0
From 08178c5820364143bc039717a4ac98a27c0a4cdb Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Thu, 4 Sep 2025 16:19:03 +0900
Subject: [PATCH v3 2/2] Add injection point for OOM failure emulation
---
src/backend/utils/activity/pgstat_shmem.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/src/backend/utils/activity/pgstat_shmem.c b/src/backend/utils/activity/pgstat_shmem.c
index 8f8b57e8ee8a..c48a52db1e88 100644
--- a/src/backend/utils/activity/pgstat_shmem.c
+++ b/src/backend/utils/activity/pgstat_shmem.c
@@ -14,6 +14,7 @@
#include "pgstat.h"
#include "storage/shmem.h"
+#include "utils/injection_point.h"
#include "utils/memutils.h"
#include "utils/pgstat_internal.h"
@@ -317,6 +318,9 @@ pgstat_init_entry(PgStat_Kind kind,
pg_atomic_init_u32(&shhashent->generation, 0);
shhashent->dropped = false;
+ if (IS_INJECTION_POINT_ATTACHED("pgstat-init-entry-oom"))
+ return NULL;
+
chunk = dsa_allocate_extended(pgStatLocal.dsa,
pgstat_get_kind_info(kind)->shared_size,
DSA_ALLOC_ZERO | DSA_ALLOC_NO_OOM);
--
2.51.0
signature.asc
Description: PGP signature
