Re: Ignore 2PC transaction GIDs in query jumbling

Michael Paquier <michael(at)paquier(dot)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(at)paquier(dot)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';