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

On Thu, Feb 06, 2025 at 07:52:53PM -0600, Sami Imseih wrote:
> This fixes the long comments in plannodes.h to make it easier to add the
> attribute annotation. It made the most sense to make this the first patch
> in the set.

A commit that happened last Friday made also this to have conflict.

> Done. Also rewrote the header comment in jumblefuncs.c to describe
> a more generic node jumbling mechanism that this file now offers.
>
> Yes, after getting my hands on this, I agree with you. It made more sense
> to keep all the jumbling work in jumblefuncs.c

-static void AppendJumble(JumbleState *jstate,
-                         const unsigned char *item, Size size

I don't understand why there is a need for publishing AppendJumble()
while it remains statis in jumblefuncs.c.  This is not needed in 0003
and 0004, either.

Should we use more generic names for the existing custom_query_jumble,
no_query_jumble, query_jumble_ignore and query_jumble_location?  Last
time I've talked about that with Peter E, "jumble" felt too generic,
so perhaps we're looking for a completely new term?  This impacts as
well the naming of the existing queryjumblefuncs.c.  The simplest term
that may be possible here is "hash", actually, because that's what we
are doing with all these node structures?  That's also a very generic
term. The concept of location does not apply to plans, based on the
current proposal, so perhaps we should talk about "query normalization
location"?

Point is that query_jumble_ignore is used in the planner nodes, which
feels inconsistent, so perhaps we could rename query_jumble_ignore and
no_query_jumble to "hash_ignore" and/or "no_hash", or something like
that?  This may point towards the need of a split, not sure, still the
picture is incomplete.

> v5-0003 and v5-0004 introduce the planId in core and pg_stat_plans. These
> needed rebasing only; but I have not yet looked at this thoroughly.
> 
> We should aim to get 0001 and 0002 committed next.

Yeah.  I didn't see any reasons why 0001 should not happen now, as it
makes the whole easier while making the header styles a bit more
consistent.  Perhaps also if somebody forks the code and adds some
pg_node_attr() properties?

v5-0003 and v5-0004, not sure yet.  The intrisincs of the planner make
putting a strict definition of what a hash means hard to set down,
we should work towards studying that more first.  I don't see this
happen until the next release freeze.
--
Michael