Re: Ignore 2PC transaction GIDs in query jumbling

Michael Paquier <michael@paquier.xyz> writes:

> On Sun, Aug 13, 2023 at 02:48:22PM +0800, Julien Rouhaud wrote:
>> On Sun, Aug 13, 2023 at 03:25:33PM +0900, Michael Paquier wrote:
>>> Perhaps not as much, actually, because I was just reminded that
>>> DEALLOCATE is something that pg_stat_statements ignores.  So this
>>> makes harder the introduction of tests.
>> 
>> Maybe it's time to revisit that?  According to [1] the reason why
>> pg_stat_statements currently ignores DEALLOCATE is because it indeed bloated
>> the entries but also because at that time the suggestion for jumbling only this
>> one was really hackish.
>
> Good point.  The argument of the other thread does not really hold
> much these days now that query jumbling can happen for all the utility
> nodes.
>
>> Now that we do have a sensible approach to jumble utility statements, I think
>> it would be beneficial to be able to track those, for instance to be easily
>> diagnose a driver that doesn't rely on the extended protocol.
>
> Fine by me.  Would you like to write a patch?  I've begun typing an
> embryon of patch a few days ago, and did not look yet at the rest.
> Please see the attached.

As far as I could tell the only thing missing was removing
DeallocateStmt from the list of unhandled utility statement types (and
updating comments to match).  Updated patch attached.

- ilmari

From 7f11e362ef8c097a78463435a4bd1ab1031ea233 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= <ilmari@ilmari.org>
Date: Mon, 14 Aug 2023 11:59:44 +0100
Subject: [PATCH] Track DEALLOCATE statements in pg_stat_activity

Treat the statement name as a constant when jumbling.
---
 .../pg_stat_statements/expected/utility.out   | 40 +++++++++++++++++++
 .../pg_stat_statements/pg_stat_statements.c   |  8 +---
 contrib/pg_stat_statements/sql/utility.sql    | 13 ++++++
 src/backend/parser/gram.y                     |  4 ++
 src/include/nodes/parsenodes.h                |  6 ++-
 5 files changed, 63 insertions(+), 8 deletions(-)

diff --git a/contrib/pg_stat_statements/expected/utility.out b/contrib/pg_stat_statements/expected/utility.out
index 93735d5d85..3681374869 100644
--- a/contrib/pg_stat_statements/expected/utility.out
+++ b/contrib/pg_stat_statements/expected/utility.out
@@ -472,6 +472,46 @@ SELECT pg_stat_statements_reset();
  
 (1 row)
 
+-- Execution statements
+SELECT 1 as a;
+ a 
+---
+ 1
+(1 row)
+
+PREPARE stat_select AS SELECT $1 AS a;
+EXECUTE stat_select (1);
+ a 
+---
+ 1
+(1 row)
+
+DEALLOCATE stat_select;
+PREPARE stat_select AS SELECT $1 AS a;
+EXECUTE stat_select (2);
+ a 
+---
+ 2
+(1 row)
+
+DEALLOCATE PREPARE stat_select;
+DEALLOCATE ALL;
+DEALLOCATE PREPARE ALL;
+SELECT calls, rows, query FROM pg_stat_statements ORDER BY query COLLATE "C";
+ calls | rows |                 query                 
+-------+------+---------------------------------------
+     4 |    0 | DEALLOCATE $1
+     2 |    2 | PREPARE stat_select AS SELECT $1 AS a
+     1 |    1 | SELECT $1 as a
+     1 |    1 | SELECT pg_stat_statements_reset()
+(4 rows)
+
+SELECT pg_stat_statements_reset();
+ pg_stat_statements_reset 
+--------------------------
+ 
+(1 row)
+
 -- SET statements.
 -- These use two different strings, still they count as one entry.
 SET work_mem = '1MB';
diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index 55b957d251..06b65aeef5 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -104,8 +104,7 @@ static const uint32 PGSS_PG_MAJOR_VERSION = PG_VERSION_NUM / 100;
  * ignores.
  */
 #define PGSS_HANDLED_UTILITY(n)        (!IsA(n, ExecuteStmt) && \
