Feedback-ID: i0fe9450f:Fastmail
Date: Thu, 26 Oct 2023 09:08:42 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Dmitry Dolgov <9erthalion6@gmail.com>
Cc: Nathan Bossart <nathandbossart@gmail.com>,
	"Gregory Stark (as CFM)" <stark.cfm@gmail.com>,
	David Geier <geidav.pg@gmail.com>, Sergei Kornilov <sk@zsrv.org>,
	Alvaro Herrera <alvherre@alvh.no-ip.org>,
	Marcos Pegoraro <marcos@f10.com.br>,
	vignesh C <vignesh21@gmail.com>,
	Robert Haas <robertmhaas@gmail.com>, Zhihong Yu <zyu@yugabyte.com>,
	David Steele <david@pgmasters.net>,
	PostgreSQL-development <pgsql-hackers@postgresql.org>,
	Pavel Trukhanov <pavel.trukhanov@gmail.com>,
	Tom Lane <tgl@sss.pgh.pa.us>
Subject: Re: pg_stat_statements and "IN" conditions
Message-ID: <ZTmuCtymIS3n3fP_@paquier.xyz>
References: <8702fa3e-5aac-dcf0-161b-faa2b9eadad6@gmail.com>
 <20230226104619.jfmwrpd5zrkmp3eu@erthalion.local>
 <CAM-w4HO29FpoX5oYR7tuxP9sZA1s3zLV94c_79h_uQeNHUjhyQ@mail.gmail.com>
 <20230314190432.l4oe3wa3pvt5b7hx@erthalion.local>
 <20230319122734.xkba536kut3g4gcy@erthalion.local>
 <20230704044611.GA3373000@nathanxps13>
 <20230704190256.arszjcwvrjekxj25@ddolgov.remote.csb>
 <ZSj6pOwUCM-Mnh__@paquier.xyz>
 <20231013133519.yf26oncodrvemfjw@ddolgov.remote.csb>
 <20231017081541.octf6qghtjllwtvj@erthalion.local>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="R4+ZdJWw003cTinA"
Content-Disposition: inline
In-Reply-To: <20231017081541.octf6qghtjllwtvj@erthalion.local>
Archived-At: <https://www.postgresql.org/message-id/ZTmuCtymIS3n3fP_%40paquier.xyz>
Precedence: bulk


--R4+ZdJWw003cTinA
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Tue, Oct 17, 2023 at 10:15:41AM +0200, Dmitry Dolgov wrote:
> In the current patch version I didn't add anything yet to address the
> question of having more parameters to tune constants merging. The main
> obstacle as I see it is that the information for that has to be
> collected when jumbling various query nodes. Anything except information
> about the ArrayExpr itself would have to be acquired when jumbling some
> other parts of the query, not directly related to the ArrayExpr. It
> seems to me this interdependency between otherwise unrelated nodes
> outweigh the value it brings, and would require some more elaborate (and
> more invasive for the purpose of this patch) mechanism to implement.

 typedef struct ArrayExpr
 {
+	pg_node_attr(custom_query_jumble)
+

Hmm.  I am not sure that this is the best approach
implementation-wise.  Wouldn't it be better to invent a new
pg_node_attr (these can include parameters as well!), say
query_jumble_merge or query_jumble_agg_location that aggregates all
the parameters of a list to be considered as a single element.  To put
it short, we could also apply the same property to other parts of a
parsed tree, and not only an ArrayExpr's list.

 /* GUC parameters */
 extern PGDLLIMPORT int compute_query_id;
-
+extern PGDLLIMPORT bool query_id_const_merge;

Not much a fan of this addition as well for an in-core GUC.  I would
suggest pushing the GUC layer to pg_stat_statements, maintaining the
computation method to use as a field of JumbleState as I suspect that
this is something we should not enforce system-wide, but at
extension-level instead.

+	/*
+	 * Indicates the constant represents the beginning or the end of a merged
+	 * constants interval.
+	 */
+	bool		merged;

Not sure that this is the best thing to do either.  Instead of this
extra boolean flag, could it be simpler if we switch LocationLen so as
we track the start position and the end position of a constant in a
query string, so as we'd use one LocationLen for a whole set of Const
nodes in an ArrayExpr?  Perhaps this could just be a refactoring piece
of its own?

+	/*
+	 * If the first expression is a constant, verify if the following elements
+	 * are constants as well. If yes, the list is eligible for merging, and the
+	 * order of magnitude need to be calculated.
+	 */
+	if (IsA(firstExpr, Const))
+	{
+		foreach(temp, elements)
+			if (!IsA(lfirst(temp), Const))
+				return false;

This path should be benchmarked, IMO.
--
Michael

--R4+ZdJWw003cTinA
Content-Type: application/pgp-signature; name="signature.asc"

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmU5rgoACgkQnvQgOdby
QH02+RAApCDE10kpHcgog5D1e7YKI/+u477UK4mFgWvh1GlxA3QZgYefC5XKiNlk
LhSW/cXgAMFhSU2UrlygGnzSFOHmh+6k9KftQmgAfq9bxNYTNh7JqMTtsWefR7J7
PXNuPuZ7beac27IDfcIomJJc4ZcGqnbS+jYraRyvJBPBXdJit8pmahpo6G5z4mej
tGMRlWEpi8m0I+sZZZYVmuIemNKR/Mk5ekQuSURSQIi1SzZQDzT3ydmx3QHpS9tz
dnH5FYKUYUnKfVrVG0ETVaWJrv1naQDZOK2/fhzSP32fj9Nk8ZtCzvbXqexOPHXW
kdUrjFlgrQTTPndKZoBsww7NwcEx3yGf53PCoTSGZCUAqI7gtaNAKfyXQZNRfSHP
Lk2eXLjrdFgg6Eq1DFgD7FN2IYBUVqFsmMYCnN9OS0bQ55RW2ExkxcjJ0ej0Ank4
iosZb1sMj3wjh+aUv76VRhW/tgQUvVnsz2pa2c9IzragMPE32YcHqhK4nzzEF1uC
fCltDNNUwd40mfzhNPRFKK2T539LJrZ5A7CDghJCuM5CDpXAozXQkbISHJwgLU1l
h8aGwWxDuv8BgYzTyNYgEA3BA0W+QdS5wTyikjcEmHj8v7zwnGwzHjOxXosshyGI
tuye5KbBblgYehCLCLo0Rfnp5S7T5RFwwaNzSGrWY93NRIoFiGA=
=QF6K
-----END PGP SIGNATURE-----

--R4+ZdJWw003cTinA--