Feedback-ID: i0fe9450f:Fastmail
Date: Wed, 26 Mar 2025 08:10:16 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Sami Imseih <samimseih@gmail.com>
Cc: Christoph Berg <myon@debian.org>, Lukas Fittl <lukas@fittl.com>,
	Tom Lane <tgl@sss.pgh.pa.us>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>,
	ma lz <ma100@hotmail.com>
Subject: Re: query_id: jumble names of temp tables for better
 pg_stat_statement UX
Message-ID: <Z-M32Cm7FM-1gpxB@paquier.xyz>
References: <461405.1742691859@sss.pgh.pa.us>
 <Z9-sUvq8pE5uinEu@paquier.xyz>
 <CAA5RZ0t4XQ_SANiJ5VfuoKnAuxMaY2ggQWoGKkHo+U2H+Sh-Sw@mail.gmail.com>
 <1189112.1742869660@sss.pgh.pa.us>
 <CAA5RZ0uNofEXfEfNw3uRN3D3oXkFPQ_s+huLLHMKR_+MCk8RPQ@mail.gmail.com>
 <Z-IoMKL8I-r-xsuW@paquier.xyz>
 <CAP53PkzAPrHNHquT76mVjfStrz0B2hKhF+CbAZ_w9QJUJ_1oJQ@mail.gmail.com>
 <Z-JU8EmGdlnNEGot@paquier.xyz>
 <Z-LOknecFpjVjhkP@msg.df7cb.de>
 <CAA5RZ0sy8gZzK0qNScY-C_jOpqQb6Y3EQaXHu0Ti9pon-4jJBg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="s8ia3hVjg8lWZQf/"
Content-Disposition: inline
In-Reply-To: <CAA5RZ0sy8gZzK0qNScY-C_jOpqQb6Y3EQaXHu0Ti9pon-4jJBg@mail.gmail.com>
Archived-At: <https://www.postgresql.org/message-id/Z-M32Cm7FM-1gpxB%40paquier.xyz>
Precedence: bulk


--s8ia3hVjg8lWZQf/
Content-Type: multipart/mixed; boundary="rRt3L36MHAlbwzfM"
Content-Disposition: inline


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

On Tue, Mar 25, 2025 at 11:58:21AM -0500, Sami Imseih wrote:
> "Since the queryid hash value is computed on the post-parse-analysis
> representation of the queries, the opposite is also possible: queries with
> identical texts might appear as separate entries, if they have different
> meanings as a result of factors such as different search_path settings."
>=20
> I think this text could remain as is, because search_path still
> matters for things like functions, etc.

Yeah, I think that's OK as-is.  I'm open to more improvements,
including more tests for these function patterns.  It's one of these
areas where we should be able to tweak RangeTblFunction and apply a
custom function to its funcexpr, and please note that I have no idea
how complex it could become as this is a Node expression.  :D

Functions in a temporary schema is not something as common as temp
tables, I guess, so these matter less, but they would still be a cause
of bloat for monitoring in very specific workloads.

> 2/
> "For example, pg_stat_statements will consider two apparently-identical
> queries to be distinct, if they reference a table that was dropped and
> recreated between the executions of the two queries."
>=20
> This is no longer true for relations, but is still true for functions. I =
think
> we should mention the caveats in a bit more detail as this change
> will have impact on the most common case. What about something
> like this?
>=20
> "For example, pg_stat_statements will consider two apparently-identical
> queries to be distinct, if they reference a function that was dropped and
> recreated between the executions of the two queries.

That's a bit larger than functions, but we could remain a bit more
evasive, with "if they reference *for example* a function that was
dropped and recreated between the executions of the two queries".

Note that for DDLs, like CREATE TABLE, we also group entries with
identical relation names, so we are kind of in line with the patch,
not with the current docs.

> Conversely, if a table is dropped and recreated between the
> executions of queries, two apparently-identical queries may be
> considered the same. However, if the alias for a table is different
> for semantically similar queries, these queries will be considered distin=
ct"

This addition sounds like an improvement here.

As this thread has proved, we had little coverage these cases in pgss,
so I've applied the tests as an independent change.  It is also useful
to track how things change in the commit history depending on how the
computation is tweaked.  I've also included your doc suggestions.  I
feel that we could do better here, but that's a common statement
anyway when it comes to the documentation.
--
Michael

--rRt3L36MHAlbwzfM
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v6-0001-Add-custom-query-jumble-function-for-RangeTblEntr.patch"
Content-Transfer-Encoding: quoted-printable

