> On Tue, Sep 23, 2025 at 01:39:00PM -0500, Sami Imseih wrote:
> > The refcount reaches 0 when all backends release their references to the
> > stat, so if something like pg_stat_statements relies on a count for
> > deallocation purposes (to stay within the max), and that value only
> > decrements when all references are released, then it could end up
> > constantly triggering deallocation attempts. So, I am wondering if we
> > actually need another value that tracks "live" entries, or those that
> > have not yet been marked for drop. This means the live entries count
> > is decremented as soon as the entry is set to ->dropped.
>
> Couldn't that mean a potential bloat in terms of memory in the dshash
> if we have a large cycle of objects still held around but marked to be
> gone? That sounds risky to me as it could go out of control.
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.
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.
>> Regarding the option name track_counts in PgStat_KindInfo.
>> In my personal opinion, I was just wondering that it shares
>> the same name as the GUC track_counts(pgstat_track_counts in the source
>> code).
>> If we want to make it clearer, renaming it to track_entry_counts
>> could be one option.
> Yes, that's a good point and I have missed the GUC part. What you are
> suggesting is cleaner overall with the flag added to the pgstats kind
> info.
IMO, "entry_counts" does not sound correct. We are not tracking more
than one count. What about track_num_entries ?
--
Sami
index 066027c387c..2482e818e4c 100644
--- a/src/backend/utils/activity/pgstat_shmem.c
+++ b/src/backend/utils/activity/pgstat_shmem.c
@@ -872,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
@@ -885,6 +886,11 @@ pgstat_free_entry(PgStatShared_HashEntry *shent,
dshash_seq_status *hstat)
dshash_delete_current(hstat);
dsa_free(pgStatLocal.dsa, pdsa);
+
+ /* Entry has been dropped, hence decrement the entry counter */
+ if (pgstat_get_kind_info(kind)->track_entry_counts)
+ pg_atomic_sub_fetch_u64(&pgStatLocal.shmem->entry_counts[kind -
1], 1);
+
}
/*
@@ -920,17 +926,8 @@ pgstat_drop_entry_internal(PgStatShared_HashEntry *shent,
/* release refcount marking entry as not dropped */
if (pg_atomic_sub_fetch_u32(&shent->refcount, 1) == 0)
{
- PgStat_Kind kind = shent->key.kind;
-
pgstat_free_entry(shent, hstat);
- /*
- * Entry has been dropped with refcount at 0, hence decrement
the
- * entry counter.
- */
- if (pgstat_get_kind_info(kind)->track_entry_counts)
-
pg_atomic_sub_fetch_u64(&pgStatLocal.shmem->entry_counts[kind - 1], 1);
-
return true;
}
else
