On Tue, Oct 06, 2026 at 11:20:16AM -0500, Sami Imseih wrote: > Agreed. I restored both comments in v3. > > Other changes in v3:
+ hash = pgstat_get_hash_for_kind(key.kind);
+ dsa = dshash_get_dsa_area(hash);
[...]
- chunk = pgstat_alloc_entry_body(key.kind);
+ chunk = pgstat_alloc_entry_body(key.kind, dsa);
There is a lot of back-and-forth with dshash_get_dsa_area() as a
matter to replace pgStatLocal.dsa. Hmm, dshash_get_dsa_area() only
retrieves a dshash_table->area, but wouldn't it be simpler to replace
the single pgStatLocal.dsa with an area based on the kind ID? Passing
around a dsa_area in pgstat_alloc_entry_body() and pgstat_init_entry()
feels unproductive if we should know the area based on the kind ID.
Knowing the dsa_area matters for the dsa_free() in
pgstat_get_entry_ref(), sure, but I'd probably just use an array that
holds all the references to limit the footprint and use of
dshash_get_dsa_area(). So that would mean to replace all the
pgStatLocal.dsa by an array-based lookup with the kind ID. Or
perhaps introduce an inline pgstat_get_dsaarea_for_kind() with an
Assert(), something that v2 did not do either.
- pgstat_get_kind_info(shent->key.kind)->name,
+ kind_info ? kind_info->name : "unknown",
Ugh. Is that something worth a backpatch? Why does this new failure
mode matter?
+ if (kind < PGSTAT_KIND_MIN || kind > PGSTAT_KIND_MAX ||
+ pgStatLocal.kind_hash[kind] == NULL)
In pgstat_drop_entry(), it seems to me that this is a no-op, as in a
should-never-happen scenario.
+static inline dshash_table *
+pgstat_get_hash_for_kind(PgStat_Kind kind)
+{
+ Assert(pgStatLocal.kind_hash[kind] != NULL);
+
+ return pgStatLocal.kind_hash[kind];
+}
Hmm, okay by me if you want to keep this one, but honestly I am biased
about how useful it is. That's here really only for the assert() to
catch programming mistakes, and we would crash shortly anyway after
calling pgstat_get_hash_for_kind().
+foreach my $use_own_hash (qw(true false))
[...]
+ pgstat_register_kind(PGSTAT_KIND_TEST_CUSTOM_VAR_STATS,
+ test_custom_stats_use_own_hash ?
+ &custom_stats_own_hash : &custom_stats_shared_hash);
Rather than a boolean given to the input function and a GUC, just
specify a string with a name. I'd imagine a point where a third
variable-sized custom kind gets introduced. Perhaps also assign a
different name to the new custom_stats_own_hash? Relying on one kind
ID for this module is a good idea. That's less bits eaten in the
available range.
+ $node->safe_psql('postgres',
+ q(select test_custom_stats_var_update('entry1')));
+ $node->safe_psql('postgres',
+ q(select test_custom_stats_var_update('entry1')));
+ $node->safe_psql('postgres',
Perhaps use a generate_series() to reduce the number of queries and
the number of duplicated lines?
No need for a catversion bump in patches posted for reviews. These
would conflict in the CI or when applied on HEAD. It conflicted
already on HEAD as of e9c03f0f1bf6.
The change for pg_stat_get_kind_info could be split into its own
patch. Minor issue and easy to split, as different code paths are
touched.
--
Michael
signature.asc
Description: PGP signature
