public inbox for [email protected]
help / color / mirror / Atom feedFrom: Michael Paquier <[email protected]>
To: Lukas Fittl <[email protected]>
Cc: Sami Imseih <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Cc: Marko M <[email protected]>
Subject: Re: [PATCH] Optionally record Plan IDs to track plan changes for a query
Date: Mon, 27 Jan 2025 12:53:36 +0900
Message-ID: <[email protected]> (raw)
In-Reply-To: <CAP53PkwuFbo3NkwZgxwNRMjMfqPEqidD-SggaoQ4ijotBVLJAA@mail.gmail.com>
References: <CAP53Pkyow59ajFMHGpmb1BK9WHDypaWtUsS_5DoYUEfsa_Hktg@mail.gmail.com>
<[email protected]>
<CAA5RZ0sUPPOpkRZD=Za83op2ngcPC7dp249vcHA-X5YS7p3n8Q@mail.gmail.com>
<CAP53PkwuFbo3NkwZgxwNRMjMfqPEqidD-SggaoQ4ijotBVLJAA@mail.gmail.com>
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
Attachments:
[text/x-diff] v3-0002-Add-pgstat_drop_matching_entries.patch (5.9K, 2-v3-0002-Add-pgstat_drop_matching_entries.patch)
download | inline diff:
From 48412fcfb708eb990136321686681e961626aabe Mon Sep 17 00:00:00 2001
From: Lukas Fittl <[email protected]>
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
[application/pgp-signature] signature.asc (833B, 3-signature.asc)
download
view thread (34+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: [PATCH] Optionally record Plan IDs to track plan changes for a query
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox