pgstat: allow a stats kind to use its own dedicated dsa/dshash

Started by Sami Imseih2 months ago2 messageshackers

samimseih@gmail.com

2 months ago

Hi,

As I have been working on [1]/messages/by-id/CAA5RZ0vZwR_dSK6fo0P2-EnskUVN0NjLHnGnJMFDPC8-kEW3sQ@mail.gmail.com in which pg_stat_statements can use
a custom cumulative stats kind, I realized it will be beneficial for a
kind to have a dedicated dsa area/dshash in which to store their stats.

A kind like pg_stat_statements will have thousands of entries and
likely will have more entries than all other kinds combined. If combined
in the same dsa/dshash with everything else, sequential scans looking
for pg_stat_statements entries will have to skip over all other kinds,
and vice versa. A dedicated hash also reduces lock contention on
unrelated kinds, particularly for garbage-collection.

There is also a need for custom kinds to independently size their own
dsa areas, so they can manage them carefully. If we want to set limits
on how much space the entries can use for a kind like pg_stat_statements,
we need to be able to set the size directly on its dsa.

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.

Also, 2 new external APIs are introduced:
- pgstat_get_hash_for_kind(PgStat_Kind kind)
- pgstat_get_dsa_for_kind(PgStat_Kind kind)
These allow the extension to retrieve the dshash and dsa pointers for
both iterating through the entries and for setting the size limit on
the dsa. Otherwise, the management of these separate structures is
transparent to the extension and managed by the pgstat infrastructure.

These APIs are necessary because we don't have higher-level abstractions
in pgstats to do things like "iterate through this kind" or "set size
limit for this stats kind"; such abstractions may be good to have to
avoid using dshash_seq_* or dsa_set_size_limit directly, but I am not
sure they are worth it at this point.

Only variable-length stats can opt into own_hash. Fixed-length allocates their
data into PgStat_ShmemControl.

[1]: /messages/by-id/CAA5RZ0vZwR_dSK6fo0P2-EnskUVN0NjLHnGnJMFDPC8-kEW3sQ@mail.gmail.com

--
Sami Imseih
Amazon Web Services (AWS)

Bertrand Drouvot

bertranddrouvot.pg@gmail.com

about 1 month ago

In reply to: Sami Imseih (#1)

Re: pgstat: allow a stats kind to use its own dedicated dsa/dshash

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

pgstat: allow a stats kind to use its own dedicated dsa/dshash

Attachments: