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

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

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

On Wed, Oct 28, 2026 at 01:35:47PM -0500, Sami Imseih wrote:

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().

My first reaction after reading this patch is why do we need two APIs?
We could have pgstat_drop_matching_entries() take an additional
PgStat_Kind, matching with all the hash tables if passing an invalid
kind? That would mean one routine instead of three as done in this
patch, as of two exposed and one static in pgstat_shmem.c. That would
also avoid the filter_kind check in the inner loop of
pgstat_drop_matching_entries_in_hash(), as done in the patch.
--
Michael

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().

My first reaction after reading this patch is why do we need two APIs?
We could have pgstat_drop_matching_entries() take an additional
PgStat_Kind, matching with all the hash tables if passing an invalid
kind?

I initially avoided changing the C API/ABI, which is why the previous
version added a separate helper. Since this is only targeting master, I
agree that changing pgstat_drop_matching_entries() is better. If there
are extensions using pgstat_drop_matching_entries(), it's a simple
change.

That would mean one routine instead of three as done in this
patch, as of two exposed and one static in pgstat_shmem.c. That would
also avoid the filter_kind check in the inner loop of
pgstat_drop_matching_entries_in_hash(), as done in the patch.

I kept the kind check in the loop because, when kind maps to the shared
hash, scanning that hash also visits entries for other kinds. Without
the check, a kind-specific scan could consider those entries too,
including in the NULL do_drop path.

The added tests cover both cases: the callback path verifies
that entries of another kind are not passed to the callback, and the NULL
do_drop path verifies that dropping all entries of the custom stats kind
does not drop a relation stats entry in the same shared hash.

--
Sami Imseih
Amazon Web Services (AWS)

On Thu, Oct 29, 2026 at 11:30:45AM -0500, Sami Imseih wrote:

I kept the kind check in the loop because, when kind maps to the shared
hash, scanning that hash also visits entries for other kinds. Without
the check, a kind-specific scan could consider those entries too,
including in the NULL do_drop path.

Okay. Looks globally fine.

The added tests cover both cases: the callback path verifies
that entries of another kind are not passed to the callback, and the NULL
do_drop path verifies that dropping all entries of the custom stats kind
does not drop a relation stats entry in the same shared hash.

-    for (int h = 0; h < pgStatLocal.num_var_hashes; h++)
+    if (filter_by_kind)
     {
+        Assert(kind >= PGSTAT_KIND_MIN && kind <= PGSTAT_KIND_MAX);
+        Assert(pgStatLocal.kind_hash[kind] != NULL);
+    }
+
+    for (int h = 0; h < num_hashes; h++)
+    {
+        dshash_table *hash = filter_by_kind ?
+            pgStatLocal.kind_hash[kind] : pgStatLocal.var_hashes[h];

Instead of allocating the dshash_table pointer in the loop, perhaps
use temporary variables to store the number of hashtables and an array
of them. That would matter if somebody would like to extend this API
to a subset of N hashtables, where 1 < N < num_var_hashes?
--
Michael

Instead of allocating the dshash_table pointer in the loop, perhaps
use temporary variables to store the number of hashtables and an array
of them. That would matter if somebody would like to extend this API
to a subset of N hashtables, where 1 < N < num_var_hashes?

Ok, there maybe a reason for the subset use case. v4 attached
does it that way.

--
Sami

On Thu, Oct 29, 2026 at 07:05:14PM -0500, Sami Imseih wrote:

Ok, there maybe a reason for the subset use case. v4 attached
does it that way.

Thanks. Bertrand, what do you think?
--
Michael

Hi,

On Fri, Oct 30, 2026 at 10:19:15AM +0900, Michael Paquier wrote:

On Thu, Oct 29, 2026 at 07:05:14PM -0500, Sami Imseih wrote:

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?

=== 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?

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

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

On Fri, Oct 30, 2026 at 09:41:12AM -0500, Sami Imseih wrote:

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

PGSTAT_KIND_ALL holds the same meaning as PGSTAT_KIND_INVALID on an
interface-basis. I'd just rely on PGSTAT_KIND_INVALID, drop
PGSTAT_KIND_ALL; that seems simpler overall.

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

The NULL callback case is already covered by the in-core code, for
example where we drop all the entries during a reset. I don't see a
specific need for more coverage with this pattern.

-static bool
-match_inj_entries(PgStatShared_HashEntry *entry, Datum match_data)
-{
- return entry->key.kind == PGSTAT_KIND_INJECTION;
-}
[...]
-injection_points_stats_drop(PG_FUNCTION_ARGS)
-{
- pgstat_drop_matching_entries(match_inj_entries, 0);
-
- PG_RETURN_VOID();

Hmmm... Indeed... We used to have a pgstat_drop_matching_entries()
call that we have dropped when moving these tests from
injection_points to test_custom_stats. That was not intentional.

I think that your test_custom_stats_var_drop_all() makes sense, with
a callback that matches with the variable-sized kind ID. This should
be backpatched down to v19, perhaps, but I'm mild with playing with
this stable branch as we are close to release. And this is just for a
small gap in the tests.

Anyway, what do you think about the attached to address this hole in
the tests? I'll continue my review of the main patch once this issue
is fixed.
--
Michael

Hi,

On Mon, Nov 02, 2026 at 09:35:50AM +0900, Michael Paquier wrote:

On Fri, Oct 30, 2026 at 09:41:12AM -0500, Sami Imseih wrote:

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

The NULL callback case is already covered by the in-core code, for
example where we drop all the entries during a reset. I don't see a
specific need for more coverage with this pattern.

Agreed.

Hmmm... Indeed... We used to have a pgstat_drop_matching_entries()
call that we have dropped when moving these tests from
injection_points to test_custom_stats. That was not intentional.

Yeah, looks like an oversight.

I think that your test_custom_stats_var_drop_all() makes sense, with
a callback that matches with the variable-sized kind ID. This should
be backpatched down to v19, perhaps, but I'm mild with playing with
this stable branch as we are close to release. And this is just for a
small gap in the tests.

Anyway, what do you think about the attached to address this hole in
the tests?

LGTM and I don't see a strong case for backpatching.

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

Anyway, what do you think about the attached to address this hole in
the tests?

LGTM and I don't see a strong case for backpatching.

The test looks good to me as well. I don't think it should be backpatched.

v5-0001 now includes the previously lost test.
v5-0002 is the same as v4, but without including the v5-0001 test and
using PGSTAT_KIND_INVALID instead of a new PGSTAT_KIND_ALL.

--
Sami