> Yeah, the approach makes sense to me. Thanks for the review!
> > 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. Fair. I added a check in the test callback to error if it receives an unexpected kind. The shared hash test path now also creates a relation stats entry before calling pgstat_drop_matching_entries_of_kind(), so the test would fail if pgstat_drop_matching_entries_in_hash() invokes the callback before filtering by kind. I also check that the relation stats entry still reports n_tup_ins = 1 after the custom stats drop, so the test covers both not invoking the callback for other kinds and not accidentally dropping those other entries. I also added a comment above the kind filter in pgstat_drop_matching_entries_in_hash() to explain that other kinds can appear there when the requested kind maps to the shared variable stats hash. Dedicated hashes contain only entries of the requested kind. I added a short comment in pgstat_internal.h for pgstat_drop_matching_entries_of_kind(), explaining what makes it different from pgstat_drop_matching_entries(). > === 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. Yeah, I mentioned that earlier, and I did not feel right about it either. So I added back the test that uses pgstat_drop_entry(), and added a separate test function for the new _of_kind API. The TAP test now drops one entry via pgstat_drop_entry() and another via pgstat_drop_matching_entries_of_kind(). I also renamed the new static helper to pgstat_drop_matching_entries_in_hash() to match the existing pgstat_reset_matching_entries_in_hash(). -- Sami
v2-0001-pgstat-Add-helper-to-drop-entries-from-a-stats-ki.patch
Description: Binary data
