On Thu, Sep 25, 2025 at 07:47:48PM -0500, Sami Imseih wrote: > I spent a bit of time testing this with a pg_stat_statements like extension > using a custom stats kind, and while I think there is value for both "live" > ( ! ->dropped) counter and an exact dshash counter ( current proposal), > I rather go with the latter at least initially, for the sake of not having 2 > atomic counters. Both will allow an extension to trigger some type of a > cleanup strategy, and in general, both should be very close in value. > That's at least my observation.
Thanks for the feedback. I'm feeling encouraged by this opinion about
the "hard" counter, so I'd like to move on with it.
> I do think however that the placement of the decrement is wrong, and that
> it should go inside pgstat_free_entry, since pgstat_free_entry can be called
> in multiple code paths. In my high churn workload, v2 ended up increasing way
> beyond the actual size of the dshash. See the attached for what
> I did to fix.
Yes, you are right and the patch is wrong. I've done the following
with my previous patch with injection_points loaded and its stats
activated, confirming your argument:
1) session with psql running in a tight loop and these queries:
select injection_points_attach('popo', 'notice');
select injection_points_run('popo');
select injection_points_detach('popo');
2) session that fetches the stats entry
select injection_points_stats_numcalls('popo');
\watch 0
3) session that checks the number of entries, should be 0 or 1:
select injection_points_stats_count();
\watch 1
And the numbers reported by the third session increase over time,
which is no good, once my patch is used. Once we move the
decrementation to pgstat_free_entry() as you propose, the counter is
under control and capped.
> IMO, "entry_counts" does not sound correct. We are not tracking more
> than one count. What about track_num_entries ?
Hmm. track_entry_counts resonates with the field name entry_counts,
and it's true that using the plural form is confusing. How about
track_entry_count in PgStat_KindInfo instead?
--
Michael
From 4761314ce27f8aaebaa531e8068513cf9924680b Mon Sep 17 00:00:00 2001 From: Michael Paquier <[email protected]> Date: Fri, 26 Sep 2025 12:53:27 +0900 Subject: [PATCH v3 1/2] Add support for entry counting in pgstats Stats kinds can set an option call track_counts, that will make pgstats track the number of entries that exist in the shared hashtable. As there is only one code path where a new entry is added, and one code path where entries are removed, the count tracking is rather straight-forward. Reads of these counters are optimistic, and may change across two calls. A use case of this facility is PGSS, where we need to be able to cap the number of entries that would be stored in the shared hashtable, based on its "max" GUC. --- src/include/utils/pgstat_internal.h | 27 +++++++++++++ src/backend/utils/activity/pgstat.c | 6 +++ src/backend/utils/activity/pgstat_shmem.c | 46 +++++++++++++++++------ 3 files changed, 67 insertions(+), 12 deletions(-) diff --git a/src/include/utils/pgstat_internal.h b/src/include/utils/pgstat_internal.h index bf75ebcef31c..c07efa1c8ebb 100644 --- a/src/include/utils/pgstat_internal.h +++ b/src/include/utils/pgstat_internal.h @@ -231,6 +231,12 @@ typedef struct PgStat_KindInfo /* Should stats be written to the on-disk stats file? */ bool write_to_file:1; + /* + * Should the number of entries be tracked? For variable-numbered stats, + * to update its PgStat_ShmemControl.entry_counts. + */ + bool track_entry_count:1; + /* * The size of an entry in the shared stats hash table (pointed to by * PgStatShared_HashEntry->body). For fixed-numbered statistics, this is @@ -500,6 +506,16 @@ typedef struct PgStat_ShmemControl */ pg_atomic_uint64 gc_request_count; + /* + * Counters for the number of entries associated to a single stats kind + * that uses variable-numbered objects stored in the shared hash table. + * These counters can be enabled on a per-kind basis, when + * track_entry_count is set. This counter is incremented each time a + * new entry is created (not reused) in the shared hash table, and is + * decremented each time an entry is dropped from the shared hash table. + */ + pg_atomic_uint64 entry_counts[PGSTAT_KIND_MAX]; + /* * Stats data for fixed-numbered objects. */ @@ -925,6 +941,17 @@ pgstat_get_entry_data(PgStat_Kind kind, PgStatShared_Common *entry) return ((char *) (entry)) + off; } +/* + * Returns the number of entries counted for a stats kind. + */ +static inline uint64 +pgstat_get_entry_count(PgStat_Kind kind) +{ + Assert(pgstat_get_kind_info(kind)->track_entry_count); + + return pg_atomic_read_u64(&pgStatLocal.shmem->entry_counts[kind - 1]); +} + /* * Returns a pointer to the shared memory area of custom stats for * fixed-numbered statistics. diff --git a/src/backend/utils/activity/pgstat.c b/src/backend/utils/activity/pgstat.c index 73c2ced3f4e2..5aee8a4ce1c8 100644 --- a/src/backend/utils/activity/pgstat.c +++ b/src/backend/utils/activity/pgstat.c @@ -1487,6 +1487,12 @@ pgstat_register_kind(PgStat_Kind kind, const PgStat_KindInfo *kind_info) ereport(ERROR, (errmsg("custom cumulative statistics property is invalid"), errhint("Custom cumulative statistics require a shared memory size for fixed-numbered objects."))); + if (kind_info->track_entry_count) + { + ereport(ERROR, + (errmsg("custom cumulative statistics property is invalid"), + errhint("Custom cumulative statistics cannot use counter tracking for fixed-numbered objects."))); + } } /* diff --git a/src/backend/utils/activity/pgstat_shmem.c b/src/backend/utils/activity/pgstat_shmem.c index ca36fd247f64..6c686d1615ba 100644 --- a/src/backend/utils/activity/pgstat_shmem.c +++ b/src/backend/utils/activity/pgstat_shmem.c @@ -210,27 +210,35 @@ StatsShmemInit(void) pg_atomic_init_u64(&ctl->gc_request_count, 1); - /* initialize fixed-numbered stats */ + /* Do the per-kind initialization */ for (PgStat_Kind kind = PGSTAT_KIND_MIN; kind <= PGSTAT_KIND_MAX; kind++) { const PgStat_KindInfo *kind_info = pgstat_get_kind_info(kind); char *ptr; - if (!kind_info || !kind_info->fixed_amount) + if (!kind_info) continue; - if (pgstat_is_kind_builtin(kind)) - ptr = ((char *) ctl) + kind_info->shared_ctl_off; - else + /* initialize counter tracking */ + if (kind_info->track_entry_count) + pg_atomic_init_u64(&ctl->entry_counts[kind - 1], 0); + + /* initialize fixed-numbered stats */ + if (kind_info->fixed_amount) { - int idx = kind - PGSTAT_KIND_CUSTOM_MIN; + if (pgstat_is_kind_builtin(kind)) + ptr = ((char *) ctl) + kind_info->shared_ctl_off; + else + { + int idx = kind - PGSTAT_KIND_CUSTOM_MIN; - Assert(kind_info->shared_size != 0); - ctl->custom_data[idx] = ShmemAlloc(kind_info->shared_size); - ptr = ctl->custom_data[idx]; + Assert(kind_info->shared_size != 0); + ctl->custom_data[idx] = ShmemAlloc(kind_info->shared_size); + ptr = ctl->custom_data[idx]; + } + + kind_info->init_shmem_cb(ptr); } - - kind_info->init_shmem_cb(ptr); } } else @@ -303,6 +311,7 @@ pgstat_init_entry(PgStat_Kind kind, /* 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 @@ -319,7 +328,7 @@ pgstat_init_entry(PgStat_Kind kind, shhashent->dropped = false; chunk = dsa_allocate_extended(pgStatLocal.dsa, - pgstat_get_kind_info(kind)->shared_size, + kind_info->shared_size, DSA_ALLOC_ZERO | DSA_ALLOC_NO_OOM); if (chunk == InvalidDsaPointer) return NULL; @@ -330,6 +339,10 @@ pgstat_init_entry(PgStat_Kind kind, /* Link the new entry from the hash entry. */ 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; @@ -859,6 +872,7 @@ static void pgstat_free_entry(PgStatShared_HashEntry *shent, dshash_seq_status *hstat) { dsa_pointer pdsa; + PgStat_Kind kind = shent->key.kind; /* * Fetch dsa pointer before deleting entry - that way we can free the @@ -872,6 +886,13 @@ pgstat_free_entry(PgStatShared_HashEntry *shent, dshash_seq_status *hstat) dshash_delete_current(hstat); dsa_free(pgStatLocal.dsa, pdsa); + + /* + * Entry has been dropped with refcount at 0, hence decrement the + * entry counter. + */ + if (pgstat_get_kind_info(kind)->track_entry_count) + pg_atomic_sub_fetch_u64(&pgStatLocal.shmem->entry_counts[kind - 1], 1); } /* @@ -908,6 +929,7 @@ pgstat_drop_entry_internal(PgStatShared_HashEntry *shent, if (pg_atomic_sub_fetch_u32(&shent->refcount, 1) == 0) { pgstat_free_entry(shent, hstat); + return true; } else -- 2.51.0
From e1f2bd22a0c80675179627ebcd3455e53129d9fd Mon Sep 17 00:00:00 2001 From: Michael Paquier <[email protected]> Date: Fri, 26 Sep 2025 12:54:15 +0900 Subject: [PATCH v3 2/2] injection_points: Add entry counting This serves as coverage for the track_counts, as other in-core stats kinds do not need to cap their maximum. --- .../injection_points/injection_points--1.0.sql | 10 ++++++++++ src/test/modules/injection_points/injection_stats.c | 12 ++++++++++++ src/test/modules/injection_points/t/001_stats.pl | 12 ++++++++++++ 3 files changed, 34 insertions(+) diff --git a/src/test/modules/injection_points/injection_points--1.0.sql b/src/test/modules/injection_points/injection_points--1.0.sql index 5f5657b2043c..a7b61fbdfe6f 100644 --- a/src/test/modules/injection_points/injection_points--1.0.sql +++ b/src/test/modules/injection_points/injection_points--1.0.sql @@ -99,6 +99,16 @@ RETURNS bigint AS 'MODULE_PATHNAME', 'injection_points_stats_numcalls' LANGUAGE C STRICT; +-- +-- injection_points_stats_count() +-- +-- Return the number of entries stored in the pgstats hash table. +-- +CREATE FUNCTION injection_points_stats_count() +RETURNS bigint +AS 'MODULE_PATHNAME', 'injection_points_stats_count' +LANGUAGE C STRICT; + -- -- injection_points_stats_drop() -- diff --git a/src/test/modules/injection_points/injection_stats.c b/src/test/modules/injection_points/injection_stats.c index ca8df4ad217a..b47a3f367fff 100644 --- a/src/test/modules/injection_points/injection_stats.c +++ b/src/test/modules/injection_points/injection_stats.c @@ -49,6 +49,7 @@ static const PgStat_KindInfo injection_stats = { .shared_data_len = sizeof(((PgStatShared_InjectionPoint *) 0)->stats), .pending_size = sizeof(PgStat_StatInjEntry), .flush_pending_cb = injection_stats_flush_cb, + .track_entry_count = true, }; /* @@ -196,6 +197,17 @@ injection_points_stats_numcalls(PG_FUNCTION_ARGS) PG_RETURN_INT64(entry->numcalls); } +/* + * SQL function returning the number of entries allocated for injection + * points in the shared hashtable of pgstats. + */ +PG_FUNCTION_INFO_V1(injection_points_stats_count); +Datum +injection_points_stats_count(PG_FUNCTION_ARGS) +{ + PG_RETURN_INT64(pgstat_get_entry_count(PGSTAT_KIND_INJECTION)); +} + /* Only used by injection_points_stats_drop() */ static bool match_inj_entries(PgStatShared_HashEntry *entry, Datum match_data) diff --git a/src/test/modules/injection_points/t/001_stats.pl b/src/test/modules/injection_points/t/001_stats.pl index 25de5fc46fe4..47ab58d0e9b8 100644 --- a/src/test/modules/injection_points/t/001_stats.pl +++ b/src/test/modules/injection_points/t/001_stats.pl @@ -36,6 +36,9 @@ $node->safe_psql('postgres', "SELECT injection_points_run('stats-notice');"); my $numcalls = $node->safe_psql('postgres', "SELECT injection_points_stats_numcalls('stats-notice');"); is($numcalls, '2', 'number of stats calls'); +my $entrycount = + $node->safe_psql('postgres', "SELECT injection_points_stats_count();"); +is($entrycount, '1', 'number of entries'); my $fixedstats = $node->safe_psql('postgres', "SELECT * FROM injection_points_stats_fixed();"); is($fixedstats, '1|0|2|0|0', 'fixed stats after some calls'); @@ -55,6 +58,9 @@ $node->restart; $numcalls = $node->safe_psql('postgres', "SELECT injection_points_stats_numcalls('stats-notice');"); is($numcalls, '3', 'number of stats after clean restart'); +$entrycount = + $node->safe_psql('postgres', "SELECT injection_points_stats_count();"); +is($entrycount, '1', 'number of entries after clean restart'); $fixedstats = $node->safe_psql('postgres', "SELECT * FROM injection_points_stats_fixed();"); is($fixedstats, '1|0|2|1|1', 'fixed stats after clean restart'); @@ -65,6 +71,9 @@ $node->start; $numcalls = $node->safe_psql('postgres', "SELECT injection_points_stats_numcalls('stats-notice');"); is($numcalls, '', 'number of stats after crash'); +$entrycount = + $node->safe_psql('postgres', "SELECT injection_points_stats_count();"); +is($entrycount, '0', 'number of entries after crash'); $fixedstats = $node->safe_psql('postgres', "SELECT * FROM injection_points_stats_fixed();"); is($fixedstats, '0|0|0|0|0', 'fixed stats after crash'); @@ -81,6 +90,9 @@ $node->safe_psql('postgres', "SELECT injection_points_stats_drop();"); $numcalls = $node->safe_psql('postgres', "SELECT injection_points_stats_numcalls('stats-notice');"); is($numcalls, '', 'no stats after drop via SQL function'); +$entrycount = + $node->safe_psql('postgres', "SELECT injection_points_stats_count();"); +is($entrycount, '0', 'number of entries after drop via SQL function'); # Stop the server, disable the module, then restart. The server # should be able to come up. -- 2.51.0
signature.asc
Description: PGP signature