-                                    !IsA(n, PrepareStmt) && \
-                                    !IsA(n, DeallocateStmt))
+                                    !IsA(n, PrepareStmt))
 
 /*
  * Extension version number, for supporting older extension versions' objects
@@ -830,8 +829,7 @@ pgss_post_parse_analyze(ParseState *pstate, Query *query, JumbleState *jstate)
 
     /*
      * Clear queryId for prepared statements related utility, as those will
-     * inherit from the underlying statement's one (except DEALLOCATE which is
-     * entirely untracked).
+     * inherit from the underlying statement's one.
      */
     if (query->utilityStmt)
     {
@@ -1116,8 +1114,6 @@ pgss_ProcessUtility(PlannedStmt *pstmt, const char *queryString,
      * calculated from the query tree) would be used to accumulate costs of
      * ensuing EXECUTEs.  This would be confusing, and inconsistent with other
      * cases where planning time is not included at all.
-     *
-     * Likewise, we don't track execution of DEALLOCATE.
      */
     if (pgss_track_utility && pgss_enabled(exec_nested_level) &&
         PGSS_HANDLED_UTILITY(parsetree))
diff --git a/contrib/pg_stat_statements/sql/utility.sql b/contrib/pg_stat_statements/sql/utility.sql
index 87666d9135..5f7d4a467f 100644
--- a/contrib/pg_stat_statements/sql/utility.sql
+++ b/contrib/pg_stat_statements/sql/utility.sql
@@ -237,6 +237,19 @@ DROP DOMAIN domain_stats;
 SELECT calls, rows, query FROM pg_stat_statements ORDER BY query COLLATE "C";
 SELECT pg_stat_statements_reset();
 
+-- Execution statements
+SELECT 1 as a;
+PREPARE stat_select AS SELECT $1 AS a;
+EXECUTE stat_select (1);
+DEALLOCATE stat_select;
+PREPARE stat_select AS SELECT $1 AS a;
+EXECUTE stat_select (2);
+DEALLOCATE PREPARE stat_select;
+DEALLOCATE ALL;
+DEALLOCATE PREPARE ALL;
+SELECT calls, rows, query FROM pg_stat_statements ORDER BY query COLLATE "C";
+SELECT pg_stat_statements_reset();
+
 -- SET statements.
 -- These use two different strings, still they count as one entry.
 SET work_mem = '1MB';
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index b3bdf947b6..680c7f3683 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -11936,6 +11936,7 @@ DeallocateStmt: DEALLOCATE name
                         DeallocateStmt *n = makeNode(DeallocateStmt);
 
                         n->name = $2;
+                        n->location = @2;
                         $$ = (Node *) n;
                     }
                 | DEALLOCATE PREPARE name
@@ -11943,6 +11944,7 @@ DeallocateStmt: DEALLOCATE name
                         DeallocateStmt *n = makeNode(DeallocateStmt);
 
                         n->name = $3;
+                        n->location = @3;
                         $$ = (Node *) n;
                     }
                 | DEALLOCATE ALL
@@ -11950,6 +11952,7 @@ DeallocateStmt: DEALLOCATE name
                         DeallocateStmt *n = makeNode(DeallocateStmt);
 
                         n->name = NULL;
+                        n->location = -1;
                         $$ = (Node *) n;
                     }
                 | DEALLOCATE PREPARE ALL
@@ -11957,6 +11960,7 @@ DeallocateStmt: DEALLOCATE name
                         DeallocateStmt *n = makeNode(DeallocateStmt);
 
                         n->name = NULL;
+                        n->location = -1;
                         $$ = (Node *) n;
                     }
         ;
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 2565348303..b7aeebe3b4 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -3925,8 +3925,10 @@ typedef struct ExecuteStmt
 typedef struct DeallocateStmt
 {
     NodeTag        type;
-    char       *name;            /* The name of the plan to remove */
-    /* NULL means DEALLOCATE ALL */
+    /* The name of the plan to remove.  NULL means DEALLOCATE ALL. */
+    char       *name pg_node_attr(query_jumble_ignore);
+    /* token location, or -1 if unknown */
+    int            location pg_node_attr(query_jumble_location);
 } DeallocateStmt;
 
 /*
-- 
2.39.2