Feedback-ID: i0fe9450f:Fastmail
Date: Wed, 24 Dec 2025 13:06:59 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Sami Imseih <samimseih@gmail.com>
Cc: zengman <zengman@halodbtech.com>,
	pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: Refactor query normalization into core query jumbling
Message-ID: <aUtm47pcAhZ1Ix0B@paquier.xyz>
References: 
 <CAA5RZ0tZp5qU0ikZEEqJnxvdSNGh1DWv80sb-k4QAUmiMoOp_Q@mail.gmail.com>
 <aUT1eblRYpwB1nC7@ip-10-97-1-34.eu-west-3.compute.internal>
 <CAA5RZ0vQfE14HyfpoPXDRThVcdCkLgY_HGz+J2qLB9soNUE9QQ@mail.gmail.com>
 <tencent_2FF1F5A9360E2DD24C6478FB@qq.com>
 <CAA5RZ0uLS9RrpO2roX7p3EHE4-VJkBsGAB970jrbo1-GRDAi0g@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="I7SKfRjmYIa+CS4q"
Content-Disposition: inline
In-Reply-To: 
 <CAA5RZ0uLS9RrpO2roX7p3EHE4-VJkBsGAB970jrbo1-GRDAi0g@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/aUtm47pcAhZ1Ix0B%40paquier.xyz>
Precedence: bulk


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

On Tue, Dec 23, 2025 at 10:35:18AM -0600, Sami Imseih wrote:
> > Though this may be tangential to the current topic, I have long been wa=
nting to revise the two instances of `Assert(len_to_wrt >=3D 0);`
> > in the code to the implementation below. Would you kindly advise if thi=
s modification is worthwhile?
> >
> > ```
> >         if (len_to_wrt > 0)
> >         {
> >                 memcpy(norm_query + n_quer_loc, query + quer_loc, len_t=
o_wrt);
> >                 n_quer_loc +=3D len_to_wrt;
> >         }
> > ```
>=20
> I am not sure I like this idea. The len_to_wrt being less than 0
> indicates a bug which will be hidden behind such a loop. I
> think it's better to keep the Assert as-is.

This set of asserts are critical to keep.  An incorrect computation is
a synonym of an out-of-bound write in this case, which would classify
any problem as something that could be worth a CVE.

> v3 implements this approach without a callback. This establishes a clear
> boundary: core owns JumbleState modifications, extensions consume the
> results through the API.
>=20
> I kept the post_parse_analyze hook signature unchanged since
> GenerateNormalizedQuery still needs to modify JumbleState
> (to populate constant lengths). Making it const would be misleading.
>=20
> For the second part of making more jumbling utilities global, this can
> be taken up in another thread. I am now thinking we would be better
> off actually taking all the generic jumbling routines and separating
> them into their own C file. So we can have a new file like primjumble.c
> which will have all the "primitive" jumbling utilities, and queryjumblefu=
ncs.c
> can worry about its core business of jumbling a Query tree. Anyhow,
> these are just some high level thoughts on this part for now.

Hmm.  Moving from pgss to the core backup the code that updates the
clocations of an existing JumbleState does not solve the issue that
we should not modify the internals of the JumbleState computed.  So
this does not move the ball at all.

The updates of clocations depend on GenerateNormalizedQuery(), which
depends on the pre-work done in CleanQuerytext() to get a query string
and its location for a multi-query string case.  If we really want to
make a JumbleState not touchable at all, we could either push more
work of CleanQuerytext() and GenerateNormalizedQuery() into core.

Or  a second thing that we can do, which would be actually much
simpler, is to rework entirely fill_in_constant_lengths() so as we
generate a *copy* of the location data PGSS wants rather than force it
into a JumbleState that would be pushed across more stacks of the
post-parse-analyze hook.  Doing that would allow us to pull the plug
into making JumbleState and the original copies of clocations a set of
consts when given to extensions.
--
Michael

--I7SKfRjmYIa+CS4q
Content-Type: application/pgp-signature; name="signature.asc"

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmlLZuMACgkQnvQgOdby
QH3wlQ/9HU1sDMED+ydsyKOWVgTF//ebMhN2maqrTZ6VQfCr/JavfhnDd8JrUq+O
xcu4yFluuf1dyxhYvfJlhfVOHr0DgdrBAi9lKypAvfjQvchcAx4++p35XwNocszo
wJ5+TbY3Z1gEEDJ9CQIUCqdOzCBIW61fNtDYQVF6GNYW2evsf3Gdp9zoXI9l9pDb
dLQoJSaJHhnYK5STvEiuvZT0hLOUBjhWjTU/oJsKF6f2AM4o5c+wHJB2B1cIQFZL
/YW+jNi70jD/XJxmiEZui2Gl6amT+pM4ikbiWhHbfQAX6UpbVFAkxFfFEcLcUVd6
T1D+ZOmrPzalsuYJb2WIJmrPOtofHoz9fb+UWJwr9W3K5iZ8nafTBoUyoFF+g6+/
7gPVtu+E2RpQ9KgCD3XfNnt5QcDuj4Skg3ALSv6PReXdyTdcqtf9efQDFrW4uPtf
ww1+3L5/9n5iT6OPGRl3YTTCgh9lCK9Op2qP7rOhIfCUKKRJ+1XLrru63DSO5y+2
6trRNNwGWaBhSMMo6bsLuBcn0Hwnc7XexcY3OSXZrdokEMZnMv0vvTmt4F5Gqe2X
qhW9yLxKpJ0UWLYbjeIYV8I1Q+vMLoV5UdJZntbV2RCRu4lmF+GvjFM55+qmjZNj
SJLTPL9MOVHWce4Dj17jajYWzF0i6sN6Z8EK7SMVsTrDxLEAKeY=
=hGlH
-----END PGP SIGNATURE-----

--I7SKfRjmYIa+CS4q--