On Mon, Oct 05, 2026 at 04:13:37PM -0500, Sami Imseih wrote:
> I kept own_hash tied to a dedicated DSA for now. I am not sure we would want to
> support that mix and match behavior. A stats kind that asks for its own hash is
> already asking to be isolated from the shared stats table, and using a dedicated
> DSA keeps the hash storage and entry storage isolated in the same way.
>
> I have not thought of a good use case for splitting those controls, but perhaps
> there is one.
>
> Attached is v2
+ dsa_area *kind_dsa[PGSTAT_KIND_MAX + 1];
dshash_table *shared_hash;
+ dshash_table *kind_hash[PGSTAT_KIND_MAX + 1];
[...]
+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;
+}
[...]
+ /* Attach dedicated DSAs and hash tables */
+ for (PgStat_Kind kind = PGSTAT_KIND_MIN; kind <= PGSTAT_KIND_MAX; kind++)
[...]
+ else
+ {
+ pgStatLocal.kind_dsa[kind] = NULL;
+ pgStatLocal.kind_hash[kind] = NULL;
+ }
Having both shared_hash and kind_dsa/kind_hash feels duplicated. Hmm.
How about enforcing a policy so as kind_dsa/kind_hash are always set
for variable-sized stats kinds? If kind_hash_valid is true, point
these to the main shared hashtable instead of using NULL. That would
make pgstat_get_hash_for_kind() unnecessary. And then perhaps remove
the shared hashtable pointer all together from the main structure?
It looks like the decision-making could just be driven by
kind_hash_valid, for the most parts.
pgstat_get_dsa_for_kind() is a nice wrapper to keep, as that means
less dshash_get_dsa_area() footprint.
+ /* All hash tables to iterate (shared + per-kind), built at attach time */
+ dshash_table *all_hashes[PGSTAT_KIND_MAX + 1];
+ int num_hashes;
It looks like you are right to have this optimization with an
additional array. That speeds the flush of the stats by not having to
go through all PGSTAT_KIND_MAX entries, which I suspect could become
costly with very short queries a-la "SELECT 1".
/* dshash entry is not modified, take shared lock */
- dshash_seq_init(&hstat, pgStatLocal.shared_hash, false);
- while ((p = dshash_seq_next(&hstat)) != NULL)
This code has been moved to pgstat_reset_matching_entries_in_hash(),
and the corresponding comment is gone. That's important to keep
documented.
/* 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);
Similar remark here. The exclusive lock part applies to
dshash_seq_init().
--
Michael
