Feedback-ID: i1ff147bf:Fastmail
From: =?utf-8?Q?Dagfinn_Ilmari_Manns=C3=A5ker?= <ilmari@ilmari.org>
To: Michael Paquier <michael@paquier.xyz>
Cc: Julien Rouhaud <rjuju123@gmail.com>,  Postgres hackers
 <pgsql-hackers@lists.postgresql.org>
Subject: Re: Ignore 2PC transaction GIDs in query jumbling
References: <ZMhT9kNtJJsHw6jK@paquier.xyz>
	<20230801012808.px3pbet4qk7l3bwk@jrouhaud>
	<ZMhnQMpS2YnvuyFb@paquier.xyz>
	<20230801022209.4ac2c4shvmu2fnnq@jrouhaud>
	<ZMhv/WvIln+NrNb8@paquier.xyz>
	<20230801024658.m4yojx4avfquylnh@jrouhaud>
	<ZNh3XV2+cubcVaWk@paquier.xyz>
	<20230813064822.qwzsyi6ezhu5uuok@jrouhaud>
	<ZNiWxttNuCVJmPXS@paquier.xyz>
Date: Mon, 14 Aug 2023 12:11:13 +0100
In-Reply-To: <ZNiWxttNuCVJmPXS@paquier.xyz> (Michael Paquier's message of
	"Sun, 13 Aug 2023 17:39:34 +0900")
Message-ID: <87msytq3ou.fsf@wibble.ilmari.org>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux)
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="=-=-="
Archived-At: 
 <https://www.postgresql.org/message-id/87msytq3ou.fsf%40wibble.ilmari.org>
Precedence: bulk

--=-=-=
Content-Type: text/plain

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


--=-=-=
Content-Type: text/x-diff
Content-Disposition: inline;
 filename=0001-Track-DEALLOCATE-statements-in-pg_stat_activity.patch

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


--=-=-=--