From 4e80ed16ba0a6554052246bd19f5509b13fb1514 Mon Sep 17 00:00:00 2001 From: Sami Imseih 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)