agora inbox for pgsql-hackers@postgresql.org
help / color / mirror / Atom feedDSA_ALLOC_NO_OOM vs dsm_create ERROR leaving a half-initialized pgstats hash entry
10+ messages / 2 participants
[nested] [flat]
* DSA_ALLOC_NO_OOM vs dsm_create ERROR leaving a half-initialized pgstats hash entry
@ 2026-09-08 09:06 Grigorev Jurij <ju.grigorev@ftdata.ru>
2026-09-08 23:52 ` Re: DSA_ALLOC_NO_OOM vs dsm_create ERROR leaving a half-initialized pgstats hash entry Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 10+ messages in thread
From: Grigorev Jurij @ 2026-09-08 09:06 UTC (permalink / raw)
To: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>; +Cc: michael@paquier.xyz <michael@paquier.xyz>
Hi,
I'm splitting this out of the pgstat_read_statsfile() cleanup thread [1],
so that discussion can stay about the restore path.
The case I reproduced is exactly that hole:
dsa_allocate_extended(..., DSA_ALLOC_NO_OOM) can still raise ERROR from
dsm_create() in make_new_segment(), after pgstat_init_entry() has marked
the hash entry live and before its body is assigned. The NULL cleanup
in pgstat_get_entry_ref() is then bypassed.
I reproduced this on a running TAP cluster under ASan with a constrained
/dev/shm (recovery/020_archive_status and 034_create_database). The
same postgresql.log shows, about a second apart:
FATAL: could not resize shared memory segment "/PostgreSQL.…"
to 1048576 bytes: No space left on device
… then a later backend …
AddressSanitizer: SEGV on unknown address 0x0
in pgstat_acquire_entry_ref()
from pgstat_get_entry_ref() existing-entry path
gdb on the crashing backend showed a live shared hash entry:
dropped = false, refcount = 1, generation = 0,
body = InvalidDsaPointer,
kind = relation, dboid = 0, objid = pg_authid or pg_database
There was no "Failed while allocating entry" / "could not allocate
entry" message, so the InvalidDsaPointer cleanup added by 8191e0c did
not run. On InitPostgres the ERROR is promoted to FATAL and the
connecting backend exits, but the postmaster does not reinitialize
shared memory, so the half-initialized entry remains visible.
I see two possible layers at which to address this.
1. pgstats only: allocate the DSA body before inserting the shared hash
entry, and initialize the entry only after a valid chunk has been
obtained. A dsm_create() failure would then not leave a live entry
whose body is InvalidDsaPointer.
This is the "flip the order" approach discussed in [2]. It would
require changing the two callers of pgstat_init_entry(), and dealing
with a concurrently inserted entry by freeing the preallocated,
unused chunk.
2. DSA/DSM: make DSA_ALLOC_NO_OOM cover failures to create or resize a
new DSM segment as well, so dsa_allocate_extended() consistently
returns InvalidDsaPointer for allocation failures instead of raising
ERROR.
This seems closer to the documented DSA_ALLOC_NO_OOM contract, but it
is the lower-level change Michael mentioned. It would need to distinguish
resource exhaustion, such as ENOSPC while resizing a POSIX shared
memory object, from DSM failures that should still be reported as
errors.
I would rather not go back to PG_TRY/PG_CATCH around
pgstat_init_entry(); that was considered in [2] and dropped in favour
of returning NULL.
Option (1) could close the pgstats corruption independently of the
lower-level question. Option (2) would make the NO_OOM behavior
consistent for other callers as well.
I can prepare the pgstats patch for (1), or investigate the DSA/DSM
approach first if you think that is the better layer. I can also add a
deterministic failure-injection test for the new-segment path.
Thanks,
Yuriy Grigoryev
[1] https://postgr.es/m/d55ecaf911844d53bd0a931751dce582@localhost.localdomain
[2] https://postgr.es/m/CAAi9E7jELo5_-sBENftnc2E8XhW2PKZJWfTC3i2y-GMQd2bcqQ@mail.gmail.com
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: DSA_ALLOC_NO_OOM vs dsm_create ERROR leaving a half-initialized pgstats hash entry
2026-09-08 09:06 DSA_ALLOC_NO_OOM vs dsm_create ERROR leaving a half-initialized pgstats hash entry Grigorev Jurij <ju.grigorev@ftdata.ru>
@ 2026-09-08 23:52 ` Michael Paquier <michael@paquier.xyz>
2026-09-09 10:56 ` Re: DSA_ALLOC_NO_OOM vs dsm_create ERROR leaving a half-initialized pgstats hash entry Grigorev Jurij <ju.grigorev@ftdata.ru>
0 siblings, 1 reply; 10+ messages in thread
From: Michael Paquier @ 2026-09-08 23:52 UTC (permalink / raw)
To: Grigorev Jurij <ju.grigorev@ftdata.ru>; +Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Tue, Sep 08, 2026 at 09:06:36AM +0000, Grigorev Jurij wrote:
> I would rather not go back to PG_TRY/PG_CATCH around
> pgstat_init_entry(); that was considered in [2] and dropped in favour
> of returning NULL.
I've never been much on favor of TRY/CATCH generally for any kind of
low-level subsystem because it makes the stack manipulations more
complicated, sometimes with static states that need to be kept at
backend level (think threading). Being able to keep track of the
state across
> Option (1) could close the pgstats corruption independently of the
> lower-level question. Option (2) would make the NO_OOM behavior
> consistent for other callers as well.
>
> I can prepare the pgstats patch for (1), or investigate the DSA/DSM
> approach first if you think that is the better layer. I can also add a
> deterministic failure-injection test for the new-segment path.
My question regarding (1) vs (2) would be: do we have other
sub-systems that display patterns similar to pgstats when it comes to
the DSA/DSM failing? If pgstats is the only one, (1) sounds like a
solution good enough for me. (2) would show more value if there is a
gazillion of call sites that would like to care about more error
states when doing a DSA/DSM allocation.
I'd still tend to prefer (1) compared to (2) on top of my mind, but
I'm proved wrong a lot, so..
--
Michael
Attachments:
[application/pgp-signature] signature.asc (832B, ../../aqCfwe5-EIiJXlIu@paquier.xyz/2-signature.asc)
download
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: DSA_ALLOC_NO_OOM vs dsm_create ERROR leaving a half-initialized pgstats hash entry
2026-09-08 09:06 DSA_ALLOC_NO_OOM vs dsm_create ERROR leaving a half-initialized pgstats hash entry Grigorev Jurij <ju.grigorev@ftdata.ru>
2026-09-08 23:52 ` Re: DSA_ALLOC_NO_OOM vs dsm_create ERROR leaving a half-initialized pgstats hash entry Michael Paquier <michael@paquier.xyz>
@ 2026-09-09 10:56 ` Grigorev Jurij <ju.grigorev@ftdata.ru>
2026-09-17 02:48 ` Re: DSA_ALLOC_NO_OOM vs dsm_create ERROR leaving a half-initialized pgstats hash entry Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 10+ messages in thread
From: Grigorev Jurij @ 2026-09-09 10:56 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Tue, Sep 08, 2026 at 11:52:33PM +0000, Michael Paquier wrote:
> My question regarding (1) vs (2) would be: do we have other
> sub-systems that display patterns similar to pgstats when it comes to
> the DSA/DSM failing? If pgstats is the only one, (1) sounds like a
> solution good enough for me.
I checked all in-tree DSA_ALLOC_NO_OOM and DSHASH_INSERT_NO_OOM call
sites, plus the comparable two-phase cases in typcache.c and async.c.
pgstats is the only one that both publishes its surrounding object
before allocating the DSA body and assumes the body is always valid.
With one exception, discussed below, they all allocate before
publishing. In dshash, insert_into_bucket() allocates the item before
linking it into a bucket, resize() allocates the new bucket array
before replacing the old one, and dshash_create() does not publish the
table until its buckets exist. pgsa_set_advice_string() in
contrib/pg_stash_advice allocates the advice string, then inserts with
DSHASH_INSERT_NO_OOM and frees the string if that returns NULL, which
is exactly the ordering the patch gives pgstats.
find_or_make_matching_shared_tupledesc() in typcache.c copies the
TupleDesc into DSA first, with a PG_TRY() around the insertion to free
it on error.
The exception, and the closest case to pgstats, is async.c:
PrepareTableEntriesForListen() inserts a channel entry with
listenersArray == InvalidDsaPointer and allocates afterwards. But that
incomplete state is supported by design -- numListeners stays zero, so
consumers do not access any array elements, and a later call retries
the allocation. pgstats has no such tolerance: the existing-entry path
dereferences body unconditionally, which is why the escaped error
becomes a SEGV instead of a retry.
So option (1) looks sufficient for this crash. Patch attached.
It allocates the DSA body before inserting the shared hash entry;
pgstat_init_entry() now receives a valid chunk and no longer allocates.
Both callers use dshash_find_or_insert_extended() with
DSHASH_INSERT_NO_OOM and free the preallocated body if insertion
returns NULL or finds an existing entry.
One residual case remains: dshash insertion can itself raise ERROR from
dsm_create() despite DSHASH_INSERT_NO_OOM, in which case the
preallocated chunk is not reclaimed. pg_stash_advice and
dshash_create() have analogous unreachable-allocation cases. No
inconsistent pgstats entry is published, but closing the leak would
require either exception cleanup or the lower-level NO_OOM change. I
left PG_TRY/PG_CATCH out based on your comments; typcache shows how it
could be used if such cleanup is preferred.
8191e0c was backpatched through 15 for the same class of corruption, so
this path may deserve the same treatment. If the approach looks right,
I can prepare back-branch versions and add a deterministic test using
an injection point in make_new_segment() before dsm_create().
Thanks,
Yuriy Grigoryev=
Attachments:
[application/octet-stream] 0001-Allocate-pgstats-entry-body-before-shared-hash-insert.patch (10.1K, ../../a72c99d5b1e448b191a89e30ecaf3e8a@localhost.localdomain/2-0001-Allocate-pgstats-entry-body-before-shared-hash-insert.patch)
download | inline diff:
From d06792297a82fb0a0a1d8dbb1f4064c324cb485a Mon Sep 17 00:00:00 2001
From: Yuriy Grigoryev <ju.grigorev@ftdata.ru>
Date: Tue, 8 Sep 2026 13:30:18 +0700
Subject: [PATCH] Allocate pgstats entry body before shared hash insert
8191e0c made pgstat_init_entry() return NULL on DSA allocation failure
so callers could delete a just-inserted hash entry. That works when
dsa_allocate_extended(..., DSA_ALLOC_NO_OOM) returns InvalidDsaPointer,
but not when creating a new DSM segment raises ERROR (for example ENOSPC
on posix shared memory).
pgstat_init_entry() marked the hash entry live before allocating the
body. If dsm_create() failed after the insert, error unwind released
the dshash lock but left a visible entry with body == InvalidDsaPointer.
The next backend crashed in pgstat_acquire_entry_ref().
Split allocation from hash initialization: allocate the DSA chunk first,
insert the hash entry only with a valid body, and free the chunk if a
concurrent insert wins or the insert returns NULL. Both callers use
dshash_find_or_insert_extended() with DSHASH_INSERT_NO_OOM so that path
can free the preallocated body.
Discussion: https://postgr.es/m/ddc3ecfb01ce4e9698b23cc59767f016@localhost.localdomain
---
src/backend/utils/activity/pgstat.c | 45 ++++++----
src/backend/utils/activity/pgstat_shmem.c | 105 ++++++++++++----------
src/include/utils/pgstat_internal.h | 4 +-
3 files changed, 91 insertions(+), 63 deletions(-)
diff --git a/src/backend/utils/activity/pgstat.c b/src/backend/utils/activity/pgstat.c
index 5177f88..3f4fe60 100644
--- a/src/backend/utils/activity/pgstat.c
+++ b/src/backend/utils/activity/pgstat.c
@@ -2007,6 +2007,7 @@ pgstat_read_statsfile(void)
PgStatShared_HashEntry *p;
PgStatShared_Common *header;
const PgStat_KindInfo *kind_info = NULL;
+ dsa_pointer chunk;
CHECK_FOR_INTERRUPTS();
@@ -2095,12 +2096,39 @@ pgstat_read_statsfile(void)
* This intentionally doesn't use pgstat_get_entry_ref() -
* putting all stats into checkpointer's
* pgStatEntryRefHash would be wasted effort and memory.
+ *
+ * Allocate the DSA body before inserting the hash entry,
+ * so a dsm_create failure cannot leave a half-initialized
+ * shared entry behind.
*/
- p = dshash_find_or_insert(pgStatLocal.shared_hash, &key, &found);
+ chunk = pgstat_alloc_entry_body(key.kind);
+ if (chunk == InvalidDsaPointer)
+ {
+ /*
+ * It would be tempting to switch this ERROR to a
+ * WARNING, but it would mean that all the statistics
+ * are discarded when the environment fails on OOM.
+ */
+ elog(ERROR, "could not allocate entry %u/%u/%" PRIu64 " of type %c",
+ key.kind, key.dboid,
+ key.objid, t);
+ }
+
+ p = dshash_find_or_insert_extended(pgStatLocal.shared_hash,
+ &key, &found,
+ DSHASH_INSERT_NO_OOM);
+ if (!p)
+ {
+ dsa_free(pgStatLocal.dsa, chunk);
+ elog(ERROR, "could not insert entry %u/%u/%" PRIu64 " of type %c",
+ key.kind, key.dboid,
+ key.objid, t);
+ }
/* don't allow duplicate entries */
if (found)
{
+ dsa_free(pgStatLocal.dsa, chunk);
dshash_release_lock(pgStatLocal.shared_hash, p);
elog(WARNING, "found duplicate stats entry %u/%u/%" PRIu64 " of type %c",
key.kind, key.dboid,
@@ -2108,20 +2136,7 @@ pgstat_read_statsfile(void)
goto error;
}
- header = pgstat_init_entry(key.kind, p);
- if (header == NULL)
- {
- dshash_delete_entry(pgStatLocal.shared_hash, p);
-
- /*
- * It would be tempting to switch this ERROR to a
- * WARNING, but it would mean that all the statistics
- * are discarded when the environment fails on OOM.
- */
- elog(ERROR, "could not allocate entry %u/%u/%" PRIu64 " of type %c",
- key.kind, key.dboid,
- key.objid, t);
- }
+ header = pgstat_init_entry(key.kind, p, chunk);
dshash_release_lock(pgStatLocal.shared_hash, p);
if (!read_chunk(fpin,
diff --git a/src/backend/utils/activity/pgstat_shmem.c b/src/backend/utils/activity/pgstat_shmem.c
index d8ac9d6..bf886b4 100644
--- a/src/backend/utils/activity/pgstat_shmem.c
+++ b/src/backend/utils/activity/pgstat_shmem.c
@@ -303,53 +303,58 @@ pgstat_detach_shmem(void)
*/
/*
- * Initialize entry newly-created.
+ * Allocate the DSA body for a new variable-numbered pgstats entry.
*
- * Returns NULL in the event of an allocation failure, so as callers can
- * take cleanup actions as the entry initialized is already inserted in the
- * shared hashtable.
+ * Returns InvalidDsaPointer if the allocation fails without throwing. Call
+ * this before inserting a hash entry: dsa_allocate_extended() can still raise
+ * ERROR when creating a new DSM segment (for example ENOSPC), and doing that
+ * after the insert would leave a live hash entry with body ==
+ * InvalidDsaPointer.
+ */
+dsa_pointer
+pgstat_alloc_entry_body(PgStat_Kind kind)
+{
+ const PgStat_KindInfo *kind_info = pgstat_get_kind_info(kind);
+
+ return dsa_allocate_extended(pgStatLocal.dsa,
+ kind_info->shared_size,
+ DSA_ALLOC_ZERO | DSA_ALLOC_NO_OOM);
+}
+
+/*
+ * Initialize a newly-inserted hash entry around an already-allocated DSA
+ * body.
+ *
+ * The caller must hold the dshash partition lock. The entry cannot be found
+ * by other backends until that lock is released, so it is safe to publish
+ * refcount/dropped/body here. Caller needs to increment the refcount further
+ * if a longer-lived reference is needed.
+ *
+ * chunk must be a valid pointer from pgstat_alloc_entry_body().
*/
PgStatShared_Common *
pgstat_init_entry(PgStat_Kind kind,
- PgStatShared_HashEntry *shhashent)
+ PgStatShared_HashEntry *shhashent,
+ dsa_pointer chunk)
{
- /* Create new stats entry. */
- dsa_pointer chunk;
PgStatShared_Common *shheader;
const PgStat_KindInfo *kind_info = pgstat_get_kind_info(kind);
- /*
- * Initialize refcount to 1, marking it as valid / not dropped. The entry
- * can't be freed before the initialization because it can't be found as
- * long as we hold the dshash partition lock. Caller needs to increase
- * further if a longer lived reference is needed.
- */
- pg_atomic_init_u32(&shhashent->refcount, 1);
-
- /*
- * Initialize "generation" to 0, as freshly created.
- */
- pg_atomic_init_u32(&shhashent->generation, 0);
- shhashent->dropped = false;
-
- chunk = dsa_allocate_extended(pgStatLocal.dsa,
- kind_info->shared_size,
- DSA_ALLOC_ZERO | DSA_ALLOC_NO_OOM);
- if (chunk == InvalidDsaPointer)
- return NULL;
+ Assert(DsaPointerIsValid(chunk));
shheader = dsa_get_address(pgStatLocal.dsa, chunk);
shheader->magic = 0xdeadbeef;
+ LWLockInitialize(&shheader->lock, LWTRANCHE_PGSTATS_DATA);
- /* Link the new entry from the hash entry. */
+ pg_atomic_init_u32(&shhashent->refcount, 1);
+ pg_atomic_init_u32(&shhashent->generation, 0);
+ shhashent->dropped = false;
shhashent->body = chunk;
/* Increment entry count, if required. */
if (kind_info->track_entry_count)
pg_atomic_fetch_add_u64(&pgStatLocal.shmem->entry_counts[kind - 1], 1);
- LWLockInitialize(&shheader->lock, LWTRANCHE_PGSTATS_DATA);
-
return shheader;
}
@@ -545,6 +550,24 @@ pgstat_get_entry_ref(PgStat_Kind kind, Oid dboid, uint64 objid, bool create,
if (create && !shhashent)
{
bool shfound;
+ dsa_pointer chunk;
+
+ /*
+ * Allocate the stats body before inserting a hash entry. Creating a
+ * new DSA segment can raise ERROR (e.g. ENOSPC on posix shm); doing
+ * that after the insert would leave a live hash entry with an
+ * invalid body.
+ */
+ chunk = pgstat_alloc_entry_body(kind);
+ if (chunk == InvalidDsaPointer)
+ {
+ pgstat_release_entry_ref(key, entry_ref, false);
+ ereport(ERROR,
+ (errcode(ERRCODE_OUT_OF_MEMORY),
+ errmsg("out of memory"),
+ errdetail("Failed while allocating entry %u/%u/%" PRIu64 ".",
+ key.kind, key.dboid, key.objid)));
+ }
/*
* It's possible that somebody created the entry since the above
@@ -556,6 +579,8 @@ pgstat_get_entry_ref(PgStat_Kind kind, Oid dboid, uint64 objid, bool create,
DSHASH_INSERT_NO_OOM);
if (!shhashent)
{
+ dsa_free(pgStatLocal.dsa, chunk);
+
/*
* Clean up the local reference when failing insert into the
* shared hashtable.
@@ -570,24 +595,7 @@ 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 both the
- * local reference and the shared hashtable before giving up.
- * Clean the local state first, since releasing the dshash
- * lock can process a pending interrupt.
- */
- pgstat_release_entry_ref(key, entry_ref, false);
- 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 ".",
- key.kind, key.dboid, key.objid)));
- }
+ shheader = pgstat_init_entry(kind, shhashent, chunk);
pgstat_acquire_entry_ref(entry_ref, shhashent, shheader);
if (created_entry != NULL)
@@ -595,6 +603,9 @@ pgstat_get_entry_ref(PgStat_Kind kind, Oid dboid, uint64 objid, bool create,
return entry_ref;
}
+
+ /* Concurrent insert won; drop the unused body. */
+ dsa_free(pgStatLocal.dsa, chunk);
}
if (!shhashent)
diff --git a/src/include/utils/pgstat_internal.h b/src/include/utils/pgstat_internal.h
index 14369e5..201e572 100644
--- a/src/include/utils/pgstat_internal.h
+++ b/src/include/utils/pgstat_internal.h
@@ -839,8 +839,10 @@ extern void pgstat_reset_matching_entries(bool (*do_reset) (PgStatShared_HashEnt
TimestampTz ts);
extern void pgstat_request_entry_refs_gc(void);
+extern dsa_pointer pgstat_alloc_entry_body(PgStat_Kind kind);
extern PgStatShared_Common *pgstat_init_entry(PgStat_Kind kind,
- PgStatShared_HashEntry *shhashent);
+ PgStatShared_HashEntry *shhashent,
+ dsa_pointer chunk);
/*
--
2.50.1 (Apple Git-155)
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: DSA_ALLOC_NO_OOM vs dsm_create ERROR leaving a half-initialized pgstats hash entry
2026-09-08 09:06 DSA_ALLOC_NO_OOM vs dsm_create ERROR leaving a half-initialized pgstats hash entry Grigorev Jurij <ju.grigorev@ftdata.ru>
2026-09-08 23:52 ` Re: DSA_ALLOC_NO_OOM vs dsm_create ERROR leaving a half-initialized pgstats hash entry Michael Paquier <michael@paquier.xyz>
2026-09-09 10:56 ` Re: DSA_ALLOC_NO_OOM vs dsm_create ERROR leaving a half-initialized pgstats hash entry Grigorev Jurij <ju.grigorev@ftdata.ru>
@ 2026-09-17 02:48 ` Michael Paquier <michael@paquier.xyz>
2026-09-17 03:44 ` Re: DSA_ALLOC_NO_OOM vs dsm_create ERROR leaving a half-initialized pgstats hash entry Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 10+ messages in thread
From: Michael Paquier @ 2026-09-17 02:48 UTC (permalink / raw)
To: Grigorev Jurij <ju.grigorev@ftdata.ru>; +Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Wed, Sep 09, 2026 at 10:56:34AM +0000, Grigorev Jurij wrote:
> With one exception, discussed below, they all allocate before
> publishing. In dshash, insert_into_bucket() allocates the item before
> linking it into a bucket, resize() allocates the new bucket array
> before replacing the old one, and dshash_create() does not publish the
> table until its buckets exist. pgsa_set_advice_string() in
> contrib/pg_stash_advice allocates the advice string, then inserts with
> DSHASH_INSERT_NO_OOM and frees the string if that returns NULL, which
> is exactly the ordering the patch gives pgstats.
> find_or_make_matching_shared_tupledesc() in typcache.c copies the
> TupleDesc into DSA first, with a PG_TRY() around the insertion to free
> it on error.
Ah. I've missed the typcache.c thing previously. So this TRY/CATCH
pattern where we care about other error types than OOMs exist. Thanks
for pointing it out.
> It allocates the DSA body before inserting the shared hash entry;
> pgstat_init_entry() now receives a valid chunk and no longer allocates.
> Both callers use dshash_find_or_insert_extended() with
> DSHASH_INSERT_NO_OOM and free the preallocated body if insertion
> returns NULL or finds an existing entry.
Reading more through the patch.. I'm OK with the extra promise that
it brings: allocate first the chunk, then attempt an insert into the
shared hash table to not polute once we hold a chunk.
> One residual case remains: dshash insertion can itself raise ERROR from
> dsm_create() despite DSHASH_INSERT_NO_OOM, in which case the
> preallocated chunk is not reclaimed. pg_stash_advice and
> dshash_create() have analogous unreachable-allocation cases. No
> inconsistent pgstats entry is published, but closing the leak would
> require either exception cleanup or the lower-level NO_OOM change. I
> left PG_TRY/PG_CATCH out based on your comments; typcache shows how it
> could be used if such cleanup is preferred.
Ahh.. You mean that inside the dshash_find_or_insert_extended(), if
dsm_create() itself fails, then we leak a DSA chunk previously
allocated. Yes, that's not a new thing. We could try to plumber
something inside dsm_create() but I take it as a cost/balance issue
because a a TRY/CATCH block is not completely free. On a very
unlikely failure, if I get you right, it means that we just leak some
memory. I'd take that leak over a shared memory state corruption all
the time taking down the cluster. Your patch is still an improvement:
we don't globally maintain an inconsistent shared memory state
anymore.
> 8191e0c was backpatched through 15 for the same class of corruption, so
> this path may deserve the same treatment. If the approach looks right,
> I can prepare back-branch versions and add a deterministic test using
> an injection point in make_new_segment() before dsm_create().
I've pondered about that. A DSA allocation error while having
inconsistent data in the shared hash table means pollution across the
whole cluster. I think that this warrants a backpatch for the same
reason as 8191e0c16a03: it is not limited to a backend-level static
state. One allocation error can bring the whole cluster down. That's
not cool. If you can produce some patches down to v15, that would
speed up my work looking at all these branches, for sure.
In the stats read path, dshash_find_or_insert_extended() combined with
pgstat_alloc_entry_body() feels kind of nice. On OOM, we get nicer
reports. Under other failures, ERRORs are upgraded to FATAL. Not
perfect as it would lack context, still OK. By the way, we don't
really need to care about this code path if we get a failure due to
the previous argument, as a ERROR->FATAL just brings the server down
when the stats are read, taking down shared memory while on it.
Accomodating the stats read path with the redesign of
pgstat_init_entry() makes sense to me anyway: we want callers to give
a pre-allocated DSA chunk, let the caller deal with any cleanup errors
during the DSA allocation.
+ /*
+ * Allocate the stats body before inserting a hash entry. Creating a
+ * new DSA segment can raise ERROR (e.g. ENOSPC on posix shm); doing
+ * that after the insert would leave a live hash entry with an
+ * invalid body.
+ */
+ chunk = pgstat_alloc_entry_body(kind);
Hmm. This still leaves a local entry_ref if pgstat_alloc_entry_body()
itself fails. Compared to the case of a corrupted shmem area. I think
that I can live with that. And if I'm reading that right, the backend
reference that may still be around self-heals on re-entry if a backend
tries to insert again the same entry?
+ LWLockInitialize(&shheader->lock, LWTRANCHE_PGSTATS_DATA);
- /* Link the new entry from the hash entry. */
+ pg_atomic_init_u32(&shhashent->refcount, 1);
[...]
- LWLockInitialize(&shheader->lock, LWTRANCHE_PGSTATS_DATA);
Why is this LWLockInitialize() moved around?
+ dsa_free(pgStatLocal.dsa, chunk);
+ dshash_release_lock(pgStatLocal.shared_hash, p);
In the "don't allow duplicate entries" case of
pgstat_read_statsfile(), doing a dsa_free() while holding the dshash
lock is just wasteful. There should be no concurrent activity in this
code path, which is OK in practice; that's just wasteful.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (832B, ../../aqtU-qs5H3LbUum1@paquier.xyz/2-signature.asc)
download
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: DSA_ALLOC_NO_OOM vs dsm_create ERROR leaving a half-initialized pgstats hash entry
2026-09-08 09:06 DSA_ALLOC_NO_OOM vs dsm_create ERROR leaving a half-initialized pgstats hash entry Grigorev Jurij <ju.grigorev@ftdata.ru>
2026-09-08 23:52 ` Re: DSA_ALLOC_NO_OOM vs dsm_create ERROR leaving a half-initialized pgstats hash entry Michael Paquier <michael@paquier.xyz>
2026-09-09 10:56 ` Re: DSA_ALLOC_NO_OOM vs dsm_create ERROR leaving a half-initialized pgstats hash entry Grigorev Jurij <ju.grigorev@ftdata.ru>
2026-09-17 02:48 ` Re: DSA_ALLOC_NO_OOM vs dsm_create ERROR leaving a half-initialized pgstats hash entry Michael Paquier <michael@paquier.xyz>
@ 2026-09-17 03:44 ` Michael Paquier <michael@paquier.xyz>
2026-09-17 06:19 ` Re: DSA_ALLOC_NO_OOM vs dsm_create ERROR leaving a half-initialized pgstats hash entry Grigorev Jurij <ju.grigorev@ftdata.ru>
0 siblings, 1 reply; 10+ messages in thread
From: Michael Paquier @ 2026-09-17 03:44 UTC (permalink / raw)
To: Grigorev Jurij <ju.grigorev@ftdata.ru>; +Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Thu, Sep 17, 2026 at 11:48:26AM +0900, Michael Paquier wrote:
> + /*
> + * Allocate the stats body before inserting a hash entry. Creating a
> + * new DSA segment can raise ERROR (e.g. ENOSPC on posix shm); doing
> + * that after the insert would leave a live hash entry with an
> + * invalid body.
> + */
> + chunk = pgstat_alloc_entry_body(kind);
>
> Hmm. This still leaves a local entry_ref if pgstat_alloc_entry_body()
> itself fails. Compared to the case of a corrupted shmem area. I think
> that I can live with that. And if I'm reading that right, the backend
> reference that may still be around self-heals on re-entry if a backend
> tries to insert again the same entry?
Ah. 4069df21beb8 points exactly at that case. Perhaps we should
extend pgstat_gc_entry_refs() so as it is able to handle gracefully a
partial reference then? I would imagine something like that, that
forces a release of the local entry if we don't have a shared_entry,
as of:
--- a/src/backend/utils/activity/pgstat_shmem.c
+++ b/src/backend/utils/activity/pgstat_shmem.c
@@ -839,6 +839,15 @@ pgstat_gc_entry_refs(void)
Assert(!entry_ref->shared_stats ||
entry_ref->shared_stats->magic == 0xdeadbeef);
+ /* NULL shared_entry marks a partial reference */
+ if (entry_ref->shared_entry == NULL)
+ {
+ Assert(entry_ref->shared_stats == NULL);
+ Assert(entry_ref->pending == NULL);
+ pgstat_release_entry_ref(ent->key, entry_ref, false);
+ continue;
+ }
What do you think about the attached? That would be an independent
safety measure.
--
Michael
From b71b8a5adad24d44905576749fbda337ae1650bd Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@paquier.xyz>
Date: Thu, 17 Sep 2026 12:41:24 +0900
Subject: [PATCH] pgstat: Add safety measure for partially initialized local
reference
---
src/backend/utils/activity/pgstat_shmem.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/src/backend/utils/activity/pgstat_shmem.c b/src/backend/utils/activity/pgstat_shmem.c
index bf886b4dad86..86d7d4a7c6a9 100644
--- a/src/backend/utils/activity/pgstat_shmem.c
+++ b/src/backend/utils/activity/pgstat_shmem.c
@@ -839,6 +839,15 @@ pgstat_gc_entry_refs(void)
Assert(!entry_ref->shared_stats ||
entry_ref->shared_stats->magic == 0xdeadbeef);
+ /* NULL shared_entry marks a partial reference */
+ if (entry_ref->shared_entry == NULL)
+ {
+ Assert(entry_ref->shared_stats == NULL);
+ Assert(entry_ref->pending == NULL);
+ pgstat_release_entry_ref(ent->key, entry_ref, false);
+ continue;
+ }
+
/*
* "generation" checks for the case of entries being reinitialized,
* and "dropped" for the case where these are.. dropped.
--
2.55.0
Attachments:
[text/plain] 0001-pgstat-Add-safety-measure-for-partially-initialized-.patch (1.1K, ../../aqtiKTvl519bu8-V@paquier.xyz/2-0001-pgstat-Add-safety-measure-for-partially-initialized-.patch)
download | inline diff:
From b71b8a5adad24d44905576749fbda337ae1650bd Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@paquier.xyz>
Date: Thu, 17 Sep 2026 12:41:24 +0900
Subject: [PATCH] pgstat: Add safety measure for partially initialized local
reference
---
src/backend/utils/activity/pgstat_shmem.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/src/backend/utils/activity/pgstat_shmem.c b/src/backend/utils/activity/pgstat_shmem.c
index bf886b4dad86..86d7d4a7c6a9 100644
--- a/src/backend/utils/activity/pgstat_shmem.c
+++ b/src/backend/utils/activity/pgstat_shmem.c
@@ -839,6 +839,15 @@ pgstat_gc_entry_refs(void)
Assert(!entry_ref->shared_stats ||
entry_ref->shared_stats->magic == 0xdeadbeef);
+ /* NULL shared_entry marks a partial reference */
+ if (entry_ref->shared_entry == NULL)
+ {
+ Assert(entry_ref->shared_stats == NULL);
+ Assert(entry_ref->pending == NULL);
+ pgstat_release_entry_ref(ent->key, entry_ref, false);
+ continue;
+ }
+
/*
* "generation" checks for the case of entries being reinitialized,
* and "dropped" for the case where these are.. dropped.
--
2.55.0
[application/pgp-signature] signature.asc (832B, ../../aqtiKTvl519bu8-V@paquier.xyz/3-signature.asc)
download
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: DSA_ALLOC_NO_OOM vs dsm_create ERROR leaving a half-initialized pgstats hash entry
2026-09-08 09:06 DSA_ALLOC_NO_OOM vs dsm_create ERROR leaving a half-initialized pgstats hash entry Grigorev Jurij <ju.grigorev@ftdata.ru>
2026-09-08 23:52 ` Re: DSA_ALLOC_NO_OOM vs dsm_create ERROR leaving a half-initialized pgstats hash entry Michael Paquier <michael@paquier.xyz>
2026-09-09 10:56 ` Re: DSA_ALLOC_NO_OOM vs dsm_create ERROR leaving a half-initialized pgstats hash entry Grigorev Jurij <ju.grigorev@ftdata.ru>
2026-09-17 02:48 ` Re: DSA_ALLOC_NO_OOM vs dsm_create ERROR leaving a half-initialized pgstats hash entry Michael Paquier <michael@paquier.xyz>
2026-09-17 03:44 ` Re: DSA_ALLOC_NO_OOM vs dsm_create ERROR leaving a half-initialized pgstats hash entry Michael Paquier <michael@paquier.xyz>
@ 2026-09-17 06:19 ` Grigorev Jurij <ju.grigorev@ftdata.ru>
2026-09-17 07:10 ` Re: DSA_ALLOC_NO_OOM vs dsm_create ERROR leaving a half-initialized pgstats hash entry Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 10+ messages in thread
From: Grigorev Jurij @ 2026-09-17 06:19 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Hi Michael,
Thanks for the review!
> Hmm. This still leaves a local entry_ref if
> pgstat_alloc_entry_body() itself fails. Compared to the case of a
> corrupted shmem area. I think that I can live with that. And if I'm
> reading that right, the backend reference that may still be around
> self-heals on re-entry if a backend tries to insert again the same
> entry?
There are two failure cases here. If pgstat_alloc_entry_body() returns
InvalidDsaPointer, the attached patch releases the local entry_ref before
raising ERROR. If dsm_create() raises ERROR from inside the allocation,
that cleanup is bypassed and the partial local reference remains.
Yes, a later lookup of the same key can reuse it:
pgstat_get_entry_ref_cached() treats shared_stats == NULL as a cache miss
and retries initialization. However, pgstat_gc_entry_refs() can run
before such a retry and currently dereferences shared_entry
unconditionally.
The check you proposed in the follow-up [1] looks right to me. A partial
reference at this point cannot have shared_stats or pending data, and
pgstat_release_entry_ref() can remove it without touching shared state.
It handles the backend-local partial reference, while this patch prevents
the same failure from leaving an inconsistent shared hash entry. I agree
that the two changes should remain independent, as you suggested.
> Why is this LWLockInitialize() moved around?
There was no intended semantic change there; it was just unnecessary
movement while splitting allocation from initialization. v2 restores
LWLockInitialize() to its original position.
And for the duplicate stats-file entry path:
> doing a dsa_free() while holding the dshash lock is just wasteful.
Agreed. v2 releases the dshash lock before freeing the unused chunk in
that path.
The attached v2 is rebased, and contains those review changes.
It passes a full assertions-enabled build and all 239 core regression tests.
I have prepared and build-tested versions for the supported branches
down to REL_15_STABLE, but I am not attaching them yet so that the HEAD
change can be reviewed first. I can send them once the shape of this
patch is agreed.
Do you think this warrants a deterministic test?
A low-level injection point in make_new_segment() would reproduce the
actual escaped ERROR, but the test would also need to force this
particular DSA area to allocate a new segment. A pgstats-specific
injection point immediately before the body allocation would be much
more deterministic, but it would model the ERROR rather than exercise
dsm_create() itself.
I left test-only instrumentation out of v2 for now. I can add one of
these approaches if you think the extra coverage is worth it.
Regards,
Yuriy
[1] https://postgr.es/m/aqtiKTvl519bu8-V%40paquier.xyz=
Attachments:
[application/octet-stream] v2-0001-Allocate-pgstats-entry-body-before-shared-hash-insert.patch (9.8K, ../../d6ed296c81c14d03b1b5e283e913c40b@localhost.localdomain/2-v2-0001-Allocate-pgstats-entry-body-before-shared-hash-insert.patch)
download | inline diff:
From c58e2be5b1877ead05c62a449c66a5fe13cc58ee Mon Sep 17 00:00:00 2001
From: Yuriy Grigoryev <ju.grigorev@ftdata.ru>
Date: Thu, 17 Sep 2026 12:35:54 +0700
Subject: [PATCH v2] Allocate pgstats entry body before shared hash insert
8191e0c made pgstat_init_entry() return NULL on DSA allocation failure
so callers could delete a just-inserted hash entry. That works when
dsa_allocate_extended(..., DSA_ALLOC_NO_OOM) returns InvalidDsaPointer,
but not when creating a new DSM segment raises ERROR (for example ENOSPC
on POSIX shared memory).
pgstat_init_entry() marked the hash entry live before allocating the
body. If dsm_create() failed after the insert, error unwind released
the dshash lock but left a visible entry with body == InvalidDsaPointer.
The next backend crashed in pgstat_acquire_entry_ref().
Split allocation from hash initialization: allocate the DSA chunk first,
insert the hash entry only with a valid body, and free the chunk if a
concurrent insert wins or the insert returns NULL. Both callers use
dshash_find_or_insert_extended() with DSHASH_INSERT_NO_OOM so that path
can free the preallocated body.
Discussion: https://postgr.es/m/ddc3ecfb01ce4e9698b23cc59767f016@localhost.localdomain
---
src/backend/utils/activity/pgstat.c | 45 ++++++----
src/backend/utils/activity/pgstat_shmem.c | 102 ++++++++++++----------
src/include/utils/pgstat_internal.h | 4 +-
3 files changed, 90 insertions(+), 61 deletions(-)
diff --git a/src/backend/utils/activity/pgstat.c b/src/backend/utils/activity/pgstat.c
index 5177f880f70..5d81e9932d3 100644
--- a/src/backend/utils/activity/pgstat.c
+++ b/src/backend/utils/activity/pgstat.c
@@ -2007,6 +2007,7 @@ pgstat_read_statsfile(void)
PgStatShared_HashEntry *p;
PgStatShared_Common *header;
const PgStat_KindInfo *kind_info = NULL;
+ dsa_pointer chunk;
CHECK_FOR_INTERRUPTS();
@@ -2095,33 +2096,47 @@ pgstat_read_statsfile(void)
* This intentionally doesn't use pgstat_get_entry_ref() -
* putting all stats into checkpointer's
* pgStatEntryRefHash would be wasted effort and memory.
+ *
+ * Allocate the DSA body before inserting the hash entry,
+ * so a dsm_create failure cannot leave a half-initialized
+ * shared entry behind.
*/
- p = dshash_find_or_insert(pgStatLocal.shared_hash, &key, &found);
+ chunk = pgstat_alloc_entry_body(key.kind);
+ if (chunk == InvalidDsaPointer)
+ {
+ /*
+ * It would be tempting to switch this ERROR to a
+ * WARNING, but it would mean that all the statistics
+ * are discarded when the environment fails on OOM.
+ */
+ elog(ERROR, "could not allocate entry %u/%u/%" PRIu64 " of type %c",
+ key.kind, key.dboid,
+ key.objid, t);
+ }
+
+ p = dshash_find_or_insert_extended(pgStatLocal.shared_hash,
+ &key, &found,
+ DSHASH_INSERT_NO_OOM);
+ if (!p)
+ {
+ dsa_free(pgStatLocal.dsa, chunk);
+ elog(ERROR, "could not insert entry %u/%u/%" PRIu64 " of type %c",
+ key.kind, key.dboid,
+ key.objid, t);
+ }
/* don't allow duplicate entries */
if (found)
{
dshash_release_lock(pgStatLocal.shared_hash, p);
+ dsa_free(pgStatLocal.dsa, chunk);
elog(WARNING, "found duplicate stats entry %u/%u/%" PRIu64 " of type %c",
key.kind, key.dboid,
key.objid, t);
goto error;
}
- header = pgstat_init_entry(key.kind, p);
- if (header == NULL)
- {
- dshash_delete_entry(pgStatLocal.shared_hash, p);
-
- /*
- * It would be tempting to switch this ERROR to a
- * WARNING, but it would mean that all the statistics
- * are discarded when the environment fails on OOM.
- */
- elog(ERROR, "could not allocate entry %u/%u/%" PRIu64 " of type %c",
- key.kind, key.dboid,
- key.objid, t);
- }
+ header = pgstat_init_entry(key.kind, p, chunk);
dshash_release_lock(pgStatLocal.shared_hash, p);
if (!read_chunk(fpin,
diff --git a/src/backend/utils/activity/pgstat_shmem.c b/src/backend/utils/activity/pgstat_shmem.c
index d8ac9d6f5e8..8752019671e 100644
--- a/src/backend/utils/activity/pgstat_shmem.c
+++ b/src/backend/utils/activity/pgstat_shmem.c
@@ -303,45 +303,51 @@ pgstat_detach_shmem(void)
*/
/*
- * Initialize entry newly-created.
+ * Allocate the DSA body for a new variable-numbered pgstats entry.
*
- * Returns NULL in the event of an allocation failure, so as callers can
- * take cleanup actions as the entry initialized is already inserted in the
- * shared hashtable.
+ * Returns InvalidDsaPointer if the allocation fails without throwing. Call
+ * this before inserting a hash entry: dsa_allocate_extended() can still raise
+ * ERROR when creating a new DSM segment (for example ENOSPC), and doing that
+ * after the insert would leave a live hash entry with body ==
+ * InvalidDsaPointer.
+ */
+dsa_pointer
+pgstat_alloc_entry_body(PgStat_Kind kind)
+{
+ const PgStat_KindInfo *kind_info = pgstat_get_kind_info(kind);
+
+ return dsa_allocate_extended(pgStatLocal.dsa,
+ kind_info->shared_size,
+ DSA_ALLOC_ZERO | DSA_ALLOC_NO_OOM);
+}
+
+/*
+ * Initialize a newly-inserted hash entry around an already-allocated DSA
+ * body.
+ *
+ * The caller must hold the dshash partition lock. The entry cannot be found
+ * by other backends until that lock is released, so it is safe to publish
+ * refcount/dropped/body here. Caller needs to increment the refcount further
+ * if a longer-lived reference is needed.
+ *
+ * chunk must be a valid pointer from pgstat_alloc_entry_body().
*/
PgStatShared_Common *
pgstat_init_entry(PgStat_Kind kind,
- PgStatShared_HashEntry *shhashent)
+ PgStatShared_HashEntry *shhashent,
+ dsa_pointer chunk)
{
- /* Create new stats entry. */
- dsa_pointer chunk;
PgStatShared_Common *shheader;
const PgStat_KindInfo *kind_info = pgstat_get_kind_info(kind);
- /*
- * Initialize refcount to 1, marking it as valid / not dropped. The entry
- * can't be freed before the initialization because it can't be found as
- * long as we hold the dshash partition lock. Caller needs to increase
- * further if a longer lived reference is needed.
- */
- pg_atomic_init_u32(&shhashent->refcount, 1);
-
- /*
- * Initialize "generation" to 0, as freshly created.
- */
- pg_atomic_init_u32(&shhashent->generation, 0);
- shhashent->dropped = false;
-
- chunk = dsa_allocate_extended(pgStatLocal.dsa,
- kind_info->shared_size,
- DSA_ALLOC_ZERO | DSA_ALLOC_NO_OOM);
- if (chunk == InvalidDsaPointer)
- return NULL;
+ Assert(DsaPointerIsValid(chunk));
shheader = dsa_get_address(pgStatLocal.dsa, chunk);
shheader->magic = 0xdeadbeef;
- /* Link the new entry from the hash entry. */
+ pg_atomic_init_u32(&shhashent->refcount, 1);
+ pg_atomic_init_u32(&shhashent->generation, 0);
+ shhashent->dropped = false;
shhashent->body = chunk;
/* Increment entry count, if required. */
@@ -545,6 +551,24 @@ pgstat_get_entry_ref(PgStat_Kind kind, Oid dboid, uint64 objid, bool create,
if (create && !shhashent)
{
bool shfound;
+ dsa_pointer chunk;
+
+ /*
+ * Allocate the stats body before inserting a hash entry. Creating a
+ * new DSA segment can raise ERROR (e.g. ENOSPC on posix shm); doing
+ * that after the insert would leave a live hash entry with an
+ * invalid body.
+ */
+ chunk = pgstat_alloc_entry_body(kind);
+ if (chunk == InvalidDsaPointer)
+ {
+ pgstat_release_entry_ref(key, entry_ref, false);
+ ereport(ERROR,
+ (errcode(ERRCODE_OUT_OF_MEMORY),
+ errmsg("out of memory"),
+ errdetail("Failed while allocating entry %u/%u/%" PRIu64 ".",
+ key.kind, key.dboid, key.objid)));
+ }
/*
* It's possible that somebody created the entry since the above
@@ -556,6 +580,8 @@ pgstat_get_entry_ref(PgStat_Kind kind, Oid dboid, uint64 objid, bool create,
DSHASH_INSERT_NO_OOM);
if (!shhashent)
{
+ dsa_free(pgStatLocal.dsa, chunk);
+
/*
* Clean up the local reference when failing insert into the
* shared hashtable.
@@ -570,24 +596,7 @@ 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 both the
- * local reference and the shared hashtable before giving up.
- * Clean the local state first, since releasing the dshash
- * lock can process a pending interrupt.
- */
- pgstat_release_entry_ref(key, entry_ref, false);
- 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 ".",
- key.kind, key.dboid, key.objid)));
- }
+ shheader = pgstat_init_entry(kind, shhashent, chunk);
pgstat_acquire_entry_ref(entry_ref, shhashent, shheader);
if (created_entry != NULL)
@@ -595,6 +604,9 @@ pgstat_get_entry_ref(PgStat_Kind kind, Oid dboid, uint64 objid, bool create,
return entry_ref;
}
+
+ /* Concurrent insert won; drop the unused body. */
+ dsa_free(pgStatLocal.dsa, chunk);
}
if (!shhashent)
diff --git a/src/include/utils/pgstat_internal.h b/src/include/utils/pgstat_internal.h
index 14369e59a1c..201e57279b1 100644
--- a/src/include/utils/pgstat_internal.h
+++ b/src/include/utils/pgstat_internal.h
@@ -839,8 +839,10 @@ extern void pgstat_reset_matching_entries(bool (*do_reset) (PgStatShared_HashEnt
TimestampTz ts);
extern void pgstat_request_entry_refs_gc(void);
+extern dsa_pointer pgstat_alloc_entry_body(PgStat_Kind kind);
extern PgStatShared_Common *pgstat_init_entry(PgStat_Kind kind,
- PgStatShared_HashEntry *shhashent);
+ PgStatShared_HashEntry *shhashent,
+ dsa_pointer chunk);
/*
--
2.54.0 (Apple Git-157)
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: DSA_ALLOC_NO_OOM vs dsm_create ERROR leaving a half-initialized pgstats hash entry
2026-09-08 09:06 DSA_ALLOC_NO_OOM vs dsm_create ERROR leaving a half-initialized pgstats hash entry Grigorev Jurij <ju.grigorev@ftdata.ru>
2026-09-08 23:52 ` Re: DSA_ALLOC_NO_OOM vs dsm_create ERROR leaving a half-initialized pgstats hash entry Michael Paquier <michael@paquier.xyz>
2026-09-09 10:56 ` Re: DSA_ALLOC_NO_OOM vs dsm_create ERROR leaving a half-initialized pgstats hash entry Grigorev Jurij <ju.grigorev@ftdata.ru>
2026-09-17 02:48 ` Re: DSA_ALLOC_NO_OOM vs dsm_create ERROR leaving a half-initialized pgstats hash entry Michael Paquier <michael@paquier.xyz>
2026-09-17 03:44 ` Re: DSA_ALLOC_NO_OOM vs dsm_create ERROR leaving a half-initialized pgstats hash entry Michael Paquier <michael@paquier.xyz>
2026-09-17 06:19 ` Re: DSA_ALLOC_NO_OOM vs dsm_create ERROR leaving a half-initialized pgstats hash entry Grigorev Jurij <ju.grigorev@ftdata.ru>
@ 2026-09-17 07:10 ` Michael Paquier <michael@paquier.xyz>
2026-09-17 08:35 ` Re: DSA_ALLOC_NO_OOM vs dsm_create ERROR leaving a half-initialized pgstats hash entry Grigorev Jurij <ju.grigorev@ftdata.ru>
0 siblings, 1 reply; 10+ messages in thread
From: Michael Paquier @ 2026-09-17 07:10 UTC (permalink / raw)
To: Grigorev Jurij <ju.grigorev@ftdata.ru>; +Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Thu, Sep 17, 2026 at 06:19:19AM +0000, Grigorev Jurij wrote:
> The check you proposed in the follow-up [1] looks right to me. A partial
> reference at this point cannot have shared_stats or pending data, and
> pgstat_release_entry_ref() can remove it without touching shared state.
> It handles the backend-local partial reference, while this patch prevents
> the same failure from leaving an inconsistent shared hash entry. I agree
> that the two changes should remain independent, as you suggested.
My second patch is an extra defense.. I guess that we should just do
that first.
> I have prepared and build-tested versions for the supported branches
> down to REL_15_STABLE, but I am not attaching them yet so that the HEAD
> change can be reviewed first. I can send them once the shape of this
> patch is agreed.
Something that I did not consider yet is how much we should worry
about the ABI change of pgstat_init_entry().. I could see nothing in
the open that uses it, but the risk seems non-zero. Perhaps we should
just limit that on HEAD.
> Do you think this warrants a deterministic test?
Nah. That feels like a waste of test cycles for what's already a very
narrow case.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (832B, ../../aquSa2gbfVMxDYmT@paquier.xyz/2-signature.asc)
download
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: DSA_ALLOC_NO_OOM vs dsm_create ERROR leaving a half-initialized pgstats hash entry
2026-09-08 09:06 DSA_ALLOC_NO_OOM vs dsm_create ERROR leaving a half-initialized pgstats hash entry Grigorev Jurij <ju.grigorev@ftdata.ru>
2026-09-08 23:52 ` Re: DSA_ALLOC_NO_OOM vs dsm_create ERROR leaving a half-initialized pgstats hash entry Michael Paquier <michael@paquier.xyz>
2026-09-09 10:56 ` Re: DSA_ALLOC_NO_OOM vs dsm_create ERROR leaving a half-initialized pgstats hash entry Grigorev Jurij <ju.grigorev@ftdata.ru>
2026-09-17 02:48 ` Re: DSA_ALLOC_NO_OOM vs dsm_create ERROR leaving a half-initialized pgstats hash entry Michael Paquier <michael@paquier.xyz>
2026-09-17 03:44 ` Re: DSA_ALLOC_NO_OOM vs dsm_create ERROR leaving a half-initialized pgstats hash entry Michael Paquier <michael@paquier.xyz>
2026-09-17 06:19 ` Re: DSA_ALLOC_NO_OOM vs dsm_create ERROR leaving a half-initialized pgstats hash entry Grigorev Jurij <ju.grigorev@ftdata.ru>
2026-09-17 07:10 ` Re: DSA_ALLOC_NO_OOM vs dsm_create ERROR leaving a half-initialized pgstats hash entry Michael Paquier <michael@paquier.xyz>
@ 2026-09-17 08:35 ` Grigorev Jurij <ju.grigorev@ftdata.ru>
2026-09-23 23:54 ` Re: DSA_ALLOC_NO_OOM vs dsm_create ERROR leaving a half-initialized pgstats hash entry Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 10+ messages in thread
From: Grigorev Jurij @ 2026-09-17 08:35 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Agreed on all three points!
The two changes touch separate parts of pgstat_shmem.c, so the current
HEAD patch should apply cleanly on top of the defensive change. I do
not expect any conflicts, but I will rebase and resend it if needed.
And yeah, regarding the ABI concern, even though pgstat_init_entry()
is declared in pgstat_internal.h and I found no external users, changing
its signature in stable branches does not seem worth the compatibility
risk. I therefore agree that this patch should be limited to HEAD.
And agreed, no injection test for this narrow path!
Thanks,
Yuriy
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: DSA_ALLOC_NO_OOM vs dsm_create ERROR leaving a half-initialized pgstats hash entry
2026-09-08 09:06 DSA_ALLOC_NO_OOM vs dsm_create ERROR leaving a half-initialized pgstats hash entry Grigorev Jurij <ju.grigorev@ftdata.ru>
2026-09-08 23:52 ` Re: DSA_ALLOC_NO_OOM vs dsm_create ERROR leaving a half-initialized pgstats hash entry Michael Paquier <michael@paquier.xyz>
2026-09-09 10:56 ` Re: DSA_ALLOC_NO_OOM vs dsm_create ERROR leaving a half-initialized pgstats hash entry Grigorev Jurij <ju.grigorev@ftdata.ru>
2026-09-17 02:48 ` Re: DSA_ALLOC_NO_OOM vs dsm_create ERROR leaving a half-initialized pgstats hash entry Michael Paquier <michael@paquier.xyz>
2026-09-17 03:44 ` Re: DSA_ALLOC_NO_OOM vs dsm_create ERROR leaving a half-initialized pgstats hash entry Michael Paquier <michael@paquier.xyz>
2026-09-17 06:19 ` Re: DSA_ALLOC_NO_OOM vs dsm_create ERROR leaving a half-initialized pgstats hash entry Grigorev Jurij <ju.grigorev@ftdata.ru>
2026-09-17 07:10 ` Re: DSA_ALLOC_NO_OOM vs dsm_create ERROR leaving a half-initialized pgstats hash entry Michael Paquier <michael@paquier.xyz>
2026-09-17 08:35 ` Re: DSA_ALLOC_NO_OOM vs dsm_create ERROR leaving a half-initialized pgstats hash entry Grigorev Jurij <ju.grigorev@ftdata.ru>
@ 2026-09-23 23:54 ` Michael Paquier <michael@paquier.xyz>
2026-09-24 03:39 ` Re: DSA_ALLOC_NO_OOM vs dsm_create ERROR leaving a half-initialized pgstats hash entry Grigorev Jurij <ju.grigorev@ftdata.ru>
0 siblings, 1 reply; 10+ messages in thread
From: Michael Paquier @ 2026-09-23 23:54 UTC (permalink / raw)
To: Grigorev Jurij <ju.grigorev@ftdata.ru>; +Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Thu, Sep 17, 2026 at 08:35:15AM +0000, Grigorev Jurij wrote:
> The two changes touch separate parts of pgstat_shmem.c, so the current
> HEAD patch should apply cleanly on top of the defensive change. I do
> not expect any conflicts, but I will rebase and resend it if needed.
I have put my head down on this patch. There was a lot of prose in
it, so I have spent time refining and making the whole shorter.
Another thing was some of the existing prose that got deleted, like
the comments at the top of "generation" and "refcount" when
initializing a new entry.
Cleaned up all that, and applied on HEAD.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (832B, ../../arRmqBEj9n0w3qCc@paquier.xyz/2-signature.asc)
download
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: DSA_ALLOC_NO_OOM vs dsm_create ERROR leaving a half-initialized pgstats hash entry
2026-09-08 09:06 DSA_ALLOC_NO_OOM vs dsm_create ERROR leaving a half-initialized pgstats hash entry Grigorev Jurij <ju.grigorev@ftdata.ru>
2026-09-08 23:52 ` Re: DSA_ALLOC_NO_OOM vs dsm_create ERROR leaving a half-initialized pgstats hash entry Michael Paquier <michael@paquier.xyz>
2026-09-09 10:56 ` Re: DSA_ALLOC_NO_OOM vs dsm_create ERROR leaving a half-initialized pgstats hash entry Grigorev Jurij <ju.grigorev@ftdata.ru>
2026-09-17 02:48 ` Re: DSA_ALLOC_NO_OOM vs dsm_create ERROR leaving a half-initialized pgstats hash entry Michael Paquier <michael@paquier.xyz>
2026-09-17 03:44 ` Re: DSA_ALLOC_NO_OOM vs dsm_create ERROR leaving a half-initialized pgstats hash entry Michael Paquier <michael@paquier.xyz>
2026-09-17 06:19 ` Re: DSA_ALLOC_NO_OOM vs dsm_create ERROR leaving a half-initialized pgstats hash entry Grigorev Jurij <ju.grigorev@ftdata.ru>
2026-09-17 07:10 ` Re: DSA_ALLOC_NO_OOM vs dsm_create ERROR leaving a half-initialized pgstats hash entry Michael Paquier <michael@paquier.xyz>
2026-09-17 08:35 ` Re: DSA_ALLOC_NO_OOM vs dsm_create ERROR leaving a half-initialized pgstats hash entry Grigorev Jurij <ju.grigorev@ftdata.ru>
2026-09-23 23:54 ` Re: DSA_ALLOC_NO_OOM vs dsm_create ERROR leaving a half-initialized pgstats hash entry Michael Paquier <michael@paquier.xyz>
@ 2026-09-24 03:39 ` Grigorev Jurij <ju.grigorev@ftdata.ru>
0 siblings, 0 replies; 10+ messages in thread
From: Grigorev Jurij @ 2026-09-24 03:39 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Hi Michael,
Thanks very much for taking the time to clean this up and commit it. I
reviewed d9de60c -- restoring the existing refcount and generation
comments was the right call, and the shorter wording still captures the
allocation-before-insert invariant. The final ordering and cleanup
paths look good to me.
Thanks,
Yuriy
^ permalink raw reply [nested|flat] 10+ messages in thread
end of thread, other threads:[~2026-09-24 03:39 UTC | newest]
Thread overview: 10+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2026-09-08 09:06 DSA_ALLOC_NO_OOM vs dsm_create ERROR leaving a half-initialized pgstats hash entry Grigorev Jurij <ju.grigorev@ftdata.ru>
2026-09-08 23:52 ` Michael Paquier <michael@paquier.xyz>
2026-09-09 10:56 ` Grigorev Jurij <ju.grigorev@ftdata.ru>
2026-09-17 02:48 ` Michael Paquier <michael@paquier.xyz>
2026-09-17 03:44 ` Michael Paquier <michael@paquier.xyz>
2026-09-17 06:19 ` Grigorev Jurij <ju.grigorev@ftdata.ru>
2026-09-17 07:10 ` Michael Paquier <michael@paquier.xyz>
2026-09-17 08:35 ` Grigorev Jurij <ju.grigorev@ftdata.ru>
2026-09-23 23:54 ` Michael Paquier <michael@paquier.xyz>
2026-09-24 03:39 ` Grigorev Jurij <ju.grigorev@ftdata.ru>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox