| From: | Sami Imseih <samimseih(dot)pg(at)gmail(dot)com> |
|---|---|
| To: | Michael Paquier <michael(at)paquier(dot)xyz> |
| Cc: | pgsql-hackers <pgsql-hackers(at)postgresql(dot)org> |
| Subject: | Re: pgstat: allow a stats kind to use its own dedicated dsa/dshash |
| Date: | 2026-10-07 21:55:21 |
| Message-ID: | CAN12+Y+VJ7B_GBr5dADJG+mCUE7=zWXe3J8y90iOR_Lr4JGhiw@mail.gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
> 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
| Attachment | Content-Type | Size |
|---|---|---|
| v4-0002-pgstat-expose-own_hash-in-pg_stat_kind_info.patch | application/octet-stream | 5.8 KB |
| v4-0001-pgstat-allow-a-stats-kind-to-use-its-own-dedicate.patch | application/octet-stream | 57.0 KB |
| From | Date | Subject | |
|---|---|---|---|
| Next Message | surya poondla | 2026-10-07 22:00:54 | Re: Wrong LSN in WAL decoding error messages |
| Previous Message | Zsolt Parragi | 2026-10-07 21:44:35 | Re: [PROPOSAL] Expand OR clauses in joins to UNION ALL paths |