v7-0001-Fix-broken-normalization-due-to-duplicate-constan.patch

application/octet-stream

Filename: v7-0001-Fix-broken-normalization-due-to-duplicate-constan.patch
Type: application/octet-stream
Part: 1
Message: Re: queryId constant squashing does not support prepared statements

Patch

Same data as JSON: GET /api/v1/attachments/:id/patch the parsed metadata as JSON — format, series position, per-file stats; never the diff bytes. API reference →
Format: format-patch
Series: patch v7-0001
Subject: Fix broken normalization due to duplicate constant locations
File+
contrib/pg_stat_statements/expected/normalize.out 27 0
contrib/pg_stat_statements/Makefile 1 1
contrib/pg_stat_statements/meson.build 1 0
contrib/pg_stat_statements/pg_stat_statements.c 3 7
contrib/pg_stat_statements/sql/normalize.sql 10 0
From 4e80ed16ba0a6554052246bd19f5509b13fb1514 Mon Sep 17 00:00:00 2001
From: Sami Imseih <simseih@amazon.com>
Date: Mon, 26 May 2025 21:41:17 -0500
Subject: [PATCH v7 1/4] Fix broken normalization due to duplicate constant
 locations

pg_stat_statements anticipates that certain constant
locations may be recorded multiple times and attempts
to avoid calculating a length for these locations in
fill_in_constant_lengths.

However, during generate_normalized_query, these
locations are not excluded from consideration and
will increment the $n counter for every recorded
occurrence of such a location. In practice, this can
lead to incorrect normalization in certain cases.

select where '1' IN ('2'::int, '3'::int::text)

would be normalized to:

select where $1 IN ($3, $4)

instead of the correct:

select where $1 IN ($2, $3)

This is because the left-expression, '1' is used
as an argument in the OpExpr generated for every
element in the IN clause.

To correct, track the number of constants replaced
with an $n by a separate counter instead of the
iterator used to loop through the list of locations.
---
 contrib/pg_stat_statements/Makefile           |  2 +-
 .../pg_stat_statements/expected/normalize.out | 27 +++++++++++++++++++
 contrib/pg_stat_statements/meson.build        |  1 +
 .../pg_stat_statements/pg_stat_statements.c   | 10 +++----
 contrib/pg_stat_statements/sql/normalize.sql  | 10 +++++++
 5 files changed, 42 insertions(+), 8 deletions(-)
 create mode 100644 contrib/pg_stat_statements/expected/normalize.out
 create mode 100644 contrib/pg_stat_statements/sql/normalize.sql

diff --git a/contrib/pg_stat_statements/Makefile b/contrib/pg_stat_statements/Makefile
index b2bd8794d2a..f08280bdcf7 100644
--- a/contrib/pg_stat_statements/Makefile
+++ b/contrib/pg_stat_statements/Makefile
@@ -20,7 +20,7 @@ LDFLAGS_SL += $(filter -lm, $(LIBS))
 REGRESS_OPTS = --temp-config $(top_srcdir)/contrib/pg_stat_statements/pg_stat_statements.conf
 REGRESS = select dml cursors utility level_tracking planning \
 	user_activity wal entry_timestamp privileges extended \
-	parallel cleanup oldextversions squashing
+	parallel cleanup oldextversions squashing normalize
 # Disabled because these tests require "shared_preload_libraries=pg_stat_statements",
 # which typical installcheck users do not have (e.g. buildfarm clients).
 NO_INSTALLCHECK = 1
diff --git a/contrib/pg_stat_statements/expected/normalize.out b/contrib/pg_stat_statements/expected/normalize.out
new file mode 100644
index 00000000000..1e94dbb9b43
--- /dev/null
+++ b/contrib/pg_stat_statements/expected/normalize.out
@@ -0,0 +1,27 @@
+--
+-- Validate normalization of constants
+--
+-- Ensure that there are no gaps in the generated $n parameters. The following
+-- queries will record some constant location one or more times.
+SELECT pg_stat_statements_reset() IS NOT NULL AS t;
+ t 
+---
+ t
+(1 row)
+
+SELECT WHERE '1' IN ('1'::int, '3'::int::text);
+--
+(1 row)
+
+SELECT WHERE (1, 2) IN ((1, 2), (2, 3));
+--
+(1 row)
+
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+                       query                        | calls 
+----------------------------------------------------+-------
+ SELECT WHERE $1 IN ($2::int, $3::int::text)        |     1
+ SELECT WHERE ($1, $2) IN (($3, $4), ($5, $6))      |     1
+ SELECT pg_stat_statements_reset() IS NOT NULL AS t |     1
+(3 rows)
+
diff --git a/contrib/pg_stat_statements/meson.build b/contrib/pg_stat_statements/meson.build
index 01a6cbdcf61..931a5b29427 100644
--- a/contrib/pg_stat_statements/meson.build
+++ b/contrib/pg_stat_statements/meson.build
@@ -57,6 +57,7 @@ tests += {
       'cleanup',
       'oldextversions',
       'squashing',
+      'normalize',
     ],
     'regress_args': ['--temp-config', files('pg_stat_statements.conf')],
     # Disabled because these tests require
diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index d8fdf42df79..c58f34e9f30 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -2818,9 +2818,7 @@ generate_normalized_query(JumbleState *jstate, const char *query,
 				last_off = 0,	/* Offset from start for previous tok */
 				last_tok_len = 0;	/* Length (in bytes) of that tok */
 	bool		in_squashed = false;	/* in a run of squashed consts? */
-	int			skipped_constants = 0;	/* Position adjustment of later
-										 * constants after squashed ones */
-
+	int			num_constants_replaced = 0;
 
 	/*
 	 * Get constants' lengths (core system only gives us locations).  Note
@@ -2878,7 +2876,7 @@ generate_normalized_query(JumbleState *jstate, const char *query,
 
 			/* ... and then a param symbol replacing the constant itself */
 			n_quer_loc += sprintf(norm_query + n_quer_loc, "$%d",
-								  i + 1 + jstate->highest_extern_param_id - skipped_constants);
+								  num_constants_replaced++ + 1 + jstate->highest_extern_param_id);
 
 			/* In case previous constants were merged away, stop doing that */
 			in_squashed = false;
@@ -2902,12 +2900,10 @@ generate_normalized_query(JumbleState *jstate, const char *query,
 
 			/* ... and then start a run of squashed constants */
 			n_quer_loc += sprintf(norm_query + n_quer_loc, "$%d /*, ... */",
-								  i + 1 + jstate->highest_extern_param_id - skipped_constants);
+								  num_constants_replaced++ + 1 + jstate->highest_extern_param_id);
 
 			/* The next location will match the block below, to end the run */
 			in_squashed = true;
-
-			skipped_constants++;
 		}
 		else
 		{
diff --git a/contrib/pg_stat_statements/sql/normalize.sql b/contrib/pg_stat_statements/sql/normalize.sql
new file mode 100644
index 00000000000..1252c9bc53d
--- /dev/null
+++ b/contrib/pg_stat_statements/sql/normalize.sql
@@ -0,0 +1,10 @@
+--
+-- Validate normalization of constants
+--
+
+-- Ensure that there are no gaps in the generated $n parameters. The following
+-- queries will record some constant location one or more times.
+SELECT pg_stat_statements_reset() IS NOT NULL AS t;
+SELECT WHERE '1' IN ('1'::int, '3'::int::text);
+SELECT WHERE (1, 2) IN ((1, 2), (2, 3));
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
\ No newline at end of file
-- 
2.39.5 (Apple Git-154)