Re: [PATCH] pgstat: Add helper to drop entries from a stats kind's hash

> > > Ok, there maybe a reason for the subset use case. v4 attached
> > > does it that way.
>
> Thanks for the new version!
>
> > Thanks. Bertrand, what do you think?
>
> Having a single pgstat_drop_matching_entries() makes sense and looks cleaner
> than the previous design to me.
>
> Regarding v4, just a few comments:
>
> === 1 (Nit)
>
> + * If kind is PGSTAT_KIND_INVALID, match all kinds;
>
> while:
>
> /* use 0 for INVALID, to catch zero-initialized data */
> #define PGSTAT_KIND_INVALID 0
>
> I wonder if a new PGSTAT_KIND_ALL wouldn't make more sense for the all kinds
> case?

Hmm, yeah. v5 adds PGSTAT_KIND_ALL and uses that for the all-kinds
case.

> === 2
>
> Looking closely at the tests, it looks like we are not testing PGSTAT_KIND_INVALID
> with a non NULL callback (means scanning all kinds and dropping only entries matching
> the callback).
>
> This is not due to this patch and looks like a past oversight: the old injection_points
> test was testing pgstat_drop_matching_entries() with a non NULL callback, but
> d52c24b0f80 removed that test and 31280d96a64 did not restore this case in
> test_custom_stats.
>
> Worth adding it back while at it?

Adding an all-kinds scan with a non-NULL callback did cross my mind,
but I did not want to overdo the tests. I also did not realize that this
used to be covered by the injection_points test, so thanks for the
research.

v5 now has these pgstat_drop_matching_entries() cases:

1. Specific kind + non-NULL callback
2. All kinds + non-NULL callback (similar to the injection points test
functionally)
3. Specific kind + NULL callback

--
Sami