From: Dagfinn Ilmari Mannsåker <ilmari@ilmari.org>
To: Michael Paquier <michael@paquier.xyz>
Cc: Julien Rouhaud <rjuju123@gmail.com>
Cc: Postgres hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: Ignore 2PC transaction GIDs in query jumbling
Date: Mon, 14 Aug 2023 12:11:13 +0100
Message-ID: <87msytq3ou.fsf@wibble.ilmari.org> (raw)
In-Reply-To: <ZNiWxttNuCVJmPXS@paquier.xyz>
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>
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
Attachments:0001-Track-DEALLOCATE-statements-in-pg_stat_activity.patch (text/x-diff, 5.8K) download
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.outindex 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.cindex 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.sqlindex 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.yindex 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.hindex 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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: ilmari@ilmari.org, michael@paquier.xyz, rjuju123@gmail.com, pgsql-hackers@lists.postgresql.org
Subject: Re: Ignore 2PC transaction GIDs in query jumbling
In-Reply-To: <87msytq3ou.fsf@wibble.ilmari.org>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox