pg_stat_statements: Fix parameter number gaps in normalized queries

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() where normalized query
strings are generated, these locations are not excluded from
consideration.  This could increment the parameter number counter for
every recorded occurrence at such a location, leading to an incorrect
normalization in certain cases with gaps in the numbers reported.

For example, take this query:
SELECT WHERE '1' IN ('2'::int, '3'::int::text)
Before this commit, it would be normalized like that, with gaps in the
parameter numbers:
SELECT WHERE $1 IN ($3::int, $4::int::text)
However the correct, less confusing one should be like that:
SELECT WHERE $1 IN ($2::int, $3::int::text)

This commit fixes the computation of the parameter numbers to 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.

The underlying query IDs are not changed, neither are the normalized
strings for existing PGSS hash entries.  New entries with fresh
normalized queries would automatically get reshaped based on the new
parameter numbering.

Issue discovered while discussing a separate problem for HEAD, but this
affects all the stable branches.

Author: Sami Imseih <samimseih@gmail.com>
Discussion: https://postgr.es/m/CAA5RZ0tzxvWXsacGyxrixdhy3tTTDfJQqxyFBRFh31nNHBQ5qA@mail.gmail.com
Backpatch-through: 13

diff --git a/contrib/pg_stat_statements/expected/extended.out b/contrib/pg_stat_statements/expected/extended.out
index dbc786802266292adeede5be9a978b0053ec4d6e..f60cbc7bb2eb6fef05df8b853c0b4c3ed3639f3c 100644 (file)
--- a/contrib/pg_stat_statements/expected/extended.out
+++ b/contrib/pg_stat_statements/expected/extended.out
@@ -8,3 +8,61 @@ SELECT query_id IS NOT NULL AS query_id_set
  t
 (1 row)
 
+-- Various parameter numbering patterns
+SELECT pg_stat_statements_reset() IS NOT NULL AS t;
+ t 
+---
+ t
+(1 row)
+
+-- Unique query IDs with parameter numbers switched.
+SELECT WHERE ($1::int, 7) IN ((8, $2::int), ($3::int, 9)) \bind '1' '2' '3' \g
+--
+(0 rows)
+
+SELECT WHERE ($2::int, 10) IN ((11, $3::int), ($1::int, 12)) \bind '1' '2' '3' \g
+--
+(0 rows)
+
+SELECT WHERE $1::int IN ($2::int, $3::int) \bind '1' '2' '3' \g
+--
+(0 rows)
+
+SELECT WHERE $2::int IN ($3::int, $1::int) \bind '1' '2' '3' \g
+--
+(0 rows)
+
+SELECT WHERE $3::int IN ($1::int, $2::int) \bind '1' '2' '3' \g
+--
+(0 rows)
+
+-- Two groups of two queries with the same query ID.
+SELECT WHERE '1'::int IN ($1::int, '2'::int) \bind '1' \g
+--
+(1 row)
+
+SELECT WHERE '4'::int IN ($1::int, '5'::int) \bind '2' \g
+--
+(0 rows)
+
+SELECT WHERE $2::int IN ($1::int, '1'::int) \bind '1' '2' \g
+--
+(0 rows)
+
+SELECT WHERE $2::int IN ($1::int, '2'::int) \bind '3' '4' \g
+--
+(0 rows)
+
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+                            query                             | calls 
+--------------------------------------------------------------+-------
+ SELECT WHERE $1::int IN ($2::int, $3::int)                   |     1
+ SELECT WHERE $2::int IN ($1::int, $3::int)                   |     2
+ SELECT WHERE $2::int IN ($1::int, $3::int)                   |     2
+ SELECT WHERE $2::int IN ($3::int, $1::int)                   |     1
+ SELECT WHERE $3::int IN ($1::int, $2::int)                   |     1
+ SELECT WHERE ($1::int, $4) IN (($5, $2::int), ($3::int, $6)) |     1
+ SELECT WHERE ($2::int, $4) IN (($5, $3::int), ($1::int, $6)) |     1
+ SELECT pg_stat_statements_reset() IS NOT NULL AS t           |     1
+(8 rows)
+

diff --git a/contrib/pg_stat_statements/expected/select.out b/contrib/pg_stat_statements/expected/select.out
index dd6c756f67d5b2399e084b67100dc57838444c0f..c031003e62419195243c89e9b292f7b2c6fc295d 100644 (file)
--- a/contrib/pg_stat_statements/expected/select.out
+++ b/contrib/pg_stat_statements/expected/select.out
@@ -152,6 +152,35 @@ SELECT pg_stat_statements_reset() IS NOT NULL AS t;
  t
 (1 row)
 
