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: <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> <87msytq3ou.fsf@wibble.ilmari.org>
	<ZNq9kRwWbKzvR+2a@paquier.xyz> <ZN64y89ECQnbOd9+@paquier.xyz>
Date: Fri, 18 Aug 2023 11:31:03 +0100
In-Reply-To: <ZN64y89ECQnbOd9+@paquier.xyz> (Michael Paquier's message of
	"Fri, 18 Aug 2023 09:18:19 +0900")
Message-ID: <87il9cprq0.fsf@wibble.ilmari.org>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Archived-At: 
 <https://www.postgresql.org/message-id/87il9cprq0.fsf%40wibble.ilmari.org>
Precedence: bulk

Michael Paquier <michael@paquier.xyz> writes:

> On Tue, Aug 15, 2023 at 08:49:37AM +0900, Michael Paquier wrote:
>> Hmm.  One issue with the patch is that we finish by considering
>> DEALLOCATE ALL and DEALLOCATE $1 as the same things, compiling the
>> same query IDs.  The difference is made in the Nodes by assigning NULL
>> to the name but we would now ignore it.  Wouldn't it be better to add
>> an extra field to DeallocateStmt to track separately the named
>> deallocate queries and ALL in monitoring?
>
> In short, I would propose something like that, with a new boolean
> field in DeallocateStmt that's part of the jumbling.
>
> Dagfinn, Julien, what do you think about the attached?

I don't have a particularly strong opinion on whether we should
distinguish DEALLOCATE ALL from DEALLOCATE <stmt> (call it +0.5), but
this seems like a reasonable way to do it unless we want to invent a
query_jumble_ignore_unless_null attribute (which seems like overkill for
this one case).

- ilmari

> Michael
>
> From ad91672367f408663824b36b6dd517513d7f0a2a Mon Sep 17 00:00:00 2001
> From: Michael Paquier <michael@paquier.xyz>
> Date: Fri, 18 Aug 2023 09:12:58 +0900
> Subject: [PATCH v2] Track DEALLOCATE statements in pg_stat_activity
>
> Treat the statement name as a constant when jumbling.  A boolean field
> is added to DeallocateStmt to make a distinction between the ALL and
> named cases.
> ---
>  src/include/nodes/parsenodes.h                |  8 +++-
>  src/backend/parser/gram.y                     |  8 ++++
>  .../pg_stat_statements/expected/utility.out   | 41 +++++++++++++++++++
>  .../pg_stat_statements/pg_stat_statements.c   |  8 +---
>  contrib/pg_stat_statements/sql/utility.sql    | 13 ++++++
>  5 files changed, 70 insertions(+), 8 deletions(-)
>
> diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
> index 2565348303..2b356336eb 100644
> --- a/src/include/nodes/parsenodes.h
> +++ b/src/include/nodes/parsenodes.h
> @@ -3925,8 +3925,12 @@ 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 if DEALLOCATE ALL. */
> +	char	   *name pg_node_attr(query_jumble_ignore);
> +	/* true if DEALLOCATE ALL */
> +	bool		isall;
> +	/* token location, or -1 if unknown */
> +	int			location pg_node_attr(query_jumble_location);
>  } DeallocateStmt;
>  
>  /*
> diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
> index b3bdf947b6..df34e96f89 100644
> --- a/src/backend/parser/gram.y
> +++ b/src/backend/parser/gram.y
> @@ -11936,6 +11936,8 @@ DeallocateStmt: DEALLOCATE name
>  						DeallocateStmt *n = makeNode(DeallocateStmt);
>  
>  						n->name = $2;
> +						n->isall = false;
> +						n->location = @2;
>  						$$ = (Node *) n;
>  					}
>  				| DEALLOCATE PREPARE name
> @@ -11943,6 +11945,8 @@ DeallocateStmt: DEALLOCATE name
>  						DeallocateStmt *n = makeNode(DeallocateStmt);
>  
>  						n->name = $3;
> +						n->isall = false;
> +						n->location = @3;
>  						$$ = (Node *) n;
>  					}
>  				| DEALLOCATE ALL
> @@ -11950,6 +11954,8 @@ DeallocateStmt: DEALLOCATE name
>  						DeallocateStmt *n = makeNode(DeallocateStmt);
>  
>  						n->name = NULL;
> +						n->isall = true;
> +						n->location = -1;
>  						$$ = (Node *) n;
>  					}
>  				| DEALLOCATE PREPARE ALL
> @@ -11957,6 +11963,8 @@ DeallocateStmt: DEALLOCATE name
>  						DeallocateStmt *n = makeNode(DeallocateStmt);
>  
>  						n->name = NULL;
> +						n->isall = true;
> +						n->location = -1;
>  						$$ = (Node *) n;
>  					}
>  		;
> diff --git a/contrib/pg_stat_statements/expected/utility.out b/contrib/pg_stat_statements/expected/utility.out
> index 93735d5d85..f331044f3e 100644
> --- a/contrib/pg_stat_statements/expected/utility.out
> +++ b/contrib/pg_stat_statements/expected/utility.out
> @@ -472,6 +472,47 @@ 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                 
> +-------+------+---------------------------------------
> +     2 |    0 | DEALLOCATE $1
> +     2 |    0 | DEALLOCATE ALL
> +     2 |    2 | PREPARE stat_select AS SELECT $1 AS a
> +     1 |    1 | SELECT $1 as a
> +     1 |    1 | SELECT pg_stat_statements_reset()
> +(5 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';