Feedback-ID: i0fe9450f:Fastmail
Date: Fri, 21 Mar 2025 09:54:55 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Christoph Berg <myon@debian.org>
Cc: 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: <Z9y43-dRvb4EtxQ0@paquier.xyz>
References: <OSZP286MB2253413D25A5D93BEA4F6459F2432@OSZP286MB2253.JPNP286.PROD.OUTLOOK.COM>
 <ZbtabEYMaZ3-wcxv@paquier.xyz>
 <Z9iWXKGwkm8RAC93@msg.df7cb.de>
 <Z9ju-lB4r6fXcIKS@paquier.xyz>
 <Z9mkqplmUpQ4xG52@msg.df7cb.de>
 <Z9n_K2YGbadpzD_s@paquier.xyz>
 <Z9qybg3FxTUefRT7@msg.df7cb.de>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="CO/zzGKTvHsx2cV5"
Content-Disposition: inline
In-Reply-To: <Z9qybg3FxTUefRT7@msg.df7cb.de>
Archived-At: <https://www.postgresql.org/message-id/Z9y43-dRvb4EtxQ0%40paquier.xyz>
Precedence: bulk


--CO/zzGKTvHsx2cV5
Content-Type: multipart/mixed; boundary="Sg+ShwoaNQu87j97"
Content-Disposition: inline


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

On Wed, Mar 19, 2025 at 01:02:54PM +0100, Christoph Berg wrote:
> You are of course right, that one-line comment was just snakeoil :D.
> Now there are proper ones, thanks.

I have been thinking about this patch for a couple of days.  What
makes me unhappy in this proposal is that RangeTblEntry is a large and
complicated Node, and we only want to force a custom computation for
the "relid" portion of the node, while being able to also take some
decisions for what the parent node has.  Leaving the existing
per-field query_jumble_ignore with the custom function is prone to
errors, as well, because we need to maintain some level of consistency
between parsenodes.h and src/backend/nodes/.

Hence here is a counter-proposal, that can bring the best of both
worlds: let's add support for custom_query_jumble at field level.
I've spent some time on that, and some concatenation in a macro used
by gen_node_support.pl to force a policy for the custom function name
and its arguments is proving to be rather straight-forward.  This
approach addresses the problem of this thread, while also tackling my=20
concerns about complex node structures.

The custom functions are named _jumble${node}_${field}, with the field
and the parent node given as arguments.  I agree that giving the field
is kind of pointless if you have the parent node, but I think that
this is better so as this forces developers to think about how to use
the field value with the node.

Please see the attached.  What do you think?=20
--
Michael

--Sg+ShwoaNQu87j97
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v3-0001-Add-support-for-custom_query_jumble-at-field-leve.patch"
Content-Transfer-Encoding: quoted-printable

=46rom 11e3154cabdc24feb14e91d35c0cfee5f6c0ca2c Mon Sep 17 00:00:00 2001
=46rom: Michael Paquier <michael@paquier.xyz>
Date: Fri, 21 Mar 2025 09:35:38 +0900
Subject: [PATCH v3 1/2] Add support for custom_query_jumble at field-level

This option gives the possibility for query jumbling to force custom
jumbling policies specific to a field of a node.  When dealing with
complex node structures, this is simpler than having to enforce a custom
function across the full node.

Custom functions need to be defined as _jumble${node}_${field}, are
given in input the parent node and the field value.  The field value is
not really required if we have the parent Node, but it makes custom
implementations easier to follow with field-specific definitions and
values.  The code generated by gen_node_support.pl uses a macro called
JUMBLE_CUSTOM(), that hides the objects specific to queryjumblefuncs.c.
---
 src/include/nodes/nodes.h             |  4 ++++
 src/backend/nodes/gen_node_support.pl | 11 +++++++++++
 src/backend/nodes/queryjumblefuncs.c  |  6 ++++++
 3 files changed, 21 insertions(+)

