From 4e3891d8f5348db7e67b0b7bae35301a0dc921fd Mon Sep 17 00:00:00 2001 From: Sami Imseih Date: Thu, 11 Jun 2026 07:30:38 -0500 Subject: [PATCH v1 1/1] pgstat: allow a stats kind to use its own dedicated dshash The pgstat infrastructure stores all variable-numbered stats entries in a single shared dshash table. Any operation that needs entries of one specific kind must iterate past all entries of every other kind. Stats can now set a new variable-numbered stats option own_hash to get a dedicated dshash table. Lookups and inserts are routed to the correct hash via pgstat_get_hash_for_kind(). Benefits of a dedicated hash beyond avoiding unnecessary iteration: - Reduced lock contention: kinds with their own hash don't compete for partition locks with unrelated stats kinds. - Independent sizing: a dedicated hash can be sized appropriately for the expected entry count of that specific kind. - Cleaner GC/drop: entry cleanup doesn't have to scan past entries of other kinds. Two new inline functions are introduced in pgstat_internal.h: - pgstat_get_hash_for_kind(): returns the dedicated dshash table for the given kind, or falls back to the shared hash. - pgstat_get_dsa_for_kind(): returns the dedicated DSA area for the given kind, or falls back to the shared DSA. Only variable-numbered stats kinds can opt in, since fixed-amount kinds have their storage pre-allocated at startup and their stats are not stored with other kinds in a dshash. The test_custom_var_stats module is updated to also test own_hash=true. --- src/backend/utils/activity/pgstat.c | 226 ++++++++-------- src/backend/utils/activity/pgstat_shmem.c | 244 +++++++++++++----- src/include/utils/pgstat_internal.h | 44 ++++ .../test_custom_stats/t/001_custom_stats.pl | 160 +++++++----- .../test_custom_var_stats--1.0.sql | 10 + .../test_custom_stats/test_custom_var_stats.c | 107 +++++++- 6 files changed, 545 insertions(+), 246 deletions(-) diff --git a/src/backend/utils/activity/pgstat.c b/src/backend/utils/activity/pgstat.c index 9234854b8b5..b4a0faf2e6f 100644 --- a/src/backend/utils/activity/pgstat.c +++ b/src/backend/utils/activity/pgstat.c @@ -1183,52 +1183,55 @@ pgstat_build_snapshot(void) /* * Snapshot all variable stats. */ - dshash_seq_init(&hstat, pgStatLocal.shared_hash, false); - while ((p = dshash_seq_next(&hstat)) != NULL) + for (int h = 0; h < pgStatLocal.num_hashes; h++) { - PgStat_Kind kind = p->key.kind; - const PgStat_KindInfo *kind_info = pgstat_get_kind_info(kind); - bool found; - PgStat_SnapshotEntry *entry; - PgStatShared_Common *stats_data; - - /* - * Check if the stats object should be included in the snapshot. - * Unless the stats kind can be accessed from all databases (e.g., - * database stats themselves), we only include stats for the current - * database or objects not associated with a database (e.g. shared - * relations). - */ - if (p->key.dboid != MyDatabaseId && - p->key.dboid != InvalidOid && - !kind_info->accessed_across_databases) - continue; - - if (p->dropped) - continue; + dshash_seq_init(&hstat, pgStatLocal.all_hashes[h], false); + while ((p = dshash_seq_next(&hstat)) != NULL) + { + PgStat_Kind kind = p->key.kind; + const PgStat_KindInfo *kind_info = pgstat_get_kind_info(kind); + bool found; + PgStat_SnapshotEntry *entry; + PgStatShared_Common *stats_data; + + /* + * Check if the stats object should be included in the snapshot. + * Unless the stats kind can be accessed from all databases (e.g., + * database stats themselves), we only include stats for the + * current database or objects not associated with a database + * (e.g. shared relations). + */ + if (p->key.dboid != MyDatabaseId && + p->key.dboid != InvalidOid && + !kind_info->accessed_across_databases) + continue; - Assert(pg_atomic_read_u32(&p->refcount) > 0); + if (p->dropped) + continue; - stats_data = dsa_get_address(pgStatLocal.dsa, p->body); - Assert(stats_data); + Assert(pg_atomic_read_u32(&p->refcount) > 0); - entry = pgstat_snapshot_insert(pgStatLocal.snapshot.stats, p->key, &found); - Assert(!found); + stats_data = dsa_get_address(pgstat_get_dsa_for_kind(kind), p->body); + Assert(stats_data); - entry->data = MemoryContextAlloc(pgStatLocal.snapshot.context, - pgstat_get_entry_len(kind)); + entry = pgstat_snapshot_insert(pgStatLocal.snapshot.stats, p->key, &found); + Assert(!found); - /* - * Acquire the LWLock directly instead of using - * pg_stat_lock_entry_shared() which requires a reference. - */ - LWLockAcquire(&stats_data->lock, LW_SHARED); - memcpy(entry->data, - pgstat_get_entry_data(kind, stats_data), - pgstat_get_entry_len(kind)); - LWLockRelease(&stats_data->lock); + entry->data = MemoryContextAlloc(pgStatLocal.snapshot.context, + pgstat_get_entry_len(kind)); + + /* + * Acquire the LWLock directly instead of using + * pg_stat_lock_entry_shared() which requires a reference. + */ + LWLockAcquire(&stats_data->lock, LW_SHARED); + memcpy(entry->data, + pgstat_get_entry_data(kind, stats_data), + pgstat_get_entry_len(kind)); + LWLockRelease(&stats_data->lock); + } + dshash_seq_term(&hstat); } - dshash_seq_term(&hstat); /* * Build snapshot of all fixed-numbered stats. @@ -1553,6 +1556,10 @@ pgstat_register_kind(PgStat_Kind kind, const PgStat_KindInfo *kind_info) ereport(ERROR, (errmsg("failed to register custom cumulative statistics \"%s\" with ID %u", kind_info->name, kind), errhint("Custom cumulative statistics cannot use entry count tracking for fixed-numbered objects."))); + if (kind_info->own_hash) + ereport(ERROR, + (errmsg("failed to register custom cumulative statistics \"%s\" with ID %u", kind_info->name, kind), + errhint("Custom cumulative statistics cannot use a dedicated hash table for fixed-numbered objects."))); } else { @@ -1708,82 +1715,85 @@ pgstat_write_statsfile(void) /* * Walk through the stats entries */ - dshash_seq_init(&hstat, pgStatLocal.shared_hash, false); - while ((ps = dshash_seq_next(&hstat)) != NULL) + for (int h = 0; h < pgStatLocal.num_hashes && status == STATS_WRITE; h++) { - PgStatShared_Common *shstats; - const PgStat_KindInfo *kind_info = NULL; - - CHECK_FOR_INTERRUPTS(); - - /* - * We should not see any "dropped" entries when writing the stats - * file, as all backends and auxiliary processes should have cleaned - * up their references before they terminated. - * - * However, since we are already shutting down, it is not worth - * crashing the server over any potential cleanup issues, so we simply - * skip such entries if encountered. - */ - Assert(!ps->dropped); - if (ps->dropped) - continue; - - /* - * This discards data related to custom stats kinds that are unknown - * to this process. - */ - if (!pgstat_is_kind_valid(ps->key.kind)) + dshash_seq_init(&hstat, pgStatLocal.all_hashes[h], false); + while ((ps = dshash_seq_next(&hstat)) != NULL) { - elog(WARNING, "found unknown stats entry %u/%u/%" PRIu64, - ps->key.kind, ps->key.dboid, - ps->key.objid); - continue; - } - - shstats = (PgStatShared_Common *) dsa_get_address(pgStatLocal.dsa, ps->body); - - kind_info = pgstat_get_kind_info(ps->key.kind); - - /* if not dropped the valid-entry refcount should exist */ - Assert(pg_atomic_read_u32(&ps->refcount) > 0); + PgStatShared_Common *shstats; + const PgStat_KindInfo *kind_info = NULL; + + CHECK_FOR_INTERRUPTS(); + + /* + * We should not see any "dropped" entries when writing the stats + * file, as all backends and auxiliary processes should have + * cleaned up their references before they terminated. + * + * However, since we are already shutting down, it is not worth + * crashing the server over any potential cleanup issues, so we + * simply skip such entries if encountered. + */ + Assert(!ps->dropped); + if (ps->dropped) + continue; - /* skip if no need to write to file */ - if (!kind_info->write_to_file) - continue; + /* + * This discards data related to custom stats kinds that are + * unknown to this process. + */ + if (!pgstat_is_kind_valid(ps->key.kind)) + { + elog(WARNING, "found unknown stats entry %u/%u/%" PRIu64, + ps->key.kind, ps->key.dboid, + ps->key.objid); + continue; + } - if (!kind_info->to_serialized_name) - { - /* normal stats entry, identified by PgStat_HashKey */ - fputc(PGSTAT_FILE_ENTRY_HASH, fpout); - write_chunk_s(fpout, &ps->key); - } - else - { - /* stats entry identified by name on disk (e.g. slots) */ - NameData name; + shstats = (PgStatShared_Common *) dsa_get_address(pgstat_get_dsa_for_kind(ps->key.kind), ps->body); - kind_info->to_serialized_name(&ps->key, shstats, &name); + kind_info = pgstat_get_kind_info(ps->key.kind); - fputc(PGSTAT_FILE_ENTRY_NAME, fpout); - write_chunk_s(fpout, &ps->key.kind); - write_chunk_s(fpout, &name); - } + /* if not dropped the valid-entry refcount should exist */ + Assert(pg_atomic_read_u32(&ps->refcount) > 0); - /* Write except the header part of the entry */ - write_chunk(fpout, - pgstat_get_entry_data(ps->key.kind, shstats), - pgstat_get_entry_len(ps->key.kind)); + /* skip if no need to write to file */ + if (!kind_info->write_to_file) + continue; - /* Write more data for the entry, if required */ - if (kind_info->to_serialized_data && - !kind_info->to_serialized_data(&ps->key, shstats, fpout)) - { - status = STATS_DISCARD; - break; + if (!kind_info->to_serialized_name) + { + /* normal stats entry, identified by PgStat_HashKey */ + fputc(PGSTAT_FILE_ENTRY_HASH, fpout); + write_chunk_s(fpout, &ps->key); + } + else + { + /* stats entry identified by name on disk (e.g. slots) */ + NameData name; + + kind_info->to_serialized_name(&ps->key, shstats, &name); + + fputc(PGSTAT_FILE_ENTRY_NAME, fpout); + write_chunk_s(fpout, &ps->key.kind); + write_chunk_s(fpout, &name); + } + + /* Write except the header part of the entry */ + write_chunk(fpout, + pgstat_get_entry_data(ps->key.kind, shstats), + pgstat_get_entry_len(ps->key.kind)); + + /* Write more data for the entry, if required */ + if (kind_info->to_serialized_data && + !kind_info->to_serialized_data(&ps->key, shstats, fpout)) + { + status = STATS_DISCARD; + break; + } } + dshash_seq_term(&hstat); } - dshash_seq_term(&hstat); /* * No more output to be done. Close the temp file and replace the old @@ -2065,12 +2075,12 @@ pgstat_read_statsfile(void) * putting all stats into checkpointer's * pgStatEntryRefHash would be wasted effort and memory. */ - p = dshash_find_or_insert(pgStatLocal.shared_hash, &key, &found); + p = dshash_find_or_insert(pgstat_get_hash_for_kind(key.kind), &key, &found); /* don't allow duplicate entries */ if (found) { - dshash_release_lock(pgStatLocal.shared_hash, p); + dshash_release_lock(pgstat_get_hash_for_kind(key.kind), p); elog(WARNING, "found duplicate stats entry %u/%u/%" PRIu64 " of type %c", key.kind, key.dboid, key.objid, t); @@ -2078,7 +2088,7 @@ pgstat_read_statsfile(void) } header = pgstat_init_entry(key.kind, p); - dshash_release_lock(pgStatLocal.shared_hash, p); + dshash_release_lock(pgstat_get_hash_for_kind(key.kind), p); if (header == NULL) { /* diff --git a/src/backend/utils/activity/pgstat_shmem.c b/src/backend/utils/activity/pgstat_shmem.c index 5ea3f1973f9..04b17db9ee7 100644 --- a/src/backend/utils/activity/pgstat_shmem.c +++ b/src/backend/utils/activity/pgstat_shmem.c @@ -139,6 +139,17 @@ StatsShmemSize(void) sz = MAXALIGN(sizeof(PgStat_ShmemControl)); sz = add_size(sz, pgstat_dsa_init_size()); + /* Add per-kind DSA space for own_hash kinds */ + for (PgStat_Kind kind = PGSTAT_KIND_MIN; kind <= PGSTAT_KIND_MAX; kind++) + { + const PgStat_KindInfo *kind_info = pgstat_get_kind_info(kind); + + if (!kind_info || kind_info->fixed_amount || !kind_info->own_hash) + continue; + + sz = add_size(sz, pgstat_dsa_init_size()); + } + /* Add shared memory for all the custom fixed-numbered statistics */ for (PgStat_Kind kind = PGSTAT_KIND_CUSTOM_MIN; kind <= PGSTAT_KIND_CUSTOM_MAX; kind++) { @@ -210,6 +221,41 @@ StatsShmemInit(void *arg) /* lift limit set above */ dsa_set_size_limit(dsa, -1); + /* + * Create dedicated DSA areas and hash tables for kinds that have + * variable-numbered stats and set own_hash to true. + */ + memset(ctl->kind_hash_valid, 0, sizeof(ctl->kind_hash_valid)); + memset(ctl->raw_kind_dsa_area, 0, sizeof(ctl->raw_kind_dsa_area)); + for (PgStat_Kind kind = PGSTAT_KIND_MIN; kind <= PGSTAT_KIND_MAX; kind++) + { + const PgStat_KindInfo *kind_info = pgstat_get_kind_info(kind); + dsa_area *kind_dsa; + dshash_table *kind_dsh; + + if (!kind_info || kind_info->fixed_amount || !kind_info->own_hash) + continue; + + /* Create a per-kind DSA in pre-allocated shared memory */ + ctl->raw_kind_dsa_area[kind] = p; + p += pgstat_dsa_init_size(); + kind_dsa = dsa_create_in_place(ctl->raw_kind_dsa_area[kind], + pgstat_dsa_init_size(), + LWTRANCHE_PGSTATS_DSA, NULL); + dsa_pin(kind_dsa); + + /* Temporarily limit to keep dshash in the in-place area */ + dsa_set_size_limit(kind_dsa, pgstat_dsa_init_size()); + + kind_dsh = dshash_create(kind_dsa, &dsh_params, NULL); + ctl->kind_hash_handles[kind] = dshash_get_hash_table_handle(kind_dsh); + ctl->kind_hash_valid[kind] = true; + + dsa_set_size_limit(kind_dsa, -1); + dshash_detach(kind_dsh); + dsa_detach(kind_dsa); + } + /* * Postmaster will never access these again, thus free the local * dsa/dshash references. @@ -257,6 +303,8 @@ pgstat_attach_shmem(void) { MemoryContext oldcontext; + pgStatLocal.num_hashes = 0; + Assert(pgStatLocal.dsa == NULL); /* stats shared memory persists for the backend lifetime */ @@ -270,6 +318,36 @@ pgstat_attach_shmem(void) pgStatLocal.shmem->hash_handle, NULL); + /* Attach dedicated DSAs and hash tables */ + for (PgStat_Kind kind = PGSTAT_KIND_MIN; kind <= PGSTAT_KIND_MAX; kind++) + { + if (pgStatLocal.shmem->kind_hash_valid[kind]) + { + pgStatLocal.kind_dsa[kind] = + dsa_attach_in_place(pgStatLocal.shmem->raw_kind_dsa_area[kind], + NULL); + dsa_pin_mapping(pgStatLocal.kind_dsa[kind]); + + pgStatLocal.kind_hash[kind] = + dshash_attach(pgStatLocal.kind_dsa[kind], &dsh_params, + pgStatLocal.shmem->kind_hash_handles[kind], + NULL); + } + else + { + pgStatLocal.kind_dsa[kind] = NULL; + pgStatLocal.kind_hash[kind] = NULL; + } + } + + /* Populate array of all hash tables */ + pgStatLocal.all_hashes[pgStatLocal.num_hashes++] = pgStatLocal.shared_hash; + for (PgStat_Kind kind = PGSTAT_KIND_MIN; kind <= PGSTAT_KIND_MAX; kind++) + { + if (pgStatLocal.kind_hash[kind] != NULL) + pgStatLocal.all_hashes[pgStatLocal.num_hashes++] = pgStatLocal.kind_hash[kind]; + } + MemoryContextSwitchTo(oldcontext); } @@ -284,6 +362,21 @@ pgstat_detach_shmem(void) dshash_detach(pgStatLocal.shared_hash); pgStatLocal.shared_hash = NULL; + /* Detach dedicated hash tables and DSAs */ + for (PgStat_Kind kind = PGSTAT_KIND_MIN; kind <= PGSTAT_KIND_MAX; kind++) + { + if (pgStatLocal.kind_hash[kind] != NULL) + { + dshash_detach(pgStatLocal.kind_hash[kind]); + pgStatLocal.kind_hash[kind] = NULL; + } + if (pgStatLocal.kind_dsa[kind] != NULL) + { + dsa_detach(pgStatLocal.kind_dsa[kind]); + pgStatLocal.kind_dsa[kind] = NULL; + } + } + dsa_detach(pgStatLocal.dsa); /* @@ -293,6 +386,12 @@ pgstat_detach_shmem(void) */ dsa_release_in_place(pgStatLocal.shmem->raw_dsa_area); + for (PgStat_Kind kind = PGSTAT_KIND_MIN; kind <= PGSTAT_KIND_MAX; kind++) + { + if (pgStatLocal.shmem->raw_kind_dsa_area[kind] != NULL) + dsa_release_in_place(pgStatLocal.shmem->raw_kind_dsa_area[kind]); + } + pgStatLocal.dsa = NULL; } @@ -332,13 +431,13 @@ pgstat_init_entry(PgStat_Kind kind, pg_atomic_init_u32(&shhashent->generation, 0); shhashent->dropped = false; - chunk = dsa_allocate_extended(pgStatLocal.dsa, + chunk = dsa_allocate_extended(pgstat_get_dsa_for_kind(kind), kind_info->shared_size, DSA_ALLOC_ZERO | DSA_ALLOC_NO_OOM); if (chunk == InvalidDsaPointer) return NULL; - shheader = dsa_get_address(pgStatLocal.dsa, chunk); + shheader = dsa_get_address(pgstat_get_dsa_for_kind(kind), chunk); shheader->magic = 0xdeadbeef; /* Link the new entry from the hash entry. */ @@ -358,7 +457,7 @@ pgstat_reinit_entry(PgStat_Kind kind, PgStatShared_HashEntry *shhashent) { PgStatShared_Common *shheader; - shheader = dsa_get_address(pgStatLocal.dsa, shhashent->body); + shheader = dsa_get_address(pgstat_get_dsa_for_kind(kind), shhashent->body); /* mark as not dropped anymore */ pg_atomic_fetch_add_u32(&shhashent->refcount, 1); @@ -404,7 +503,7 @@ pgstat_acquire_entry_ref(PgStat_EntryRef *entry_ref, pg_atomic_fetch_add_u32(&shhashent->refcount, 1); - dshash_release_lock(pgStatLocal.shared_hash, shhashent); + dshash_release_lock(pgstat_get_hash_for_kind(shhashent->key.kind), shhashent); entry_ref->shared_stats = shheader; entry_ref->shared_entry = shhashent; @@ -478,6 +577,7 @@ pgstat_get_entry_ref(PgStat_Kind kind, Oid dboid, uint64 objid, bool create, PgStatShared_HashEntry *shhashent; PgStatShared_Common *shheader = NULL; PgStat_EntryRef *entry_ref; + dshash_table *hash; key.kind = kind; key.dboid = dboid; @@ -521,7 +621,9 @@ pgstat_get_entry_ref(PgStat_Kind kind, Oid dboid, uint64 objid, bool create, * 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. */ - shhashent = dshash_find(pgStatLocal.shared_hash, &key, false); + hash = pgstat_get_hash_for_kind(kind); + + shhashent = dshash_find(hash, &key, false); if (create && !shhashent) { @@ -532,7 +634,7 @@ pgstat_get_entry_ref(PgStat_Kind kind, Oid dboid, uint64 objid, bool create, * lookup. If so, fall through to the same path as if we'd have if it * already had been created before the dshash_find() calls. */ - shhashent = dshash_find_or_insert(pgStatLocal.shared_hash, &key, &shfound); + shhashent = dshash_find_or_insert(hash, &key, &shfound); if (!shfound) { shheader = pgstat_init_entry(kind, shhashent); @@ -542,7 +644,7 @@ pgstat_get_entry_ref(PgStat_Kind kind, Oid dboid, uint64 objid, bool create, * Failed the allocation of a new entry, so clean up the * shared hashtable before giving up. */ - dshash_delete_entry(pgStatLocal.shared_hash, shhashent); + dshash_delete_entry(hash, shhashent); ereport(ERROR, (errcode(ERRCODE_OUT_OF_MEMORY), @@ -598,14 +700,14 @@ pgstat_get_entry_ref(PgStat_Kind kind, Oid dboid, uint64 objid, bool create, } else if (shhashent->dropped) { - dshash_release_lock(pgStatLocal.shared_hash, shhashent); + dshash_release_lock(hash, shhashent); pgstat_release_entry_ref(key, entry_ref, false); return NULL; } else { - shheader = dsa_get_address(pgStatLocal.dsa, shhashent->body); + shheader = dsa_get_address(pgstat_get_dsa_for_kind(kind), shhashent->body); pgstat_acquire_entry_ref(entry_ref, shhashent, shheader); return entry_ref; @@ -647,7 +749,7 @@ 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(pgStatLocal.shared_hash, + shent = dshash_find(pgstat_get_hash_for_kind(key.kind), &entry_ref->shared_entry->key, true); if (!shent) @@ -672,7 +774,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(pgStatLocal.shared_hash, shent); + dshash_release_lock(pgstat_get_hash_for_kind(key.kind), shent); } } } @@ -886,11 +988,11 @@ pgstat_free_entry(PgStatShared_HashEntry *shent, dshash_seq_status *hstat) pdsa = shent->body; if (!hstat) - dshash_delete_entry(pgStatLocal.shared_hash, shent); + dshash_delete_entry(pgstat_get_hash_for_kind(kind), shent); else dshash_delete_current(hstat); - dsa_free(pgStatLocal.dsa, pdsa); + dsa_free(pgstat_get_dsa_for_kind(kind), pdsa); /* Decrement entry count, if required. */ if (pgstat_get_kind_info(kind)->track_entry_count) @@ -929,7 +1031,7 @@ pgstat_drop_entry_internal(PgStatShared_HashEntry *shent, else { if (!hstat) - dshash_release_lock(pgStatLocal.shared_hash, shent); + dshash_release_lock(pgstat_get_hash_for_kind(shent->key.kind), shent); return false; } } @@ -959,25 +1061,29 @@ pgstat_drop_database_and_contents(Oid dboid) pgstat_release_db_entry_refs(dboid); /* some of the dshash entries are to be removed, take exclusive lock. */ - dshash_seq_init(&hstat, pgStatLocal.shared_hash, true); - while ((p = dshash_seq_next(&hstat)) != NULL) + for (int h = 0; h < pgStatLocal.num_hashes; h++) { - if (p->dropped) - continue; + dshash_seq_init(&hstat, pgStatLocal.all_hashes[h], true); + while ((p = dshash_seq_next(&hstat)) != NULL) + { + if (p->dropped) + continue; - if (p->key.dboid != dboid) - continue; + if (p->key.dboid != dboid) + continue; - if (!pgstat_drop_entry_internal(p, &hstat)) - { - /* - * Even statistics for a dropped database might currently be - * accessed (consider e.g. database stats for pg_stat_database). - */ - not_freed_count++; + if (!pgstat_drop_entry_internal(p, &hstat)) + { + /* + * Even statistics for a dropped database might currently be + * accessed (consider e.g. database stats for + * pg_stat_database). + */ + not_freed_count++; + } } + dshash_seq_term(&hstat); } - dshash_seq_term(&hstat); /* * If some of the stats data could not be freed, signal the reference @@ -1023,8 +1129,8 @@ pgstat_drop_entry(PgStat_Kind kind, Oid dboid, uint64 objid, true); } - /* mark entry in shared hashtable as deleted, drop if possible */ - shent = dshash_find(pgStatLocal.shared_hash, &key, true); + /* mark entry in the kind's hashtable as deleted, drop if possible */ + shent = dshash_find(pgstat_get_hash_for_kind(kind), &key, true); if (shent) { if (shent->dropped) @@ -1037,7 +1143,7 @@ pgstat_drop_entry(PgStat_Kind kind, Oid dboid, uint64 objid, shent->key.objid, pg_atomic_read_u32(&shent->refcount), pg_atomic_read_u32(&shent->generation)); - dshash_release_lock(pgStatLocal.shared_hash, shent); + dshash_release_lock(pgstat_get_hash_for_kind(kind), shent); return true; } @@ -1057,8 +1163,8 @@ pgstat_drop_entry(PgStat_Kind kind, Oid dboid, uint64 objid, } /* - * Scan through the shared hashtable of stats, dropping statistics if - * approved by the optional do_drop() function. + * Scan through all stats hash tables, dropping statistics if approved by the + * optional do_drop() function. */ void pgstat_drop_matching_entries(bool (*do_drop) (PgStatShared_HashEntry *, Datum), @@ -1069,37 +1175,40 @@ pgstat_drop_matching_entries(bool (*do_drop) (PgStatShared_HashEntry *, Datum), uint64 not_freed_count = 0; /* entries are removed, take an exclusive lock */ - dshash_seq_init(&hstat, pgStatLocal.shared_hash, true); - while ((ps = dshash_seq_next(&hstat)) != NULL) + for (int h = 0; h < pgStatLocal.num_hashes; h++) { - if (ps->dropped) - continue; + dshash_seq_init(&hstat, pgStatLocal.all_hashes[h], true); + while ((ps = dshash_seq_next(&hstat)) != NULL) + { + if (ps->dropped) + continue; - if (do_drop != NULL && !do_drop(ps, match_data)) - continue; + if (do_drop != NULL && !do_drop(ps, match_data)) + continue; - /* delete local reference */ - if (pgStatEntryRefHash) - { - PgStat_EntryRefHashEntry *lohashent = - pgstat_entry_ref_hash_lookup(pgStatEntryRefHash, ps->key); + /* delete local reference */ + if (pgStatEntryRefHash) + { + PgStat_EntryRefHashEntry *lohashent = + pgstat_entry_ref_hash_lookup(pgStatEntryRefHash, ps->key); - if (lohashent) - pgstat_release_entry_ref(lohashent->key, lohashent->entry_ref, - true); - } + if (lohashent) + pgstat_release_entry_ref(lohashent->key, lohashent->entry_ref, + true); + } - if (!pgstat_drop_entry_internal(ps, &hstat)) - not_freed_count++; + if (!pgstat_drop_entry_internal(ps, &hstat)) + not_freed_count++; + } + dshash_seq_term(&hstat); } - dshash_seq_term(&hstat); if (not_freed_count > 0) pgstat_request_entry_refs_gc(); } /* - * Scan through the shared hashtable of stats and drop all entries. + * Scan through all stats hash tables and drop all entries. */ void pgstat_drop_all_entries(void) @@ -1140,8 +1249,8 @@ pgstat_reset_entry(PgStat_Kind kind, Oid dboid, uint64 objid, TimestampTz ts) } /* - * Scan through the shared hashtable of stats, resetting statistics if - * approved by the provided do_reset() function. + * Scan through all stats hash tables, resetting statistics if approved by the + * provided do_reset() function. */ void pgstat_reset_matching_entries(bool (*do_reset) (PgStatShared_HashEntry *, Datum), @@ -1151,26 +1260,29 @@ pgstat_reset_matching_entries(bool (*do_reset) (PgStatShared_HashEntry *, Datum) PgStatShared_HashEntry *p; /* dshash entry is not modified, take shared lock */ - dshash_seq_init(&hstat, pgStatLocal.shared_hash, false); - while ((p = dshash_seq_next(&hstat)) != NULL) + for (int h = 0; h < pgStatLocal.num_hashes; h++) { - PgStatShared_Common *header; + dshash_seq_init(&hstat, pgStatLocal.all_hashes[h], false); + while ((p = dshash_seq_next(&hstat)) != NULL) + { + PgStatShared_Common *header; - if (p->dropped) - continue; + if (p->dropped) + continue; - if (!do_reset(p, match_data)) - continue; + if (!do_reset(p, match_data)) + continue; - header = dsa_get_address(pgStatLocal.dsa, p->body); + header = dsa_get_address(pgstat_get_dsa_for_kind(p->key.kind), p->body); - LWLockAcquire(&header->lock, LW_EXCLUSIVE); + LWLockAcquire(&header->lock, LW_EXCLUSIVE); - shared_stat_reset_contents(p->key.kind, header, ts); + shared_stat_reset_contents(p->key.kind, header, ts); - LWLockRelease(&header->lock); + LWLockRelease(&header->lock); + } + dshash_seq_term(&hstat); } - dshash_seq_term(&hstat); } static bool diff --git a/src/include/utils/pgstat_internal.h b/src/include/utils/pgstat_internal.h index e62122b883b..96ac8bd1370 100644 --- a/src/include/utils/pgstat_internal.h +++ b/src/include/utils/pgstat_internal.h @@ -251,6 +251,12 @@ typedef struct PgStat_KindInfo */ bool track_entry_count:1; + /* + * Should entries of this kind be stored in a dedicated dshash table + * rather than the shared hash table? For variable-numbered stats only. + */ + bool own_hash:1; + /* * The size of an entry in the shared stats hash table (pointed to by * PgStatShared_HashEntry->body). For fixed-numbered statistics, this is @@ -549,6 +555,11 @@ typedef struct PgStat_ShmemControl */ dshash_table_handle hash_handle; /* shared dbstat hash */ + /* Per-kind hash and DSA for kinds with own_hash set */ + dshash_table_handle kind_hash_handles[PGSTAT_KIND_MAX + 1]; + void *raw_kind_dsa_area[PGSTAT_KIND_MAX + 1]; + bool kind_hash_valid[PGSTAT_KIND_MAX + 1]; + /* Has the stats system already been shut down? Just a debugging check. */ bool is_shutdown; @@ -638,7 +649,13 @@ typedef struct PgStat_LocalState { PgStat_ShmemControl *shmem; dsa_area *dsa; + dsa_area *kind_dsa[PGSTAT_KIND_MAX + 1]; dshash_table *shared_hash; + dshash_table *kind_hash[PGSTAT_KIND_MAX + 1]; + + /* All hash tables to iterate (shared + per-kind), built at attach time */ + dshash_table *all_hashes[PGSTAT_KIND_MAX + 1]; + int num_hashes; /* the current statistics snapshot */ PgStat_Snapshot snapshot; @@ -1054,4 +1071,31 @@ pgstat_get_custom_snapshot_data(PgStat_Kind kind) return pgStatLocal.snapshot.custom_data[idx]; } +/* + * Returns the dshash table for a given kind. Kinds with own_hash set have + * a dedicated hash table; others use the shared hash. + */ +static inline dshash_table * +pgstat_get_hash_for_kind(PgStat_Kind kind) +{ + if (pgStatLocal.kind_hash[kind] != NULL) + return pgStatLocal.kind_hash[kind]; + + Assert(!pgstat_get_kind_info(kind)->own_hash); + return pgStatLocal.shared_hash; +} + +/* + * Returns the DSA area for a given kind. Kinds with own_hash set have + * a dedicated DSA; others use the shared DSA. + */ +static inline dsa_area * +pgstat_get_dsa_for_kind(PgStat_Kind kind) +{ + if (pgStatLocal.kind_dsa[kind] != NULL) + return pgStatLocal.kind_dsa[kind]; + + return pgStatLocal.dsa; +} + #endif /* PGSTAT_INTERNAL_H */ 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 69f2284229e..b855f435db4 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 @@ -8,6 +8,9 @@ # - Persistence across restarts. # - Loss after crash recovery. # - Resets for fixed-sized stats. +# +# Variable-sized stats are tested with both own_hash=true (dedicated +# dshash) and own_hash=false (shared dshash) to cover both paths. use strict; use warnings FATAL => 'all'; @@ -17,6 +20,7 @@ use PostgreSQL::Test::Utils; use Test::More; use File::Copy; +my $result; my $node = PostgreSQL::Test::Cluster->new('main'); $node->init; $node->append_conf('postgresql.conf', @@ -28,7 +32,7 @@ $node->safe_psql('postgres', q(CREATE EXTENSION test_custom_var_stats)); $node->safe_psql('postgres', q(CREATE EXTENSION test_custom_fixed_stats)); # Verify custom stats kinds appear in pg_stat_kind_info. -my $result = $node->safe_psql( +$result = $node->safe_psql( 'postgres', q(SELECT id, name, builtin, fixed_amount, accessed_across_databases, write_to_file @@ -39,82 +43,100 @@ is( $result, 26|test_custom_fixed_stats|f|t|f|t}, "custom stats kinds visible in pg_stat_kind_info"); -# Create entries for variable-sized stats. -$node->safe_psql('postgres', - q(select test_custom_stats_var_create('entry1', 'Test entry 1'))); -$node->safe_psql('postgres', - q(select test_custom_stats_var_create('entry2', 'Test entry 2'))); -$node->safe_psql('postgres', - q(select test_custom_stats_var_create('entry3', 'Test entry 3'))); -$node->safe_psql('postgres', - q(select test_custom_stats_var_create('entry4', 'Test entry 4'))); - -# Update counters: entry1=2, entry2=3, entry3=2, entry4=3, fixed=3 -$node->safe_psql('postgres', - q(select test_custom_stats_var_update('entry1'))); -$node->safe_psql('postgres', - q(select test_custom_stats_var_update('entry1'))); -$node->safe_psql('postgres', - q(select test_custom_stats_var_update('entry2'))); -$node->safe_psql('postgres', - q(select test_custom_stats_var_update('entry2'))); -$node->safe_psql('postgres', - q(select test_custom_stats_var_update('entry2'))); -$node->safe_psql('postgres', - q(select test_custom_stats_var_update('entry3'))); -$node->safe_psql('postgres', - q(select test_custom_stats_var_update('entry3'))); -$node->safe_psql('postgres', - q(select test_custom_stats_var_update('entry4'))); -$node->safe_psql('postgres', - q(select test_custom_stats_var_update('entry4'))); -$node->safe_psql('postgres', - q(select test_custom_stats_var_update('entry4'))); +# Test variable-sized stats with both own_hash configurations (true and false). +foreach my $use_own_hash (qw(true false)) +{ + + # Restart with the appropriate setting. + $node->append_conf('postgresql.conf', + "test_custom_var_stats.use_own_hash = $use_own_hash"); + $node->restart; + + $result = $node->safe_psql('postgres', + q(select test_custom_stats_var_is_own_hash())); + is( $result, + $use_own_hash eq 'true' ? 't' : 'f', + "check if dedicated hash is allocated (own_hash=$use_own_hash)"); + + # Create entries for variable-sized stats. + $node->safe_psql('postgres', + q(select test_custom_stats_var_create('entry1', 'Test entry 1'))); + $node->safe_psql('postgres', + q(select test_custom_stats_var_create('entry2', 'Test entry 2'))); + $node->safe_psql('postgres', + q(select test_custom_stats_var_create('entry3', 'Test entry 3'))); + $node->safe_psql('postgres', + 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', + q(select test_custom_stats_var_update('entry1'))); + $node->safe_psql('postgres', + q(select test_custom_stats_var_update('entry1'))); + $node->safe_psql('postgres', + q(select test_custom_stats_var_update('entry2'))); + $node->safe_psql('postgres', + q(select test_custom_stats_var_update('entry2'))); + $node->safe_psql('postgres', + q(select test_custom_stats_var_update('entry2'))); + $node->safe_psql('postgres', + q(select test_custom_stats_var_update('entry3'))); + $node->safe_psql('postgres', + q(select test_custom_stats_var_update('entry3'))); + $node->safe_psql('postgres', + q(select test_custom_stats_var_update('entry4'))); + $node->safe_psql('postgres', + q(select test_custom_stats_var_update('entry4'))); + $node->safe_psql('postgres', + q(select test_custom_stats_var_update('entry4'))); + + # Test data reports. + $result = $node->safe_psql('postgres', + q(select * from test_custom_stats_var_report('entry1'))); + is( $result, + "entry1|2|Test entry 1", + "report for variable-sized data of entry1"); + + $result = $node->safe_psql('postgres', + q(select * from test_custom_stats_var_report('entry2'))); + is( $result, + "entry2|3|Test entry 2", + "report for variable-sized data of entry2"); + + $result = $node->safe_psql('postgres', + q(select * from test_custom_stats_var_report('entry3'))); + is( $result, + "entry3|2|Test entry 3", + "report for variable-sized data of entry3"); + + $result = $node->safe_psql('postgres', + q(select * from test_custom_stats_var_report('entry4'))); + is( $result, + "entry4|3|Test entry 4", + "report for variable-sized data of entry4"); + + # Test drop of variable-sized stats. + $node->safe_psql('postgres', + q(select * from test_custom_stats_var_drop('entry3'))); + $result = $node->safe_psql('postgres', + q(select * from test_custom_stats_var_report('entry3'))); + is($result, "", "entry3 not found after drop"); + $node->safe_psql('postgres', + q(select * from test_custom_stats_var_drop('entry4'))); + $result = $node->safe_psql('postgres', + q(select * from test_custom_stats_var_report('entry4'))); + is($result, "", "entry4 not found after drop"); +} + +# fixed-sized stats are updated 3 times, so the report below should match. $node->safe_psql('postgres', q(select test_custom_stats_fixed_update())); $node->safe_psql('postgres', q(select test_custom_stats_fixed_update())); $node->safe_psql('postgres', q(select test_custom_stats_fixed_update())); -# Test data reports. -$result = $node->safe_psql('postgres', - q(select * from test_custom_stats_var_report('entry1'))); -is( $result, - "entry1|2|Test entry 1", - "report for variable-sized data of entry1"); - -$result = $node->safe_psql('postgres', - q(select * from test_custom_stats_var_report('entry2'))); -is( $result, - "entry2|3|Test entry 2", - "report for variable-sized data of entry2"); - -$result = $node->safe_psql('postgres', - q(select * from test_custom_stats_var_report('entry3'))); -is( $result, - "entry3|2|Test entry 3", - "report for variable-sized data of entry3"); - -$result = $node->safe_psql('postgres', - q(select * from test_custom_stats_var_report('entry4'))); -is( $result, - "entry4|3|Test entry 4", - "report for variable-sized data of entry4"); - $result = $node->safe_psql('postgres', q(select * from test_custom_stats_fixed_report())); is($result, "3|", "report for fixed-sized stats"); -# Test drop of variable-sized stats. -$node->safe_psql('postgres', - q(select * from test_custom_stats_var_drop('entry3'))); -$result = $node->safe_psql('postgres', - q(select * from test_custom_stats_var_report('entry3'))); -is($result, "", "entry3 not found after drop"); -$node->safe_psql('postgres', - q(select * from test_custom_stats_var_drop('entry4'))); -$result = $node->safe_psql('postgres', - q(select * from test_custom_stats_var_report('entry4'))); -is($result, "", "entry4 not found after drop"); - # Test persistence across clean restart. $node->stop(); $node->start(); diff --git a/src/test/modules/test_custom_stats/test_custom_var_stats--1.0.sql b/src/test/modules/test_custom_stats/test_custom_var_stats--1.0.sql index 5ed8cfc2dcf..d8c07d874f9 100644 --- a/src/test/modules/test_custom_stats/test_custom_var_stats--1.0.sql +++ b/src/test/modules/test_custom_stats/test_custom_var_stats--1.0.sql @@ -24,3 +24,13 @@ CREATE FUNCTION test_custom_stats_var_report(INOUT name TEXT, RETURNS SETOF record AS 'MODULE_PATHNAME', 'test_custom_stats_var_report' LANGUAGE C STRICT PARALLEL UNSAFE; + +CREATE FUNCTION test_custom_stats_var_scan() +RETURNS BIGINT +AS 'MODULE_PATHNAME', 'test_custom_stats_var_scan' +LANGUAGE C STRICT PARALLEL UNSAFE; + +CREATE FUNCTION test_custom_stats_var_is_own_hash() +RETURNS BOOLEAN +AS 'MODULE_PATHNAME', 'test_custom_stats_var_is_own_hash' +LANGUAGE C STRICT PARALLEL UNSAFE; diff --git a/src/test/modules/test_custom_stats/test_custom_var_stats.c b/src/test/modules/test_custom_stats/test_custom_var_stats.c index a39ada0b67c..e9ac15c243d 100644 --- a/src/test/modules/test_custom_stats/test_custom_var_stats.c +++ b/src/test/modules/test_custom_stats/test_custom_var_stats.c @@ -15,6 +15,7 @@ #include "access/htup_details.h" #include "common/hashfn.h" #include "funcapi.h" +#include "utils/guc.h" #include "storage/dsm_registry.h" #include "storage/fd.h" #include "utils/builtins.h" @@ -111,7 +112,27 @@ static void test_custom_stats_var_finish(PgStat_StatsFileOp status); *-------------------------------------------------------------------------- */ -static const PgStat_KindInfo custom_stats = { +/* Whether to use a dedicated dshash for this kind */ +static bool test_custom_stats_use_own_hash = false; + +static const PgStat_KindInfo custom_stats_own_hash = { + .name = "test_custom_var_stats", + .fixed_amount = false, /* variable number of entries */ + .write_to_file = true, /* persist across restarts */ + .track_entry_count = true, /* count active entries */ + .accessed_across_databases = true, /* global statistics */ + .own_hash = true, /* use dedicated dshash */ + .shared_size = sizeof(PgStatShared_CustomVarEntry), + .shared_data_off = offsetof(PgStatShared_CustomVarEntry, stats), + .shared_data_len = sizeof(((PgStatShared_CustomVarEntry *) 0)->stats), + .pending_size = sizeof(PgStat_StatCustomVarEntry), + .flush_pending_cb = test_custom_stats_var_flush_pending_cb, + .to_serialized_data = test_custom_stats_var_to_serialized_data, + .from_serialized_data = test_custom_stats_var_from_serialized_data, + .finish = test_custom_stats_var_finish, +}; + +static const PgStat_KindInfo custom_stats_shared_hash = { .name = "test_custom_var_stats", .fixed_amount = false, /* variable number of entries */ .write_to_file = true, /* persist across restarts */ @@ -135,8 +156,29 @@ static const PgStat_KindInfo custom_stats = { void _PG_init(void) { - /* Register custom statistics kind */ - pgstat_register_kind(PGSTAT_KIND_TEST_CUSTOM_VAR_STATS, &custom_stats); + /* Must be loaded via shared_preload_libraries */ + if (!process_shared_preload_libraries_in_progress) + return; + + /* + * test_custom_stats_use_own_hash = (on|off) + * + * Use the shared hash (default) or a dedicated hash for the + * test_custom_var_stats kind. + */ + DefineCustomBoolVariable("test_custom_var_stats.use_own_hash", + "Use dedicated dshash for test custom var stats", + NULL, + &test_custom_stats_use_own_hash, + false, + PGC_POSTMASTER, + 0, + NULL, NULL, NULL); + + /* Register with the appropriate kind info */ + pgstat_register_kind(PGSTAT_KIND_TEST_CUSTOM_VAR_STATS, + test_custom_stats_use_own_hash ? + &custom_stats_own_hash : &custom_stats_shared_hash); } /*-------------------------------------------------------------------------- @@ -701,3 +743,62 @@ test_custom_stats_var_report(PG_FUNCTION_ARGS) SRF_RETURN_DONE(funcctx); } + +/* + * test_custom_stats_var_scan + * Count entries by scanning the kind's dedicated dshash directly + * + * Exercises pgstat_get_hash_for_kind() and dshash_seq iteration on + * a dedicated hash table. + */ +PG_FUNCTION_INFO_V1(test_custom_stats_var_scan); +Datum +test_custom_stats_var_scan(PG_FUNCTION_ARGS) +{ + dshash_table *hash; + dshash_seq_status hstat; + PgStatShared_HashEntry *p; + int64 count = 0; + + hash = pgstat_get_hash_for_kind(PGSTAT_KIND_TEST_CUSTOM_VAR_STATS); + + dshash_seq_init(&hstat, hash, false); + while ((p = dshash_seq_next(&hstat)) != NULL) + { + if (p->dropped) + continue; + + if (p->key.kind != PGSTAT_KIND_TEST_CUSTOM_VAR_STATS) + continue; + + count++; + } + dshash_seq_term(&hstat); + + PG_RETURN_INT64(count); +} + +/* + * test_custom_stats_var_is_own_hash + * Verify whether the kind uses a dedicated dshash + * + * Scans pgStatLocal.all_hashes[] looking for the hash returned by + * pgstat_get_hash_for_kind(). Index 0 is always the shared hash, + * so finding it at a non-zero index confirms it has its own hash. + */ +PG_FUNCTION_INFO_V1(test_custom_stats_var_is_own_hash); +Datum +test_custom_stats_var_is_own_hash(PG_FUNCTION_ARGS) +{ + dshash_table *hash; + + hash = pgstat_get_hash_for_kind(PGSTAT_KIND_TEST_CUSTOM_VAR_STATS); + + for (int i = 0; i < pgStatLocal.num_hashes; i++) + { + if (pgStatLocal.all_hashes[i] == hash) + PG_RETURN_BOOL(i != 0); + } + + PG_RETURN_BOOL(false); +} -- 2.50.1 (Apple Git-155)