diff --git a/src/backend/utils/activity/pgstat.c b/src/backend/utils/activity/pgstat.c index 5177f880f70..6dd13ab9dec 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,24 +2096,12 @@ 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. */ - p = dshash_find_or_insert(pgStatLocal.shared_hash, &key, &found); - - /* don't allow duplicate entries */ - if (found) + chunk = pgstat_alloc_entry_body(key.kind); + if (chunk == InvalidDsaPointer) { - dshash_release_lock(pgStatLocal.shared_hash, p); - 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 @@ -2122,6 +2111,35 @@ pgstat_read_statsfile(void) 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); + + /* + * for the same reason as previously, ERROR not + * WARNING + */ + 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, 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 5feed5f842a..1858d68f918 100644 --- a/src/backend/utils/activity/pgstat_shmem.c +++ b/src/backend/utils/activity/pgstat_shmem.c @@ -303,21 +303,36 @@ 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 in the event of an allocation failure. + */ +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 variable-numbered pgstats entry. + * + * "chunk" must be a valid pointer, allocated previously by + * 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); + Assert(DsaPointerIsValid(chunk)); + /* * 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 @@ -332,12 +347,6 @@ pgstat_init_entry(PgStat_Kind kind, 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; - shheader = dsa_get_address(pgStatLocal.dsa, chunk); shheader->magic = 0xdeadbeef; @@ -545,6 +554,19 @@ 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. */ + 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 +578,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 +594,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 +602,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); /*