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

> 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