On Fri, Jan 24, 2025 at 01:59:00AM -0800, Lukas Fittl wrote: > Overall, I also do wonder if it wouldn't be better to have a callback > mechanism in the shared memory stats, so stats plugins can do extra work > when an entry gets dropped (like freeing the DSA memory for the plan text), > vs having to add all this extra logic to do it.
Not sure about this part yet. I have looked at 0002 to begin with something and it is really useful on its own. Stats kinds calling this routine don't need to worry about the internals of dropping local references or doing a seqscan on the shared hash table. However, what you have sent lacks in flexibility to me, and the duplication with pgstat_drop_all_entries is annoying. This had better be merged in a single routine. Attached is an updated version that adds an optional "do_drop" callback in the function that does the seqscan on the dshash, to decide if an entry should be gone or not. This follows the same model as the "reset" part, where stats kind can push the matching function they want to work on the individual entries. We could add a pgstat_drop_entries_of_kind(), but I'm not feeling that this is strongly necessary with the basic interface in place. The changes in the module injection_points were not good. The SQL function was named "reset" but that's a drop operation. What do you think? -- Michael
From 48412fcfb708eb990136321686681e961626aabe Mon Sep 17 00:00:00 2001
From: Lukas Fittl <lu...@fittl.com>
Date: Thu, 2 Jan 2025 10:46:30 -0800
Subject: [PATCH v3] Add pgstat_drop_matching_entries()
This allows users of the cumulative statistics systems to drop all
entries based on the decisions of a matching function, similar to how
pgstat_reset_matching_entries() works.
---
src/include/utils/pgstat_internal.h | 2 ++
src/backend/utils/activity/pgstat_shmem.c | 31 ++++++++++++++++++-
.../injection_points--1.0.sql | 10 ++++++
.../injection_points/injection_stats.c | 19 ++++++++++++
.../modules/injection_points/t/001_stats.pl | 13 ++++++++
5 files changed, 74 insertions(+), 1 deletion(-)
diff --git a/src/include/utils/pgstat_internal.h b/src/include/utils/pgstat_internal.h
index a3d39d2b725..06dcea3f0dc 100644
--- a/src/include/utils/pgstat_internal.h
+++ b/src/include/utils/pgstat_internal.h
@@ -718,6 +718,8 @@ extern bool pgstat_lock_entry_shared(PgStat_EntryRef *entry_ref, bool nowait);
extern void pgstat_unlock_entry(PgStat_EntryRef *entry_ref);
extern bool pgstat_drop_entry(PgStat_Kind kind, Oid dboid, uint64 objid);
extern void pgstat_drop_all_entries(void);
+extern void pgstat_drop_matching_entries(bool (*do_drop) (PgStatShared_HashEntry *, Datum),
+ Datum match_data);
extern PgStat_EntryRef *pgstat_get_entry_ref_locked(PgStat_Kind kind, Oid dboid, uint64 objid,
bool nowait);
extern void pgstat_reset_entry(PgStat_Kind kind, Oid dboid, uint64 objid, TimestampTz ts);
diff --git a/src/backend/utils/activity/pgstat_shmem.c b/src/backend/utils/activity/pgstat_shmem.c
index 342586397d6..770d62425c5 100644
--- a/src/backend/utils/activity/pgstat_shmem.c
+++ b/src/backend/utils/activity/pgstat_shmem.c
@@ -993,19 +993,39 @@ pgstat_drop_entry(PgStat_Kind kind, Oid dboid, uint64 objid)
return freed;
}
+/*
+ * Scan through the shared hashtable of stats, dropping statistics if
+ * approved by the optional do_drop() function.
+ */
void
-pgstat_drop_all_entries(void)
+pgstat_drop_matching_entries(bool (*do_drop) (PgStatShared_HashEntry *, Datum),
+ Datum match_data)
{
dshash_seq_status hstat;
PgStatShared_HashEntry *ps;
uint64 not_freed_count = 0;
+ /* entries are removed, take an exclusive lock */
dshash_seq_init(&hstat, pgStatLocal.shared_hash, true);
while ((ps = dshash_seq_next(&hstat)) != NULL)
{
if (ps->dropped)
continue;
+ if (do_drop != NULL && !do_drop(ps, match_data))
+ continue;
+
+ /* delete local reference */
+ if (pgStatEntryRefHash)
+ {
+ PgStat_EntryRefHashEntry *lohashent =
+ pgstat_entry_ref_hash_lookup(pgStatEntryRefHash, ps->key);
+
+ if (lohashent)
+ pgstat_release_entry_ref(lohashent->key, lohashent->entry_ref,
+ true);
+ }
+
if (!pgstat_drop_entry_internal(ps, &hstat))
not_freed_count++;
}
@@ -1015,6 +1035,15 @@ pgstat_drop_all_entries(void)
pgstat_request_entry_refs_gc();
}
+/*
+ * Scan through the shared hashtable of stats and drop all entries.
+ */
+void
+pgstat_drop_all_entries(void)
+{
+ pgstat_drop_matching_entries(NULL, 0);
+}
+
static void
shared_stat_reset_contents(PgStat_Kind kind, PgStatShared_Common *header,
TimestampTz ts)
diff --git a/src/test/modules/injection_points/injection_points--1.0.sql b/src/test/modules/injection_points/injection_points--1.0.sql
index c445bf64e62..5d83f08811b 100644
--- a/src/test/modules/injection_points/injection_points--1.0.sql
+++ b/src/test/modules/injection_points/injection_points--1.0.sql
@@ -85,6 +85,16 @@ RETURNS bigint
AS 'MODULE_PATHNAME', 'injection_points_stats_numcalls'
LANGUAGE C STRICT;
+--
+-- injection_points_stats_drop()
+--
+-- Drop all statistics of injection points.
+--
+CREATE FUNCTION injection_points_stats_drop()
+RETURNS void
+AS 'MODULE_PATHNAME', 'injection_points_stats_drop'
+LANGUAGE C STRICT;
+
--
-- injection_points_stats_fixed()
--
diff --git a/src/test/modules/injection_points/injection_stats.c b/src/test/modules/injection_points/injection_stats.c
index 5db62bca66f..14903c629e0 100644
--- a/src/test/modules/injection_points/injection_stats.c
+++ b/src/test/modules/injection_points/injection_stats.c
@@ -197,3 +197,22 @@ injection_points_stats_numcalls(PG_FUNCTION_ARGS)
PG_RETURN_INT64(entry->numcalls);
}
+
+/* Only used by injection_points_stats_drop() */
+static bool
+match_inj_entries(PgStatShared_HashEntry *entry, Datum match_data)
+{
+ return entry->key.kind == PGSTAT_KIND_INJECTION;
+}
+
+/*
+ * SQL function that drops all injection point statistics.
+ */
+PG_FUNCTION_INFO_V1(injection_points_stats_drop);
+Datum
+injection_points_stats_drop(PG_FUNCTION_ARGS)
+{
+ pgstat_drop_matching_entries(match_inj_entries, 0);
+
+ PG_RETURN_VOID();
+}
diff --git a/src/test/modules/injection_points/t/001_stats.pl b/src/test/modules/injection_points/t/001_stats.pl
index d4539fe8727..25de5fc46fe 100644
--- a/src/test/modules/injection_points/t/001_stats.pl
+++ b/src/test/modules/injection_points/t/001_stats.pl
@@ -69,6 +69,19 @@ $fixedstats = $node->safe_psql('postgres',
"SELECT * FROM injection_points_stats_fixed();");
is($fixedstats, '0|0|0|0|0', 'fixed stats after crash');
+# On drop all stats are gone
+$node->safe_psql('postgres',
+ "SELECT injection_points_attach('stats-notice', 'notice');");
+$node->safe_psql('postgres', "SELECT injection_points_run('stats-notice');");
+$node->safe_psql('postgres', "SELECT injection_points_run('stats-notice');");
+$numcalls = $node->safe_psql('postgres',
+ "SELECT injection_points_stats_numcalls('stats-notice');");
+is($numcalls, '2', 'number of stats calls');
+$node->safe_psql('postgres', "SELECT injection_points_stats_drop();");
+$numcalls = $node->safe_psql('postgres',
+ "SELECT injection_points_stats_numcalls('stats-notice');");
+is($numcalls, '', 'no stats after drop via SQL function');
+
# Stop the server, disable the module, then restart. The server
# should be able to come up.
$node->stop;
--
2.47.2
signature.asc
Description: PGP signature