=46rom 8ea61bb0d7d6c4dbbb40dbaedb5e751027163cfe Mon Sep 17 00:00:00 2001
=46rom: Michael Paquier <michael@paquier.xyz>
Date: Wed, 26 Mar 2025 08:07:59 +0900
Subject: [PATCH v6] Add custom query jumble function for RangeTblEntry.eref

---
 src/include/nodes/parsenodes.h                | 11 +++++++---
 src/backend/nodes/queryjumblefuncs.c          | 19 ++++++++++++++++++
 doc/src/sgml/pgstatstatements.sgml            |  9 +++++++--
 .../pg_stat_statements/expected/select.out    | 20 ++++++++-----------
 4 files changed, 42 insertions(+), 17 deletions(-)

diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 23c9e3c5abf2..a87f949b389e 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -1050,8 +1050,13 @@ typedef struct RangeTblEntry
 	 */
 	/* user-written alias clause, if any */
 	Alias	   *alias pg_node_attr(query_jumble_ignore);
-	/* expanded reference names */
-	Alias	   *eref pg_node_attr(query_jumble_ignore);
+
+	/*
+	 * Expanded reference names.  This uses a custom query jumble function so
+	 * as the table name is included in the computation, not its list of
+	 * columns.
+	 */
+	Alias	   *eref pg_node_attr(custom_query_jumble);
=20
 	RTEKind		rtekind;		/* see above */
=20
@@ -1094,7 +1099,7 @@ typedef struct RangeTblEntry
 	 * tables to be invalidated if the underlying table is altered.
 	 */
 	/* OID of the relation */
-	Oid			relid;
+	Oid			relid pg_node_attr(query_jumble_ignore);
 	/* inheritance requested? */
 	bool		inh;
 	/* relation kind (see pg_class.relkind) */
diff --git a/src/backend/nodes/queryjumblefuncs.c b/src/backend/nodes/query=
jumblefuncs.c
index f8b0f91704ba..62d6cfb7ac15 100644
--- a/src/backend/nodes/queryjumblefuncs.c
+++ b/src/backend/nodes/queryjumblefuncs.c
@@ -67,6 +67,9 @@ static void _jumbleElements(JumbleState *jstate, List *el=
ements);
 static void _jumbleA_Const(JumbleState *jstate, Node *node);
 static void _jumbleList(JumbleState *jstate, Node *node);
 static void _jumbleVariableSetStmt(JumbleState *jstate, Node *node);
+static void _jumbleRangeTblEntry_eref(JumbleState *jstate,
+									  RangeTblEntry *rte,
+									  Alias *expr);
=20
 /*
  * Given a possibly multi-statement source string, confine our attention t=
o the
@@ -516,3 +519,19 @@ _jumbleVariableSetStmt(JumbleState *jstate, Node *node)
 	JUMBLE_FIELD(is_local);
 	JUMBLE_LOCATION(location);
 }
+
+/*
+ * Custom query jumble function for RangeTblEntry.eref.
+ */
+static void
+_jumbleRangeTblEntry_eref(JumbleState *jstate,
+						  RangeTblEntry *rte,
+						  Alias *expr)
+{
+	JUMBLE_FIELD(type);
+
+	/*
+	 * This includes only the table name, the list of column names is ignored.
+	 */
+	JUMBLE_STRING(aliasname);
+}
diff --git a/doc/src/sgml/pgstatstatements.sgml b/doc/src/sgml/pgstatstatem=
ents.sgml
index f4e384e95aea..679381f00607 100644
--- a/doc/src/sgml/pgstatstatements.sgml
+++ b/doc/src/sgml/pgstatstatements.sgml
@@ -675,8 +675,13 @@ calls | 2
    things, the internal object identifiers appearing in this representatio=
n.
    This has some counterintuitive implications.  For example,
    <filename>pg_stat_statements</filename> will consider two apparently-id=
entical
-   queries to be distinct, if they reference a table that was dropped
-   and recreated between the executions of the two queries.
+   queries to be distinct, if they reference for example a function that w=
as
+   dropped and recreated between the executions of the two queries.
+   Conversely, if a table is dropped and recreated between the
+   executions of queries, two apparently-identical queries may be
+   considered the same. However, if the alias for a table is different
+   for semantically similar queries, these queries will be considered
+   distinct.
    The hashing process is also sensitive to differences in
    machine architecture and other facets of the platform.
    Furthermore, it is not safe to assume that <structfield>queryid</struct=
