Hi,
On Wed, Jul 15, 2026 at 12:16:26PM -0500, Sami Imseih wrote:
> The attached proposal implements a new stats kind option, own_hash, which
> when set to true allocates an independent dsa/dshash for that stats kind.
Thanks for the patch!
The dedicated hash idea makes sense to me.
A few comments:
=== 1
- shhashent = dshash_find_or_insert(pgStatLocal.shared_hash, &key, &shfound);
+ shhashent = dshash_find_or_insert(hash, &key, &shfound);
With the patch, an extension can set a size limit on its dedicated DSA, making
an allocation failure much easier to reach. Such a failure in
dshash_find_or_insert()
would raise ERROR, leaving the backend local cache entry with a NULL
shared_entry.
A later pgstat_gc_entry_refs() call could then dereference the NULL
entry_ref->shared_entry pointer.
I wonder if this should use dshash_find_or_insert_extended(...,
DSHASH_INSERT_NO_OOM)
and release the local entry if it returns NULL?
=== 2
@@ -2080,12 +2090,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);
If the dedicated DSA cannot accommodate the persisted entries,
dshash_find_or_insert()
raises ERROR and startup can fail.
I wonder if this case should discard the statistics that no longer fit rather
than prevent the server from starting?
=== 3
+ /* 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);
I wonder if it wouldn't make more sense for own_hash to create a dedicated
dshash
in the existing pgstat DSA, with a separate option for a dedicated DSA?
That would retain independent iteration and hash partition locks while avoiding
an additional shared memory allocation and per backend DSA state when
independent
memory accounting is not needed.
=== 4
+ * 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];
+
Current master provides dshash_get_dsa_area() since 762e329e83f, so this one
looks now redundant.
=== 5
pgstat_reset_matching_entries(bool (*do_reset) (PgStatShared_HashEntry *,
Datum),
@@ -1173,26 +1282,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)
+ {
pgstat_reset_entries_of_kind() uses this routine, so resetting one kind still
scans every hash, including unrelated dedicated hashes.
Could pgstat_reset_entries_of_kind() scan only the hash returned by
pgstat_get_hash_for_kind(kind)? The kind filter would still be needed when
that returns the shared hash.
=== 6
- /* 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;
+
This restores the behavior removed by 5045d9ff3b5. The SQL functions remain
callable with the kind unregistered.
I think the branch should be removed so pgstat_register_kind() reports the
intended prerequisite error.
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
