Re: [PATCH] Optionally record Plan IDs to track plan changes for a query

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 = 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 = dshash_seq_next(&hstat)) != 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
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