field>
diff --git a/contrib/pg_stat_statements/expected/select.out b/contrib/pg_st=
at_statements/expected/select.out
index 708c6b0e9c41..1eebc2898ab9 100644
--- a/contrib/pg_stat_statements/expected/select.out
+++ b/contrib/pg_stat_statements/expected/select.out
@@ -433,11 +433,10 @@ COMMIT;
 SELECT calls, query FROM pg_stat_statements ORDER BY query COLLATE "C";
  calls |                                 query                            =
     =20
 -------+------------------------------------------------------------------=
------
-     1 | SELECT * FROM temp_t
-     1 | SELECT * FROM temp_t
+     2 | SELECT * FROM temp_t
      0 | SELECT calls, query FROM pg_stat_statements ORDER BY query COLLAT=
E "C"
      1 | SELECT pg_stat_statements_reset() IS NOT NULL AS t
-(4 rows)
+(3 rows)
=20
 SELECT pg_stat_statements_reset() IS NOT NULL AS t;
  t=20
@@ -623,18 +622,15 @@ SELECT a AS a1 FROM pgss_schema_2.tab_search_diff_2;
 SELECT calls, query FROM pg_stat_statements ORDER BY query COLLATE "C";
  calls |                                 query                            =
     =20
 -------+------------------------------------------------------------------=
------
-     3 | SELECT a FROM pgss_schema_2.tab_search_diff_2 AS t1
-     9 | SELECT a FROM tab_search_diff_2 AS t1
-     1 | SELECT a, b FROM pgss_schema_1.tab_search_same
-     3 | SELECT a, b FROM tab_search_same
+     8 | SELECT a FROM tab_search_diff_2
+     4 | SELECT a FROM tab_search_diff_2 AS t1
+     4 | SELECT a, b FROM tab_search_same
      0 | SELECT calls, query FROM pg_stat_statements ORDER BY query COLLAT=
E "C"
-     1 | SELECT count(*) FROM pgss_schema_1.tab_search_same
-     1 | SELECT count(*) FROM pgss_schema_2.tab_search_diff_1
-     3 | SELECT count(*) FROM tab_search_diff_1
+     4 | SELECT count(*) FROM tab_search_diff_1
      4 | SELECT count(*) FROM tab_search_diff_2
-     3 | SELECT count(*) FROM tab_search_same
+     4 | SELECT count(*) FROM tab_search_same
      1 | SELECT pg_stat_statements_reset() IS NOT NULL AS t
-(11 rows)
+(8 rows)
=20
 DROP SCHEMA pgss_schema_1 CASCADE;
 NOTICE:  drop cascades to 3 other objects
--=20
2.49.0


--rRt3L36MHAlbwzfM--

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

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmfjN9gACgkQnvQgOdby
QH1WVQ/9EyLUAhS+boIbF+OdqdxB7WliGPUVcR4t2Ltyj0l9I4qP3rzQYax+IASa
anFI/SKdmGchuaJBlrkB2ip/plvVH80HLg/WaV0uO73mI7uRjVpLiZEoycFXcKFq
7wBGkfaf3S05N24aIc4+7CqURfWWhoNh3t+TgvJ5H5y0q6pFkM+D4Ga8lKzwR3Gu
BZafCMOcLYIMhPhfVlW7tA5zPYUQnY0wBwNGA5Uw2PAKBEomOmiK4HWPBpCHTR48
kQ33IKudF03IonaTIv3OoQ25r3fL44Rb59Nh4rtcHTLVV7Vta+9APHePhKDg1hbA
CfOXuNNKKc8RbaYG/JDDsL3rGr6XJ2Lm5ghBwqVYYUDvDb5dOPh3fcBnkn24gj/K
sH8Ug7CHBQaaHD0A4IHwupaC3o61+SAj/TGArnyvckYPEZ3TQ6qtvsQHQMHgGd1L
ris27l0vKJlL28xzZyxzx1LhUHiVtCF/+DzNMdjDXOj9YcXytK+ISm6a7HnA6PYP
5m7TA3kGiD9nnypAdcfqGo9TIvHtSYbmhWzZjigLHoR1NgtgtbLIBUXFBOB9qwQX
80RQYDbXHoEXNxCxYHhJRrssRjP/RZdin12gg/hdNSdEpP5uXQ2E9y/CJweTrQsy
i7GZgptuGTgyFZfIA1N1yvpqm/FYQdU/VIytIaLI5l+vhuWvRUY=
=0gjP
-----END PGP SIGNATURE-----

--s8ia3hVjg8lWZQf/--