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


--mu6spM7cVlRq37L/
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Thu, Jan 02, 2025 at 12:46:04PM -0800, Lukas Fittl wrote:
> Inspired by a prior proposal by Sami Imseih for tracking Plan IDs [0], as
> well as extensions like pg_stat_plans [1] (unmaintained), pg_store_plans
> [2] (not usable on production, see notes later) and aurora_stat_plans [3]
> (enabled by default on AWS), this proposed patch set adds:

0002 introduces this new routine to delete all the entries of the new
stats kind you are adding:
+void
+pgstat_drop_entries_of_kind(PgStat_Kind kind)
+{
+	dshash_seq_status hstat;
+	PgStatShared_HashEntry *ps;
+	uint64		not_freed_count =3D 0;
+
+	dshash_seq_init(&hstat, pgStatLocal.shared_hash, true);

This is the same as pgstat_drop_all_entries(), except for the filter
based on the stats kind and the fact that you need to take care of the
local reference for an entry of this kind, if there are any, like
pgstat_drop_entry().  Why not, that can be useful on its own depending
on the stats you are working on.  May I suggest the addition of a code
path outside of your main proposal to test this API?  For example
injection_stats.c with a new SQL function to reset everything.

+static void
+pgstat_gc_plan_memory()
+{
+	dshash_seq_status hstat;
+	PgStatShared_HashEntry *p;
+
+	/* dshash entry is not modified, take shared lock */
+	dshash_seq_init(&hstat, pgStatLocal.shared_hash, false);
+	while ((p =3D dshash_seq_next(&hstat)) !=3D NULL)
+	{
+		PgStatShared_Common *header;
+		PgStat_StatPlanEntry *statent;

Question time: pgstat_drop_entries_of_kind() is called once in 0004,
which does a second sequential scan of pgStatLocal.shared_hash.
That's not efficient, making me question what's the reason to think
why pgstat_drop_entries_of_kind() is the best approach to use.  I like=20
the idea of pgstat_drop_entries_of_kind(), less how it's applied in
the context of the main patch.

Mixed feelings about the choices of JumblePlanNode() in 0003 based on
its complexity as implemented.  When it comes to such things, we
should keep the custom node functions short, applying node_attr
instead to the elements of the nodes so as the assumptions behind the
jumbling are documented within the structure definitions in the
headers, not the jumbling code itself.
--
Michael

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

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmeQP/0ACgkQnvQgOdby
QH0Xzw/+Ng6bqeemJ90PzZLefazs7BkhgmwXCZzSPu/1Ty9GPqnk0Gxfe4TdRlZ8
gYGeRqFXcRTN6uQk/gJUYOlARMCsErLfaI3nxqYNyPfc3RU+833OOrKrABN4w+do
yje6FNupFHkl/DRb1vLJbm3gIJD854GkyKqgF9hPpCKwzf63pk3rZ3zeVRXBhDzr
oN0NG/+E7Yasl9jIa0Vmtvi4kRFLA8kV/qhz7K+C9jJtt6h4TCdUqYB7POXdGjsH
PSCryImCyt1Wqo3QbiP/aHTSZoQ5e3XMBZkZEQYJYqpcUKMZQMDd4+B4LZYNOI+d
ijF1fsHoLYIj2WNZdJrZsxMCy8Z3lkFyk8D6v90oNV6Id2KvfgtdTC+ZhR2IuyUD
y9NHcdkAf/Mre38vDNJQu547kgrfN1LmtnY6S8lAqi+KbNj1jvAZOaEMaG28V+eS
q86t5hW52VaO+PP7AdB69FcudyLA7vDoV/mki9KtaSGLyALE0HOBTFqw+5vwrAZH
QLTCZPyqTXtenAzo6yT+U8jeiwvPQqVhLn3adx9G0S8fTrQ7K1fblhAFzj/Z5y6I
dUGMbAS+jyBRbYiinKIe13pLT3L1cYvHt/1i5w7ziTAdH8y1zw/ktiZJwHBIIEZd
Yi0fscW7HxXaKacqbWK+8LMAmlVlaT3FEHjFi8CHdo7XIkikY/Q=
=myPW
-----END PGP SIGNATURE-----

--mu6spM7cVlRq37L/--