> 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?
Done in v4. pgStatLocal.dsa is replaced by pgStatLocal.kind_dsa[],
so pgStatLocal.kind_hash[kind] and pgStatLocal.kind_dsa[kind] can be
used directly instead of passing a dsa_area around.
> Or perhaps introduce an inline pgstat_get_dsaarea_for_kind() with an
> Assert(), something that v2 did not do either.
I used the array lookup directly instead. This also addresses your later
point about the value of an assert-only helper, and keeps the DSA lookup
consistent with the hash lookup after removing pgstat_get_hash_for_kind().
> - 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?
Removed in v4. This is not a failure mode we should expect on this
path, so the fallback was just defensive noise.
> + 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.
Removed in v4.
> 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.
Done in v4. I replaced the boolean GUC with
test_custom_var_stats.kind, so the test selects a custom stats kind by
name. The TAP test now uses a Perl array with the two names and expected
own_hash values. More cases can be added to the array later while still
sharing the same custom kind ID.
> Perhaps use a generate_series() to reduce the number of queries and
> the number of duplicated lines?
Done.
> No need for a catversion bump in patches posted for reviews.
Removed the catversion bump from v4.
> The change for pg_stat_get_kind_info could be split into its own
> patch.
Done. v4 is split into two patches.
--
Sami
