Feedback-ID: i0fe9450f:Fastmail
Date: Fri, 27 Dec 2024 09:23:12 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Postgres hackers <pgsql-hackers@lists.postgresql.org>
Cc: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Subject: Fix handling of injection_points regarding pending stats
Message-ID: <Z23zcE4w1djukkva@paquier.xyz>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="XVDjTINYhr5PlByi"
Content-Disposition: inline
Archived-At: <https://www.postgresql.org/message-id/Z23zcE4w1djukkva%40paquier.xyz>
Precedence: bulk


--XVDjTINYhr5PlByi
Content-Type: multipart/mixed; boundary="Ei5FCfEnZZLhcy4e"
Content-Disposition: inline


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

Hi all,

While doing more stuff related to pgstats and WAL, I have bumped on
the fact that the module injection_points uses for its
variable-numbered stats a pending flush callback but it does not
handle its entries so as these would be handled locally first, with a
final flush timed by pgstat_report_stat() that would itself call the
pending flush callback.  This is a less efficient practice, because it
requires to touch the pgstats dshash more often than necessary.

This is caused by the use of pgstat_get_entry_ref_locked() rather than
pgstat_prep_pending_entry() which would make sure to time the flush of
any pending entries appropriately depending on pgstat_report_stat().
I'd like to fix the module so as the flush timing is controlled like
all the other stats kinds, which is a better and more efficient
practice to have anyway as this code should stand as a template for
custom pgstats kinds.

While doing so, I have also noticed that injection_stats_flush_cb()
missed a pgstat_unlock_entry() when entries get locked.

All that is addressed in the patch attached.  Feel free if you have
any comments.

Thanks,
--
Michael

--Ei5FCfEnZZLhcy4e
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="0001-injection_points-Tweak-variable-numbered-stats-to-wo.patch"
Content-Transfer-Encoding: quoted-printable

=46rom d0aae1f873ec4f0cde91d851d131767e7ca66171 Mon Sep 17 00:00:00 2001
=46rom: Michael Paquier <michael@paquier.xyz>
Date: Fri, 27 Dec 2024 09:16:02 +0900
Subject: [PATCH] injection_points: Tweak variable-numbered stats to work wi=
th
 pending data

As coded, the module was not using pending entries to store its data
locally before a flush to the central dshash is done with a timed
pgstat_report_stat() call.  Hence, the flush callback was defined, but
finished by being not used.  As a template, this is more efficient than
the original logic of updating directly the shared memory entries as
this reduces the interactions that need to be done with the pgstats
hash table in shared memory.

injection_stats_flush_cb() was also missing a pgstat_unlock_entry(), so
add one, while on it.
---
 .../modules/injection_points/injection_stats.c    | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/src/test/modules/injection_points/injection_stats.c b/src/test=
/modules/injection_points/injection_stats.c
index e16b9db284..21d5c10f39 100644
--- a/src/test/modules/injection_points/injection_stats.c
+++ b/src/test/modules/injection_points/injection_stats.c
@@ -80,6 +80,9 @@ injection_stats_flush_cb(PgStat_EntryRef *entry_ref, bool=
 nowait)
 		return false;
=20
 	shfuncent->stats.numcalls +=3D localent->numcalls;
+
+	pgstat_unlock_entry(entry_ref);
+
 	return true;
 }
=20
@@ -127,13 +130,13 @@ pgstat_create_inj(const char *name)
 	if (!inj_stats_loaded || !inj_stats_enabled)
 		return;
=20
-	entry_ref =3D pgstat_get_entry_ref_locked(PGSTAT_KIND_INJECTION, InvalidO=
id,
-											PGSTAT_INJ_IDX(name), false);
+	entry_ref =3D pgstat_prep_pending_entry(PGSTAT_KIND_INJECTION, InvalidOid,
+										  PGSTAT_INJ_IDX(name), NULL);
+
 	shstatent =3D (PgStatShared_InjectionPoint *) entry_ref->shared_stats;
=20
 	/* initialize shared memory data */
 	memset(&shstatent->stats, 0, sizeof(shstatent->stats));
-	pgstat_unlock_entry(entry_ref);
 }
=20
 /*
@@ -168,16 +171,14 @@ pgstat_report_inj(const char *name)
 	if (!inj_stats_loaded || !inj_stats_enabled)
 		return;
=20
-	entry_ref =3D pgstat_get_entry_ref_locked(PGSTAT_KIND_INJECTION, InvalidO=
id,
-											PGSTAT_INJ_IDX(name), false);
+	entry_ref =3D pgstat_prep_pending_entry(PGSTAT_KIND_INJECTION, InvalidOid,
+										  PGSTAT_INJ_IDX(name), NULL);
=20
 	shstatent =3D (PgStatShared_InjectionPoint *) entry_ref->shared_stats;
 	statent =3D &shstatent->stats;
=20
 	/* Update the injection point statistics */
 	statent->numcalls++;
-
-	pgstat_unlock_entry(entry_ref);
 }
=20
 /*
--=20
2.45.2


--Ei5FCfEnZZLhcy4e--

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

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmdt83AACgkQnvQgOdby
QH0NdA//e5+XD34x67enTP9oEb362a3TpDhg5+lhm7MYnoGSQX4LIDicAQejr4kA
o+vHAyon6LgNLagPsseZafSXSN2QE2SZEll0mII+S5ZiKLMs5YB2obAXfDBDPTwJ
Hppni00P3oJieTi5uf2Jjw1jempTIrHkkycYATnvNZzTPR49aPMwHyxlmnvgqBXT
1YD958Zrccg1QD3N860pgnodS7HKeV7tYJ0jPt5fJTl2+xavOoqNdNAsVUkeQzMn
36UNYe6CkR/iKbzFm9k7hXRAs0meeCIRXgYjfg52YUHnmKQ+ZdmoQvnScmJgQYJ6
cNEeQAgLEtDDLKwFmxGEb6gL9412aq/nxFoOw3a7LMeXY8n7I4sp/xV/CZDfb8uG
FmUXZmNo8vc2HEIDQfQHyT231RCfOkqUOLRh/k2RAx4KSOrFLoRIvjrhnRa3BMtg
itUMwPHitwOgxL1MN5HbzGwGJCh87xh86tmNc4YpEB3HOVmsbRmTdoe7iY+sNG+/
D2p6dxqDSMwPzshp8n4Fk4AnKtDPdNknmIYjyiUaCI0ny8P4JXsA3rCxwihLn7he
zHysEQHqxajRlzvFDSSPe6+TlOGFo3Q25HqSrvtwlDPAMuExBtnaHTzILdccquHC
b6wdjUMfjAC4iz/7Bf7GkieK0Xodm/LXlTKxnI9Mqz/+9APyMio=
=2AaJ
-----END PGP SIGNATURE-----

--XVDjTINYhr5PlByi--