From 7bc1c596e3952ccbd9845e88a01848c09723bf64 Mon Sep 17 00:00:00 2001 From: Michael Paquier Date: Thu, 8 Oct 2026 16:42:41 +0900 Subject: [PATCH v5 3/3] Michael's edits --- src/include/utils/pgstat_internal.h | 4 +- src/backend/utils/activity/pgstat.c | 12 ++-- src/backend/utils/activity/pgstat_shmem.c | 55 ++++++++----------- .../test_custom_stats/t/001_custom_stats.pl | 21 ++++--- 4 files changed, 41 insertions(+), 51 deletions(-) diff --git a/src/include/utils/pgstat_internal.h b/src/include/utils/pgstat_internal.h index 82655b81c64f..64857a41564c 100644 --- a/src/include/utils/pgstat_internal.h +++ b/src/include/utils/pgstat_internal.h @@ -576,8 +576,8 @@ typedef struct PgStat_ShmemControl /* * Counters for the number of entries associated to a single - * variable-numbered stats kind. These counters can be enabled on a - * per-kind basis, when track_entry_count is set. This counter is + * variable-numbered stats kind. These counters can be enabled on a + * per-kind basis, when track_entry_count is set. A counter is * incremented each time a new entry is created (not reused) in a stats * hashtable, and is decremented each time an entry is freed from a stats * hashtable. diff --git a/src/backend/utils/activity/pgstat.c b/src/backend/utils/activity/pgstat.c index 2c51404d26f9..0cac003e5f96 100644 --- a/src/backend/utils/activity/pgstat.c +++ b/src/backend/utils/activity/pgstat.c @@ -1746,7 +1746,8 @@ pgstat_write_statsfile(void) } /* - * Walk through the stats entries + * Walk through the stats entries, as long as writes can happen (note that + * switching to a STATS_DISCARD status is possible while walking through). */ for (int h = 0; h < pgStatLocal.num_hashes && status == STATS_WRITE; h++) { @@ -1903,7 +1904,6 @@ pgstat_read_statsfile(void) PgStat_StatsFileOp status = STATS_READ; const char *statfile = PGSTAT_STAT_PERMANENT_FILENAME; PgStat_ShmemControl *shmem = pgStatLocal.shmem; - dshash_table *hash; /* shouldn't be called from postmaster */ Assert(IsUnderPostmaster || !IsPostmasterEnvironment); @@ -2105,8 +2105,6 @@ pgstat_read_statsfile(void) Assert(key.kind == kind); } - hash = pgStatLocal.kind_hash[key.kind]; - /* * This intentionally doesn't use pgstat_get_entry_ref() - * putting all stats into checkpointer's @@ -2127,7 +2125,7 @@ pgstat_read_statsfile(void) key.objid, t); } - p = dshash_find_or_insert_extended(hash, + p = dshash_find_or_insert_extended(pgStatLocal.kind_hash[key.kind], &key, &found, DSHASH_INSERT_NO_OOM); if (!p) @@ -2146,7 +2144,7 @@ pgstat_read_statsfile(void) /* don't allow duplicate entries */ if (found) { - dshash_release_lock(hash, p); + dshash_release_lock(pgStatLocal.kind_hash[key.kind], p); dsa_free(pgStatLocal.kind_dsa[key.kind], chunk); elog(WARNING, "found duplicate stats entry %u/%u/%" PRIu64 " of type %c", key.kind, key.dboid, @@ -2155,7 +2153,7 @@ pgstat_read_statsfile(void) } header = pgstat_init_entry(key.kind, p, chunk); - dshash_release_lock(hash, p); + dshash_release_lock(pgStatLocal.kind_hash[key.kind], p); 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 81cb28b6ebf3..86bef0e354b6 100644 --- a/src/backend/utils/activity/pgstat_shmem.c +++ b/src/backend/utils/activity/pgstat_shmem.c @@ -327,6 +327,7 @@ pgstat_attach_shmem(void) { dsa_area *kind_dsa; + /* per-kind hashtable */ kind_dsa = dsa_attach_in_place(pgStatLocal.shmem->raw_kind_dsa_area[kind], NULL); @@ -342,11 +343,13 @@ pgstat_attach_shmem(void) } else if (kind_info && !kind_info->fixed_amount) { + /* main shared hashtable */ pgStatLocal.kind_hash[kind] = shared_hash; pgStatLocal.kind_dsa[kind] = shared_dsa; } else { + /* unassigned kind ID */ pgStatLocal.kind_hash[kind] = NULL; pgStatLocal.kind_dsa[kind] = NULL; } @@ -410,9 +413,8 @@ dsa_pointer pgstat_alloc_entry_body(PgStat_Kind kind) { const PgStat_KindInfo *kind_info = pgstat_get_kind_info(kind); - dsa_area *dsa = pgStatLocal.kind_dsa[kind]; - return dsa_allocate_extended(dsa, + return dsa_allocate_extended(pgStatLocal.kind_dsa[kind], kind_info->shared_size, DSA_ALLOC_ZERO | DSA_ALLOC_NO_OOM); } @@ -430,7 +432,6 @@ pgstat_init_entry(PgStat_Kind kind, { PgStatShared_Common *shheader; const PgStat_KindInfo *kind_info = pgstat_get_kind_info(kind); - dsa_area *dsa = pgStatLocal.kind_dsa[kind]; Assert(DsaPointerIsValid(chunk)); @@ -448,7 +449,7 @@ pgstat_init_entry(PgStat_Kind kind, pg_atomic_init_u32(&shhashent->generation, 0); shhashent->dropped = false; - shheader = dsa_get_address(dsa, chunk); + shheader = dsa_get_address(pgStatLocal.kind_dsa[kind], chunk); shheader->magic = 0xdeadbeef; /* Link the new entry from the hash entry. */ @@ -464,12 +465,12 @@ pgstat_init_entry(PgStat_Kind kind, } static PgStatShared_Common * -pgstat_reinit_entry(PgStat_Kind kind, dsa_area *dsa, - PgStatShared_HashEntry *shhashent) +pgstat_reinit_entry(PgStat_Kind kind, PgStatShared_HashEntry *shhashent) { PgStatShared_Common *shheader; - shheader = dsa_get_address(dsa, shhashent->body); + shheader = dsa_get_address(pgStatLocal.kind_dsa[kind], + shhashent->body); /* mark as not dropped anymore */ pg_atomic_fetch_add_u32(&shhashent->refcount, 1); @@ -650,13 +651,14 @@ pgstat_get_entry_ref(PgStat_Kind kind, Oid dboid, uint64 objid, bool create, Assert(entry_ref != NULL); + /* Per-kind hashtable and DSA area */ + hash = pgStatLocal.kind_hash[kind]; + dsa = pgStatLocal.kind_dsa[kind]; + /* * Do a lookup in the hash table first - it's quite likely that the entry * already exists, and that way we only need a shared lock. */ - hash = pgStatLocal.kind_hash[kind]; - dsa = pgStatLocal.kind_dsa[kind]; - shhashent = dshash_find(hash, &key, false); if (create && !shhashent) @@ -689,8 +691,8 @@ pgstat_get_entry_ref(PgStat_Kind kind, Oid dboid, uint64 objid, bool create, dsa_free(dsa, chunk); /* - * Clean up the local reference when failing to insert into the - * stats hashtable. + * Clean up the local reference when failing insert into the stats + * hashtable. */ pgstat_release_entry_ref(key, entry_ref, false); ereport(ERROR, @@ -744,7 +746,7 @@ pgstat_get_entry_ref(PgStat_Kind kind, Oid dboid, uint64 objid, bool create, * stats to their plain state, while incrementing its "generation" * in the shared entry for any remaining local references. */ - shheader = pgstat_reinit_entry(kind, dsa, shhashent); + shheader = pgstat_reinit_entry(kind, shhashent); pgstat_acquire_entry_ref(entry_ref, hash, shhashent, shheader); if (created_entry != NULL) @@ -793,7 +795,6 @@ pgstat_release_entry_ref(PgStat_HashKey key, PgStat_EntryRef *entry_ref, */ if (pg_atomic_fetch_sub_u32(&entry_ref->shared_entry->refcount, 1) == 1) { - dshash_table *hash = pgStatLocal.kind_hash[key.kind]; PgStatShared_HashEntry *shent; /* @@ -804,7 +805,8 @@ pgstat_release_entry_ref(PgStat_HashKey key, PgStat_EntryRef *entry_ref, /* only dropped entries can reach a 0 refcount */ Assert(entry_ref->shared_entry->dropped); - shent = dshash_find(hash, &entry_ref->shared_entry->key, true); + shent = dshash_find(pgStatLocal.kind_hash[key.kind], + &entry_ref->shared_entry->key, true); if (!shent) elog(ERROR, "could not find just referenced shared stats entry"); @@ -827,7 +829,7 @@ pgstat_release_entry_ref(PgStat_HashKey key, PgStat_EntryRef *entry_ref, * Shared stats entry has been reinitialized, so do not drop * its shared entry, only release its lock. */ - dshash_release_lock(hash, shent); + dshash_release_lock(pgStatLocal.kind_hash[key.kind], shent); } } } @@ -1040,29 +1042,24 @@ pgstat_release_db_entry_refs(Oid dboid) static void pgstat_free_entry(PgStatShared_HashEntry *shent, dshash_seq_status *hstat) { - dshash_table *hash; - dsa_area *dsa; dsa_pointer pdsa; PgStat_Kind kind = shent->key.kind; - const PgStat_KindInfo *kind_info = pgstat_get_kind_info(kind); /* * Fetch dsa pointer before deleting entry - that way we can free the * memory after releasing the lock. */ - hash = pgStatLocal.kind_hash[kind]; - dsa = pgStatLocal.kind_dsa[kind]; pdsa = shent->body; if (!hstat) - dshash_delete_entry(hash, shent); + dshash_delete_entry(pgStatLocal.kind_hash[kind], shent); else dshash_delete_current(hstat); - dsa_free(dsa, pdsa); + dsa_free(pgStatLocal.kind_dsa[kind], pdsa); /* Decrement entry count, if required. */ - if (kind_info && kind_info->track_entry_count) + if (pgstat_get_kind_info(kind)->track_entry_count) pg_atomic_sub_fetch_u64(&pgStatLocal.shmem->entry_counts[kind - 1], 1); } @@ -1178,14 +1175,12 @@ pgstat_drop_entry(PgStat_Kind kind, Oid dboid, uint64 objid, bool missing_ok) { PgStat_HashKey key = {0}; - dshash_table *hash; PgStatShared_HashEntry *shent; bool freed = true; Assert(kind >= PGSTAT_KIND_MIN && kind <= PGSTAT_KIND_MAX); Assert(pgStatLocal.kind_hash[kind] != NULL); - hash = pgStatLocal.kind_hash[kind]; key.kind = kind; key.dboid = dboid; @@ -1203,22 +1198,20 @@ pgstat_drop_entry(PgStat_Kind kind, Oid dboid, uint64 objid, } /* mark entry in the stats hashtable as deleted, drop if possible */ - shent = dshash_find(hash, &key, true); + shent = dshash_find(pgStatLocal.kind_hash[kind], &key, true); if (shent) { if (shent->dropped) { - const PgStat_KindInfo *kind_info = pgstat_get_kind_info(shent->key.kind); - if (!missing_ok) elog(ERROR, "trying to drop stats entry already dropped: kind=%s dboid=%u objid=%" PRIu64 " refcount=%u generation=%u", - kind_info->name, + pgstat_get_kind_info(shent->key.kind)->name, shent->key.dboid, shent->key.objid, pg_atomic_read_u32(&shent->refcount), pg_atomic_read_u32(&shent->generation)); - dshash_release_lock(hash, shent); + dshash_release_lock(pgStatLocal.kind_hash[kind], shent); return true; } diff --git a/src/test/modules/test_custom_stats/t/001_custom_stats.pl b/src/test/modules/test_custom_stats/t/001_custom_stats.pl index d298b02d74fc..873a006ba3ed 100644 --- a/src/test/modules/test_custom_stats/t/001_custom_stats.pl +++ b/src/test/modules/test_custom_stats/t/001_custom_stats.pl @@ -68,14 +68,12 @@ foreach my $stats_kind (@var_stats_kinds) 'postgres', q(SELECT own_hash FROM pg_stat_kind_info WHERE name = current_setting('test_custom_var_stats.kind'))); - is( $result, - $own_hash, + is($result, $own_hash, "pg_stat_kind_info reports own_hash=$own_hash for $kind_name"); $result = $node->safe_psql('postgres', q(select test_custom_stats_var_is_own_hash())); - is( $result, - $own_hash, + is($result, $own_hash, "check if dedicated hash is allocated for $kind_name"); # Create entries for variable-sized stats. @@ -89,7 +87,8 @@ foreach my $stats_kind (@var_stats_kinds) q(select test_custom_stats_var_create('entry4', 'Test entry 4'))); # Update counters: entry1=2, entry2=3, entry3=2, entry4=3 - $node->safe_psql('postgres', + $node->safe_psql( + 'postgres', q(select test_custom_stats_var_update(name) from (values ('entry1', 2), ('entry2', 3), ('entry3', 2), ('entry4', 3)) as stats(name, n), @@ -168,14 +167,14 @@ foreach my $stats_kind (@var_stats_kinds) $result = $node->safe_psql('postgres', q(select * from test_custom_stats_var_report('entry1'))); - is( $result, - "", - "variable-sized stats of entry1 lost after crash recovery for $kind_name"); + is($result, "", + "variable-sized stats of entry1 lost after crash recovery for $kind_name" + ); $result = $node->safe_psql('postgres', q(select * from test_custom_stats_var_report('entry2'))); - is( $result, - "", - "variable-sized stats of entry2 lost after crash recovery for $kind_name"); + is($result, "", + "variable-sized stats of entry2 lost after crash recovery for $kind_name" + ); } # fixed-sized stats are updated 3 times, so the report below should match. -- 2.55.0