| From: | Bertrand Drouvot <bertranddrouvot(dot)pg(at)gmail(dot)com> |
|---|---|
| To: | Sami Imseih <samimseih(dot)pg(at)gmail(dot)com> |
| Cc: | Postgres hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
| Subject: | Re: [PATCH] pgstat: Add helper to drop entries from a stats kind's hash |
| Date: | 2026-10-28 13:03:09 |
| Message-ID: | auHyja6zbCQxtmA4@bdtpg |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
Hi,
On Tue, Oct 27, 2026 at 04:18:36PM -0500, Sami Imseih wrote:
> Hi,
>
> 44539786b82 allowed a variable-numbered stats kind to use its own
> dshash. One remaining gap is that pgstat_drop_matching_entries() still
> scans all variable-numbered stats hash tables, even when the caller
> already knows which stats kind it wants.
Yeah, the approach makes sense to me.
> Because pgStatLocal.kind_hash[kind] may be the shared variable stats
> hash, the helper filters by kind before using the match callback. A nice
> side effect is that callers of the new function do not need to repeat the kind
> check in their match callbacks.
Agreed. I think filtering before the callback is actually an important behavior.
A couple of comments:
>
> The test_custom_stats module is adjusted so its existing named drop path
> uses the new function with a small match callback.
=== 1
+ if (filter_kind != PGSTAT_KIND_INVALID &&
+ ps->key.kind != filter_kind)
+ continue;
It looks like the test does not ensure this filtering happens before calling
do_drop(). Indeed, removing the above block still makes all 32 subtests pass,
as the test callback only compares objid.
I wonder if test_custom_stats_var_match_entry() could report an error when it
receives an unexpected kind? That would make this test deterministic, provided
another kind has an entry in the shared hash.
=== 2
- /* Drop entry and request GC if the entry could not be freed */
- if (!pgstat_drop_entry(PGSTAT_KIND_TEST_CUSTOM_VAR_STATS, InvalidOid,
- PGSTAT_CUSTOM_VAR_STATS_IDX(stat_name), false))
- pgstat_request_entry_refs_gc();
+ pgstat_drop_matching_entries_of_kind(PGSTAT_KIND_TEST_CUSTOM_VAR_STATS,
+ test_custom_stats_var_match_entry,
+ UInt64GetDatum(stat_idx));
AFAICS, this removes the only coverage of pgstat_drop_entry() with an own_hash
kind.
I wonder if it wouldn't make sense to keep the existing path for one entry and
use the new helper for the other one?
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Greg Burd | 2026-10-28 13:29:39 | Re: WAL record CRC calculated incorrectly because of underlying buffer modification |
| Previous Message | Bertrand Drouvot | 2026-10-28 12:34:24 | Re: Report buffer access strategies in EXPLAIN |