+-- normalization of constants and parameters, with constant locations
+-- recorded 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 WHERE (3, 4) IN ((5, 6), (8, 7));
+--
+(0 rows)
+
+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))                          |     2
+ SELECT pg_stat_statements_reset() IS NOT NULL AS t                     |     1
+ SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C" |     0
+(4 rows)
+
 --
 -- queries with locking clauses
 --

diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index 12718dfe45c2dc414b40968916ff3c0bd201da79..c5c4edce4236274865fde84b0a44579f0f37299d 100644 (file)
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -2801,6 +2801,7 @@ generate_normalized_query(JumbleState *jstate, const char *query,
                                n_quer_loc = 0, /* Normalized query byte location */
                                last_off = 0,   /* Offset from start for previous tok */
                                last_tok_len = 0;       /* Length (in bytes) of that tok */
+       int                     num_constants_replaced = 0;
 
        /*
         * Get constants' lengths (core system only gives us locations).  Note
@@ -2844,7 +2845,8 @@ generate_normalized_query(JumbleState *jstate, const char *query,
 
                /* And insert a param symbol in place of the constant token */
                n_quer_loc += sprintf(norm_query + n_quer_loc, "$%d",
-                                                         i + 1 + jstate->highest_extern_param_id);
+                                                         num_constants_replaced + 1 + jstate->highest_extern_param_id);
+               num_constants_replaced++;
 
                quer_loc = off + tok_len;
                last_off = off;

diff --git a/contrib/pg_stat_statements/sql/extended.sql b/contrib/pg_stat_statements/sql/extended.sql
index 07b6c5a93d673a3b3de185b23269983fb35ade62..1dd49cf2025038b8eaaaac875103015403b49cd8 100644 (file)
--- a/contrib/pg_stat_statements/sql/extended.sql
+++ b/contrib/pg_stat_statements/sql/extended.sql
@@ -5,3 +5,19 @@ SET pg_stat_statements.track_utility = FALSE;
 -- This test checks that an execute message sets a query ID.
 SELECT query_id IS NOT NULL AS query_id_set
   FROM pg_stat_activity WHERE pid = pg_backend_pid() \bind \g
+
+-- Various parameter numbering patterns
+SELECT pg_stat_statements_reset() IS NOT NULL AS t;
+-- Unique query IDs with parameter numbers switched.
+SELECT WHERE ($1::int, 7) IN ((8, $2::int), ($3::int, 9)) \bind '1' '2' '3' \g
+SELECT WHERE ($2::int, 10) IN ((11, $3::int), ($1::int, 12)) \bind '1' '2' '3' \g
+SELECT WHERE $1::int IN ($2::int, $3::int) \bind '1' '2' '3' \g
+SELECT WHERE $2::int IN ($3::int, $1::int) \bind '1' '2' '3' \g
+SELECT WHERE $3::int IN ($1::int, $2::int) \bind '1' '2' '3' \g
+-- Two groups of two queries with the same query ID.
+SELECT WHERE '1'::int IN ($1::int, '2'::int) \bind '1' \g
+SELECT WHERE '4'::int IN ($1::int, '5'::int) \bind '2' \g
+SELECT WHERE $2::int IN ($1::int, '1'::int) \bind '1' '2' \g
+SELECT WHERE $2::int IN ($1::int, '2'::int) \bind '3' '4' \g
+
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";

diff --git a/contrib/pg_stat_statements/sql/select.sql b/contrib/pg_stat_statements/sql/select.sql
index eb45cb81ad23f058e405509ee5f3aaf5977d397f..c419c845796070dbf01ed95b8fbca0bce5f847fd 100644 (file)
--- a/contrib/pg_stat_statements/sql/select.sql
+++ b/contrib/pg_stat_statements/sql/select.sql
@@ -58,6 +58,14 @@ DEALLOCATE pgss_test;
 SELECT calls, rows, query FROM pg_stat_statements ORDER BY query COLLATE "C";
 SELECT pg_stat_statements_reset() IS NOT NULL AS t;
 
+-- normalization of constants and parameters, with constant locations
+-- recorded 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 WHERE (3, 4) IN ((5, 6), (8, 7));
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+
 --
 -- queries with locking clauses
 --