Re: Ignore 2PC transaction GIDs in query jumbling
On Sat, Aug 19, 2023 at 01:47:48PM +0900, Michael Paquier wrote: > On Fri, Aug 18, 2023 at 11:31:03AM +0100, Dagfinn Ilmari Mannsåker wrote: >> I don't have a particularly strong opinion on whether we should >> distinguish DEALLOCATE ALL from DEALLOCATE (call it +0.5), but > > The difference looks important to me, especially for monitoring. After thinking a few days about it, I'd rather still make the difference between both, so applied this way. > And pgbouncer may also use both of them, actually? (Somebody, please > correct me here if necessary.) I cannot see any signs of that in pgbouncer, btw. -- Michael signature.asc Description: PGP signature
Re: Ignore 2PC transaction GIDs in query jumbling
On Fri, Aug 18, 2023 at 11:31:03AM +0100, Dagfinn Ilmari Mannsåker wrote: > I don't have a particularly strong opinion on whether we should > distinguish DEALLOCATE ALL from DEALLOCATE (call it +0.5), but The difference looks important to me, especially for monitoring. And pgbouncer may also use both of them, actually? (Somebody, please correct me here if necessary.) > 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). I don't really want to have a NULL-based property for that :) -- Michael signature.asc Description: PGP signature
Re: Ignore 2PC transaction GIDs in query jumbling
Michael Paquier 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 (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 > 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 */ > + boolisall; > + /* 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
Re: Ignore 2PC transaction GIDs in query jumbling
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? -- Michael From ad91672367f408663824b36b6dd517513d7f0a2a Mon Sep 17 00:00:00 2001 From: Michael Paquier 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
Re: Ignore 2PC transaction GIDs in query jumbling
On Mon, Aug 14, 2023 at 12:11:13PM +0100, Dagfinn Ilmari Mannsåker wrote: > 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. 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? -- Michael signature.asc Description: PGP signature
Re: Ignore 2PC transaction GIDs in query jumbling
Michael Paquier 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?= 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
Re: Ignore 2PC transaction GIDs in query jumbling
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. >> Anyway, I guess that your own >> extension modules have a need for a query ID compiled with these >> fields ignored? > > My module has a need to track statements and still work efficiently after > that. > So anything that can lead to virtually an infinite number of > pg_stat_statements > entries is a problem for me, and I guess to all the other modules / tools that > track pg_stat_statements. I guess the reason why my module is still ignoring > DEALLOCATE is because it existed before pg 9.4 (with a homemade queryid as it > wasn't exposed before that), and it just stayed there since with the rest of > still problematic statements. Yeah, probably. -- Michael 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; /* 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/contrib/pg_stat_statements/expected/utility.out b/contrib/pg_stat_statements/expected/utility.out index 93735d5d85..823d78802b 100644 --- a/contrib/pg_stat_statements/expected/utility.out +++ b/contrib/pg_stat_statements/expected/utility.out @@ -472,6 +472,45 @@ 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 |2 | PREPARE stat_select AS SELECT $1 AS a + 1 |1 | SELECT $1 as a + 1 |1 | SELECT pg_stat_statements_reset() +(3 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/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";
Re: Ignore 2PC transaction GIDs in query jumbling
On Sun, Aug 13, 2023 at 03:25:33PM +0900, Michael Paquier wrote: > On Tue, Aug 01, 2023 at 10:46:58AM +0800, Julien Rouhaud wrote: > > > > Agreed, it should be as trivial to implement as for the 2pc commands :) > > 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. 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. > Anyway, I guess that your own > extension modules have a need for a query ID compiled with these > fields ignored? My module has a need to track statements and still work efficiently after that. So anything that can lead to virtually an infinite number of pg_stat_statements entries is a problem for me, and I guess to all the other modules / tools that track pg_stat_statements. I guess the reason why my module is still ignoring DEALLOCATE is because it existed before pg 9.4 (with a homemade queryid as it wasn't exposed before that), and it just stayed there since with the rest of still problematic statements. > For now, I have applied the 2PC bits independently, as of 638d42a. Thanks! [1] https://www.postgresql.org/message-id/flat/alpine.DEB.2.10.1404011631560.2557%40sto
Re: Ignore 2PC transaction GIDs in query jumbling
On Tue, Aug 01, 2023 at 10:46:58AM +0800, Julien Rouhaud wrote: > On Tue, Aug 01, 2023 at 11:37:49AM +0900, Michael Paquier wrote: >> On Tue, Aug 01, 2023 at 10:22:09AM +0800, Julien Rouhaud wrote: >>> Looking at the rest of the ignored patterns, the only remaining one would be >>> DEALLOCATE, which AFAICS doesn't have a query_jumble_ignore tag for now. >> >> This one seems to be simple as well with a location field, looking >> quickly at its Node. > > Agreed, it should be as trivial to implement as for the 2pc commands :) 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. Anyway, I guess that your own extension modules have a need for a query ID compiled with these fields ignored? For now, I have applied the 2PC bits independently, as of 638d42a. -- Michael signature.asc Description: PGP signature
Re: Ignore 2PC transaction GIDs in query jumbling
On Tue, Aug 01, 2023 at 11:37:49AM +0900, Michael Paquier wrote: > On Tue, Aug 01, 2023 at 10:22:09AM +0800, Julien Rouhaud wrote: > > Looking at the rest of the ignored patterns, the only remaining one would be > > DEALLOCATE, which AFAICS doesn't have a query_jumble_ignore tag for now. > > This one seems to be simple as well with a location field, looking > quickly at its Node. Agreed, it should be as trivial to implement as for the 2pc commands :)
Re: Ignore 2PC transaction GIDs in query jumbling
On Tue, Aug 01, 2023 at 10:22:09AM +0800, Julien Rouhaud wrote: > Looking at the rest of the ignored patterns, the only remaining one would be > DEALLOCATE, which AFAICS doesn't have a query_jumble_ignore tag for now. This one seems to be simple as well with a location field, looking quickly at its Node. -- Michael signature.asc Description: PGP signature
Re: Ignore 2PC transaction GIDs in query jumbling
On Tue, Aug 01, 2023 at 11:00:32AM +0900, Michael Paquier wrote: > On Tue, Aug 01, 2023 at 09:28:08AM +0800, Julien Rouhaud wrote: > > > FTR we had to entirely ignore all those statements in powa years ago to try > > to > > make the tool usable in such case for some users who where using 2pc, it > > would > > be nice to be able to track them back for pg16+. > > That would be nice for your users. Indeed, although I will need to make that part a runtime variable based on the server version rather than a hardcoded string since the extension framework doesn't provide a way to do that cleanly across major pg versions. > Are there more query patterns you'd > be interested in grouping in the backend? I had a few folks aiming > for CallStmt and SetStmt, but both a a bit tricky without a custom > routine. Looking at the rest of the ignored patterns, the only remaining one would be DEALLOCATE, which AFAICS doesn't have a query_jumble_ignore tag for now.
Re: Ignore 2PC transaction GIDs in query jumbling
On Tue, Aug 01, 2023 at 09:28:08AM +0800, Julien Rouhaud wrote: > Having an application relying on 2pc leads to pg_stat_statements being > virtually unusable on the whole instance, so +1 for the patch. Cool, thanks for the feedback! > FTR we had to entirely ignore all those statements in powa years ago to try to > make the tool usable in such case for some users who where using 2pc, it would > be nice to be able to track them back for pg16+. That would be nice for your users. Are there more query patterns you'd be interested in grouping in the backend? I had a few folks aiming for CallStmt and SetStmt, but both a a bit tricky without a custom routine. -- Michael signature.asc Description: PGP signature
Re: Ignore 2PC transaction GIDs in query jumbling
Hi, On Tue, Aug 01, 2023 at 09:38:14AM +0900, Michael Paquier wrote: > > 31de7e6 has silenced savepoint names in the query jumbling, and > something similar can be done for 2PC transactions once the GID is > ignored in TransactionStmt. This leads to the following grouping in > pg_stat_statements, for instance, which is something that matters with > workloads that heavily rely on 2PC: > COMMIT PREPARED $1 > PREPARE TRANSACTION $1 > ROLLBACK PREPARED $1 Having an application relying on 2pc leads to pg_stat_statements being virtually unusable on the whole instance, so +1 for the patch. FTR we had to entirely ignore all those statements in powa years ago to try to make the tool usable in such case for some users who where using 2pc, it would be nice to be able to track them back for pg16+. The patch LGTM.
Ignore 2PC transaction GIDs in query jumbling
Hi all, 31de7e6 has silenced savepoint names in the query jumbling, and something similar can be done for 2PC transactions once the GID is ignored in TransactionStmt. This leads to the following grouping in pg_stat_statements, for instance, which is something that matters with workloads that heavily rely on 2PC: COMMIT PREPARED $1 PREPARE TRANSACTION $1 ROLLBACK PREPARED $1 Thoughts or comments? -- Michael diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h index fe003ded50..2565348303 100644 --- a/src/include/nodes/parsenodes.h +++ b/src/include/nodes/parsenodes.h @@ -3540,7 +3540,8 @@ typedef struct TransactionStmt List *options; /* for BEGIN/START commands */ /* for savepoint commands */ char *savepoint_name pg_node_attr(query_jumble_ignore); - char *gid; /* for two-phase-commit related commands */ + /* for two-phase-commit related commands */ + char *gid pg_node_attr(query_jumble_ignore); bool chain; /* AND CHAIN option */ /* token location, or -1 if unknown */ int location pg_node_attr(query_jumble_location); diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y index 15ece871a0..b3bdf947b6 100644 --- a/src/backend/parser/gram.y +++ b/src/backend/parser/gram.y @@ -10924,7 +10924,7 @@ TransactionStmt: n->kind = TRANS_STMT_PREPARE; n->gid = $3; - n->location = -1; + n->location = @3; $$ = (Node *) n; } | COMMIT PREPARED Sconst @@ -10933,7 +10933,7 @@ TransactionStmt: n->kind = TRANS_STMT_COMMIT_PREPARED; n->gid = $3; - n->location = -1; + n->location = @3; $$ = (Node *) n; } | ROLLBACK PREPARED Sconst @@ -10942,7 +10942,7 @@ TransactionStmt: n->kind = TRANS_STMT_ROLLBACK_PREPARED; n->gid = $3; - n->location = -1; + n->location = @3; $$ = (Node *) n; } ; diff --git a/contrib/pg_stat_statements/expected/utility.out b/contrib/pg_stat_statements/expected/utility.out index 3d920fb5f7..93735d5d85 100644 --- a/contrib/pg_stat_statements/expected/utility.out +++ b/contrib/pg_stat_statements/expected/utility.out @@ -197,6 +197,29 @@ SELECT pg_stat_statements_reset(); (1 row) +-- Two-phase transactions +BEGIN; +PREPARE TRANSACTION 'stat_trans1'; +COMMIT PREPARED 'stat_trans1'; +BEGIN; +PREPARE TRANSACTION 'stat_trans2'; +ROLLBACK PREPARED 'stat_trans2'; +SELECT calls, rows, query FROM pg_stat_statements ORDER BY query COLLATE "C"; + calls | rows | query +---+--+--- + 2 |0 | BEGIN + 1 |0 | COMMIT PREPARED $1 + 2 |0 | PREPARE TRANSACTION $1 + 1 |0 | ROLLBACK PREPARED $1 + 1 |1 | SELECT pg_stat_statements_reset() +(5 rows) + +SELECT pg_stat_statements_reset(); + pg_stat_statements_reset +-- + +(1 row) + -- Savepoints BEGIN; SAVEPOINT sp1; diff --git a/contrib/pg_stat_statements/pg_stat_statements.conf b/contrib/pg_stat_statements/pg_stat_statements.conf index 13346e2807..0e900d7119 100644 --- a/contrib/pg_stat_statements/pg_stat_statements.conf +++ b/contrib/pg_stat_statements/pg_stat_statements.conf @@ -1 +1,2 @@ shared_preload_libraries = 'pg_stat_statements' +max_prepared_transactions = 5 diff --git a/contrib/pg_stat_statements/sql/utility.sql b/contrib/pg_stat_statements/sql/utility.sql index 859e57955e..87666d9135 100644 --- a/contrib/pg_stat_statements/sql/utility.sql +++ b/contrib/pg_stat_statements/sql/utility.sql @@ -115,6 +115,16 @@ COMMIT; SELECT calls, rows, query FROM pg_stat_statements ORDER BY query COLLATE "C"; SELECT pg_stat_statements_reset(); +-- Two-phase transactions +BEGIN; +PREPARE TRANSACTION 'stat_trans1'; +COMMIT PREPARED 'stat_trans1'; +BEGIN; +PREPARE TRANSACTION 'stat_trans2'; +ROLLBACK PREPARED 'stat_trans2'; +SELECT calls, rows, query FROM pg_stat_statements ORDER BY query COLLATE "C"; +SELECT pg_stat_statements_reset(); + -- Savepoints BEGIN; SAVEPOINT sp1; signature.asc Description: PGP signature