Feedback-ID: i0fe9450f:Fastmail
Date: Mon, 10 Feb 2025 10:25:05 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Sami Imseih <samimseih@gmail.com>
Cc: Lukas Fittl <lukas@fittl.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: <Z6lVcWdNQtgz9BGI@paquier.xyz>
References: <CAP53PkwuFbo3NkwZgxwNRMjMfqPEqidD-SggaoQ4ijotBVLJAA@mail.gmail.com>
 <Z5cDQJM9nnoXuStO@paquier.xyz>
 <Z5xTb5iBHVGns35R@paquier.xyz>
 <CAP53Pkw1QJHH9UDjkArS=XdAxtK7XWMpZLGHnVMDmhRTp_HYYw@mail.gmail.com>
 <Z5xgmgyV-wnzMpkO@paquier.xyz>
 <CAA5RZ0vM9AsEqvKued2drKZJ1opt3wbYaDbxGzi-khkNzwn7og@mail.gmail.com>
 <Z6Ks3ta5089DXiPc@paquier.xyz>
 <CAA5RZ0sfRbd1xcq_oHNA0TPr57yM8qkg-GoD6A0nUfyxZhs33Q@mail.gmail.com>
 <CAP53PkxocbNr+eRag3FEJp3-7S1U80FspOg8UQjO902TWMG=6A@mail.gmail.com>
 <CAA5RZ0u6yJdFL=p5vdpbZFS-2YY+Z6vtzmt4gejgZa3RcNiWMQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="axjwv/aSxA6msmsa"
Content-Disposition: inline
In-Reply-To: <CAA5RZ0u6yJdFL=p5vdpbZFS-2YY+Z6vtzmt4gejgZa3RcNiWMQ@mail.gmail.com>
Archived-At: <https://www.postgresql.org/message-id/Z6lVcWdNQtgz9BGI%40paquier.xyz>
Precedence: bulk


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

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.
>=20
> 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

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

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmepVXEACgkQnvQgOdby
QH1Fng/8Du7zQmop7NpT7n3jcnVOQedI8UXaBagHlwI7h8UBuxqix8QZHIzJbr2b
2vdfTelftdAWYd0s/WjCJpxHkwnG6bRCbH+xsIdODLyeyLoR4Yaov8ChwoSjYzLF
SsrKnfRAU1mcC5k+1JbnCw+w20kCaT134ua6w/+3i5VLH0b8Wn34QJu8tYlIYenq
2LurPxHwBvmdwdx63uY1AGjlUlA4rEJkQWeRJrPeJHC+biwpQRy0Qom67cx4ShCp
16ylNnLuqbtBcIbMysgQ19PfXmwVDgKgAEiiuJXNVxLIouFdRJp4GPvKo5V78O2q
0wGJO8l8NaSR+IF8CvB9HLZo7z5II1HlsWUPHX9hM99SzUZP6HGX4Da2AiIBRNsw
xozWNHh2MWVJYDALl7x74kddh/tzHOfyw+8t4EJjc3gVYtnfVJ30YmH2jC85x7ds
mSCqJ8ij3t5PkH490K2PL3U8+tI9+2XZR2G/wZNYqNsS0LLMsteW8E48q64SLaj+
WDZ9rUmWM/bE/O5oJS4dHYN9kFdqQh4ozt6SuEoDK+Iz7QVacQXz40hUoyYZSrc5
xGZ+bQYQ3JMIrWg0i5CMo/5ShvlajfHyCiHlhygAmFrmwO6gySXKy+GHkzf3F3nJ
vbt/hu6wouXhPgi/w07HdhZODn2fdwJzqBAc8ASSsEJmqAJOXOY=
=q6K9
-----END PGP SIGNATURE-----

--axjwv/aSxA6msmsa--