diff --git a/src/include/nodes/nodes.h b/src/include/nodes/nodes.h
index d18044b4e650..adec68a45ab8 100644
--- a/src/include/nodes/nodes.h
+++ b/src/include/nodes/nodes.h
@@ -54,6 +54,7 @@ typedef enum NodeTag
  *   readfuncs.c.
  *
  * - custom_query_jumble: Has custom implementation in queryjumblefuncs.c.
+ *   Available as well as a per-field attribute.
  *
  * - no_copy: Does not support copyObject() at all.
  *
@@ -101,6 +102,9 @@ typedef enum NodeTag
  * - equal_ignore_if_zero: Ignore the field for equality if it is zero.
  *   (Otherwise, compare normally.)
  *
+ * - custom_query_jumble: Has custom implementation in queryjumblefuncs.c
+ *   applied for the field of a node.  Available as well as a node attribu=
te.
+ *
  * - query_jumble_ignore: Ignore the field for the query jumbling.  Note
  *   that typmod and collation information are usually irrelevant for the
  *   query jumbling.
diff --git a/src/backend/nodes/gen_node_support.pl b/src/backend/nodes/gen_=
node_support.pl
index 7e3f335ac09d..bcae70cea7d4 100644
--- a/src/backend/nodes/gen_node_support.pl
+++ b/src/backend/nodes/gen_node_support.pl
@@ -471,6 +471,7 @@ foreach my $infile (@ARGV)
 								&& $attr !~ /^read_as\(\w+\)$/
 								&& !elem $attr,
 								qw(copy_as_scalar
+								custom_query_jumble
 								equal_as_scalar
 								equal_ignore
 								equal_ignore_if_zero
@@ -1283,12 +1284,17 @@ _jumble${n}(JumbleState *jstate, Node *node)
 		my $t =3D $node_type_info{$n}->{field_types}{$f};
 		my @a =3D @{ $node_type_info{$n}->{field_attrs}{$f} };
 		my $query_jumble_ignore =3D $struct_no_query_jumble;
+		my $query_jumble_custom =3D 0;
 		my $query_jumble_location =3D 0;
 		my $query_jumble_squash =3D 0;
=20
 		# extract per-field attributes
 		foreach my $a (@a)
 		{
+			if ($a eq 'custom_query_jumble')
+			{
+				$query_jumble_custom =3D 1;
+			}
 			if ($a eq 'query_jumble_ignore')
 			{
 				$query_jumble_ignore =3D 1;
@@ -1328,6 +1334,11 @@ _jumble${n}(JumbleState *jstate, Node *node)
 				  unless $query_jumble_ignore;
 			}
 		}
+		elsif ($query_jumble_custom)
+		{
+			# Custom function that applies to one field of a node.
+			print $jff "\tJUMBLE_CUSTOM($n, $f);\n";
+		}
 		elsif ($t eq 'char*')
 		{
 			print $jff "\tJUMBLE_STRING($f);\n"
diff --git a/src/backend/nodes/queryjumblefuncs.c b/src/backend/nodes/query=
jumblefuncs.c
index 189bfda610aa..9b9cf6f34381 100644
--- a/src/backend/nodes/queryjumblefuncs.c
+++ b/src/backend/nodes/queryjumblefuncs.c
@@ -333,6 +333,12 @@ do { \
 	if (expr->str) \
 		AppendJumble(jstate, (const unsigned char *) (expr->str), strlen(expr->s=
tr) + 1); \
 } while(0)
+/*
+ * Note that the argument types are enforced for the per-field custom
+ * functions.
+ */
+#define JUMBLE_CUSTOM(nodetype, item) \
+	_jumble##nodetype##_##item(jstate, expr, expr->item)
=20
 #include "queryjumblefuncs.funcs.c"
=20
--=20
2.49.0


--Sg+ShwoaNQu87j97
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v3-0002-Add-custom-query-jumble-function-for-RangeTblEntr.patch"
Content-Transfer-Encoding: quoted-printable

=46rom 1c5b1a99aee4d0872d42b6edfdaab1266d13a522 Mon Sep 17 00:00:00 2001
=46rom: Michael Paquier <michael@paquier.xyz>
Date: Fri, 21 Mar 2025 09:43:48 +0900
Subject: [PATCH v3 2/2] Add custom query jumble function for
 RangeTblEntry.relid

---
 src/include/nodes/parsenodes.h                |  9 +++--
 src/backend/nodes/queryjumblefuncs.c          | 27 +++++++++++++++
 .../pg_stat_statements/expected/utility.out   | 33 +++++++++++++++++++
 contrib/pg_stat_statements/sql/utility.sql    | 12 +++++++
 4 files changed, 79 insertions(+), 2 deletions(-)

diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 23c9e3c5abf2..3eb16846e0e1 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -1093,8 +1093,13 @@ typedef struct RangeTblEntry
 	 * relation.  This allows plans referencing AFTER trigger transition
 	 * tables to be invalidated if the underlying table is altered.
 	 */
-	/* OID of the relation */
-	Oid			relid;
+
+	/*
+	 * OID of the relation.  A custom query jumble function is used here for
+	 * temporary tables, where the computation uses the relation name instead
+	 * of the OID.
+	 */
+	Oid			relid pg_node_attr(custom_query_jumble);
 	/* 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 9b9cf6f34381..fbb05eab1bbe 100644
--- a/src/backend/nodes/queryjumblefuncs.c
+++ b/src/backend/nodes/queryjumblefuncs.c
@@ -33,6 +33,7 @@
 #include "postgres.h"
=20
 #include "access/transam.h"
+#include "catalog/namespace.h"
 #include "catalog/pg_proc.h"
 #include "common/hashfn.h"
 #include "miscadmin.h"
@@ -67,6 +68,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_relid(JumbleState *jstate,
+									   RangeTblEntry *expr,
+									   Oid relid);
=20
 /*
  * Given a possibly multi-statement source string, confine our attention t=
o the
@@ -519,3 +523,26 @@ _jumbleVariableSetStmt(JumbleState *jstate, Node *node)
 	JUMBLE_FIELD(is_local);
 	JUMBLE_LOCATION(location);
 }
+
+/*
+ * Custom query jumble function for RangeTblEntry.relid.
+ */
+static void
+_jumbleRangeTblEntry_relid(JumbleState *jstate,
+						   RangeTblEntry *expr,
+						   Oid relid)
+{
+	/*
+	 * If this is a temporary table, jumble its name instead of the table OID.
+	 */
+	if (expr->rtekind =3D=3D RTE_RELATION &&
+		isAnyTempNamespace(get_rel_namespace(expr->relid)))
+	{
+		char	   *relname =3D get_rel_name(expr->relid);
+
+		AppendJumble(jstate, (const unsigned char *) "pg_temp", sizeof("pg_temp"=
));
+		AppendJumble(jstate, (const unsigned char *) relname, strlen(relname));
+	}
+	else
+		JUMBLE_FIELD(relid);
+}
diff --git a/contrib/pg_stat_statements/expected/utility.out b/contrib/pg_s=
tat_statements/expected/utility.out
index aa4f0f7e6280..941ba0e66deb 100644
--- a/contrib/pg_stat_statements/expected/utility.out
+++ b/contrib/pg_stat_statements/expected/utility.out
@@ -9,6 +9,39 @@ SELECT pg_stat_statements_reset() IS NOT NULL AS t;
  t
 (1 row)
=20
+-- Temporary tables, grouped together.
+BEGIN;
+  CREATE TEMP TABLE temp_t (id int) ON COMMIT DROP;
+  SELECT * FROM temp_t;
+ id=20
+----
+(0 rows)
+
+COMMIT;
+BEGIN;
+  CREATE TEMP TABLE temp_t (id int) ON COMMIT DROP;
+  SELECT * FROM temp_t;
+ id=20
+----
+(0 rows)
+
+COMMIT;
+SELECT calls, query FROM pg_stat_statements ORDER BY query COLLATE "C";
+ calls |                       query                       =20
+-------+----------------------------------------------------
+     2 | BEGIN
+     2 | COMMIT
+     2 | CREATE TEMP TABLE temp_t (id int) ON COMMIT DROP
+     2 | SELECT * FROM temp_t
+     1 | SELECT pg_stat_statements_reset() IS NOT NULL AS t
+(5 rows)
+
+SELECT pg_stat_statements_reset() IS NOT NULL AS t;
+ t=20
+---
+ t
+(1 row)
+
 -- Tables, indexes, triggers
 CREATE TEMP TABLE tab_stats (a int, b char(20));
 CREATE INDEX index_stats ON tab_stats(b, (b || 'data1'), (b || 'data2')) W=
HERE a > 0;
diff --git a/contrib/pg_stat_statements/sql/utility.sql b/contrib/pg_stat_s=
tatements/sql/utility.sql
index dd97203c2102..e21b656d44a8 100644
--- a/contrib/pg_stat_statements/sql/utility.sql
+++ b/contrib/pg_stat_statements/sql/utility.sql
@@ -6,6 +6,18 @@
 SET pg_stat_statements.track_utility =3D TRUE;
 SELECT pg_stat_statements_reset() IS NOT NULL AS t;
=20
+-- Temporary tables, grouped together.
+BEGIN;
+  CREATE TEMP TABLE temp_t (id int) ON COMMIT DROP;
+  SELECT * FROM temp_t;
+COMMIT;
+BEGIN;
+  CREATE TEMP TABLE temp_t (id int) ON COMMIT DROP;
+  SELECT * FROM temp_t;
+COMMIT;
+SELECT calls, query FROM pg_stat_statements ORDER BY query COLLATE "C";
+SELECT pg_stat_statements_reset() IS NOT NULL AS t;
+
 -- Tables, indexes, triggers
 CREATE TEMP TABLE tab_stats (a int, b char(20));
 CREATE INDEX index_stats ON tab_stats(b, (b || 'data1'), (b || 'data2')) W=
HERE a > 0;
--=20
2.49.0


--Sg+ShwoaNQu87j97--

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

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmfcuN8ACgkQnvQgOdby
QH3ddg/+IhEse1ylksTN7kFoH4mICIo3LT80KJWwve53Q1lGqkqQXKROuxHvo0cw
Ivl33fj6n26sbbhBEkVfDmaORsu2bWsFPHVsciVqcB5QoKzo8j8Q27WrNMT5XBqO
0LVHi+Ea1Nqc+mrIdY9K89+jLrdnwobjCMLzMWJ2lPsv6dtw1JUQX2K8MvdPwPUs
KK4m3i5OBtfARsTJl5qtyzpJtllKhW2EJaNPGbzOcpE237F3l8ChmRHqUoV/EmXy
ooZ4OQmSgpXznGZGHwwE3cHGvgcKkIA0MDZQNSX809GHWizyRqkd6BF3G1M4ubZe
trk4/jQCepY6ye83wcIX5H5ZS97lNmzCBsismpDCxeAq5uSkiw0wZIpJV9GZLNOO
o8FSHdhanJ99IpULIi/wHZ8okEpCZW594F9hobmSTlMQBbDSpVZgELr9GyMZEiwq
VRBzIMGQlAVmFQDp5ZB4lpm6hyYpswqhlEBOGoroEalP3czXm7fJSG1O2hsV1cSN
fCVhP9gjyEM//A1g+ChhtTDN8sHVqFJgUNlfJw/TOyXdr13EYGgCJfizc04cwhxi
IBfsPg/LXajWKUK+5TNo4h4eDAVjTvFWmmB6/FvJIaFf1ng56u737fZZVn5XSryB
OVWGBMaPYPqx1h9gzjkRLSaUE5EJcr/xiqvDbI3u7O8t+8Kq+Qg=
=uu0N
-----END PGP SIGNATURE-----

--CO/zzGKTvHsx2cV5--