Feedback-ID: i0fe9450f:Fastmail
Date: Mon, 27 Jan 2025 12:53:36 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Lukas Fittl <lukas@fittl.com>
Cc: Sami Imseih <samimseih@gmail.com>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>,
	Marko M <marko@pganalyze.com>
Subject: Re: [PATCH] Optionally record Plan IDs to track plan changes for a
 query
Message-ID: <Z5cDQJM9nnoXuStO@paquier.xyz>
References: <CAP53Pkyow59ajFMHGpmb1BK9WHDypaWtUsS_5DoYUEfsa_Hktg@mail.gmail.com>
 <Z5A__Yuy23aXqUad@paquier.xyz>
 <CAA5RZ0sUPPOpkRZD=Za83op2ngcPC7dp249vcHA-X5YS7p3n8Q@mail.gmail.com>
 <CAP53PkwuFbo3NkwZgxwNRMjMfqPEqidD-SggaoQ4ijotBVLJAA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="qbYvi8k4MAxxhY8+"
Content-Disposition: inline
In-Reply-To: <CAP53PkwuFbo3NkwZgxwNRMjMfqPEqidD-SggaoQ4ijotBVLJAA@mail.gmail.com>
Archived-At: <https://www.postgresql.org/message-id/Z5cDQJM9nnoXuStO%40paquier.xyz>
Precedence: bulk


--qbYvi8k4MAxxhY8+
Content-Type: multipart/mixed; boundary="/MfvyD7Cq4Lo0QAQ"
Content-Disposition: inline


--/MfvyD7Cq4Lo0QAQ
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

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

--/MfvyD7Cq4Lo0QAQ
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v3-0002-Add-pgstat_drop_matching_entries.patch"
Content-Transfer-Encoding: quoted-printable

=46rom 48412fcfb708eb990136321686681e961626aabe Mon Sep 17 00:00:00 2001
=46rom: Lukas Fittl <lukas@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 *e=
ntry_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_Has=
hEntry *, 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;
 }
=20
+/*
+ * 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 *, Da=
tum),
+							 Datum match_data)
 {
 	dshash_seq_status hstat;
 	PgStatShared_HashEntry *ps;
 	uint64		not_freed_count =3D 0;
=20
+	/* entries are removed, take an exclusive lock */
 	dshash_seq_init(&hstat, pgStatLocal.shared_hash, true);
 	while ((ps =3D dshash_seq_next(&hstat)) !=3D NULL)
 	{
 		if (ps->dropped)
 			continue;
=20
+		if (do_drop !=3D NULL && !do_drop(ps, match_data))
+			continue;
+
+		/* delete local reference */
+		if (pgStatEntryRefHash)
+		{
+			PgStat_EntryRefHashEntry *lohashent =3D
+				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();
 }
=20
+/*
+ * 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;
=20
+--
+-- 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)
=20
 	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 =3D=3D 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/mo=
dules/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 =3D $node->safe_psql('postgres',
 	"SELECT * FROM injection_points_stats_fixed();");
 is($fixedstats, '0|0|0|0|0', 'fixed stats after crash');
=20
+# 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 =3D $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 =3D $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;
--=20
2.47.2


--/MfvyD7Cq4Lo0QAQ--

--qbYvi8k4MAxxhY8+
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmeXA0AACgkQnvQgOdby
QH3iLA//Q2/FZzaMLo//bTYkUAx34/+FMogEBh8yL+4BoC0GtMxRf9bl9eQc6DWA
9O5Y6/7KzJudL5sAtA1XZA/Lp0UQW4GODcb59KteqNWfPhJAeJkNDOVmsjHmmoju
tBt8g/6Ugj5h5/7eFzV1iZe3wRskDA5+KDFvTVPkau344qftw1VCIM28IiUMVZIp
eBMChrYOOwW/LWfjIj1JBluazL1YK+lyxf05OOpoW+sn+8VinrugvxML/jSWn+J3
17J4azkw2PmD8iYHkx+3hpjHlrKcGYJKFKL/uDq3eL/XQVUe+tkxnu2d6iMSKapJ
oDgfkMgnqEDUDFh1bEc80rL0BrZ0eclpcCbLQcN6gCoBA32B/ai4VD92p7DlByWM
BLs+ogln7KCqINu5n0nhUs//co6mlzNuI/t2BYaRTMIM+9F/ZTXoULjgGoV52RXj
WcojugTaIc16LvrYyfuFSvnuNjN6vBSMJj7VhLf808lVJXdcMFcf+cX/z+bgLBuh
HhjErzB3WNgYBZ/durVjObqdYkdNLezbwiCvijnQ1qDEzsfdBTsULmzDXdGAHWNc
zl1fayy6Z1cSwjg9pegx4xF1X4+TkSjeEym3f3Snwc/j2LJOR4hjQphx5uxT9Cx2
RpA+99TJYRUKrvQob39cKeqUE/RPPE3T+n18RLDoeaf0n+06Ypc=
=WpJb
-----END PGP SIGNATURE-----

--qbYvi8k4MAxxhY8+--