> > > 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
v5-0001-pgstat-Add-kind-filter-to-pgstat_drop_matching_en.patch
Description: Binary data
