Hi,
On 9/21/22 6:07 PM, Fujii Masao wrote:
On 2022/09/19 15:29, Drouvot, Bertrand wrote:
Please find attached v6 taking care of the remarks mentioned above.
Thanks for updating the patch!
+SET pg_stat_statements.track_utility = TRUE;
+
+-- PL/pgSQL procedure and pg_stat_statements.track = all
+-- we drop and recreate the procedures to avoid any caching funnies
+SET pg_stat_statements.track_utility = FALSE;
Could you tell me why track_utility is enabled just before it's disabled?
Thanks for looking at the new version!
No real reason, I removed those useless SET in the new V7 attached.
Could you tell me what actually happens if we don't drop and
recreate the procedures? I'd like to know what "any caching funnies" are.
Without the drop/recreate the procedure body does not appear normalized
(while the CALL itself is) when switching from track = top to track = all.
I copy-pasted this comment from the already existing "function" section
in the pg_stat_statements.sql file. This comment has been introduced for
the function section in commit 83f2061dd0. Note that the behavior would
be the same for the function case (function body does not appear
normalized without the drop/recreate).
+SELECT pg_stat_statements_reset();
+CALL MINUS_TWO(3);
+CALL MINUS_TWO(7);
+CALL SUM_TWO(3, 8);
+CALL SUM_TWO(7, 5);
+
+SELECT query, calls, rows FROM pg_stat_statements ORDER BY query
COLLATE "C";
This test set for the procedures is executed with the following
four conditions, respectively. Do we really need all of these tests?
track = top, track_utility = true
track = top, track_utility = false
track = all, track_utility = true
track = all, track_utility = false
Oh right, the track_utility = false cases have been added when we
decided up-thread to force track CALL.
But now that's probably not needed to test with track_utility = true. So
I'm just keeping track_utility = off with track = top or all in the new
V7 attached (like this is the case for the "function" section).
+begin;
+prepare transaction 'tx1';
+insert into test_tx values (1);
+commit prepared 'tx1';
The test set of 2PC commands is also executed with track_utility = on
and off, respectively. But why do we need to run that test when
track_utility = off?
That's useless, thanks for pointing out. Removed in V7 attached.
- if (query->utilityStmt)
+ if (query->utilityStmt && !jstate)
{
if (pgss_track_utility &&
!PGSS_HANDLED_UTILITY(query->utilityStmt))
"pgss_track_utility" should be
"pgss_track_utility || FORCE_TRACK_UTILITY(parsetree)" theoretically?
Good catch! That's not needed (in practice) with the current code but
that is "theoretically" needed indeed, let's add it in V7 attached
(that's safer should the code change later on).
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
diff --git a/contrib/pg_stat_statements/expected/pg_stat_statements.out
b/contrib/pg_stat_statements/expected/pg_stat_statements.out
index ff0166fb9d..f5fc2f1f38 100644
--- a/contrib/pg_stat_statements/expected/pg_stat_statements.out
+++ b/contrib/pg_stat_statements/expected/pg_stat_statements.out
@@ -272,7 +272,7 @@ FROM pg_stat_statements ORDER BY query COLLATE "C";
wal_records > $2 as wal_records_generated, +| | |
| |
wal_records >= rows as wal_records_ge_rows +| | |
| |
FROM pg_stat_statements ORDER BY query COLLATE "C" | | |
| |
- SET pg_stat_statements.track_utility = FALSE | 1 | 0 | f
| f | t
+ SET pg_stat_statements.track_utility = $1 | 1 | 0 | f
| f | t
UPDATE pgss_test SET b = $1 WHERE a > $2 | 1 | 3 | t
| t | t
(7 rows)
@@ -423,6 +423,111 @@ SELECT query, calls, rows FROM pg_stat_statements ORDER
BY query COLLATE "C";
SELECT query, calls, rows FROM pg_stat_statements ORDER BY query COLLATE "C"
| 0 | 0
(6 rows)
+-- PL/pgSQL procedure and pg_stat_statements.track = top
+CREATE PROCEDURE MINUS_TWO(i INTEGER) AS $$
+DECLARE
+ r INTEGER;
+BEGIN
+ SELECT (i - 1 - 1.0)::INTEGER INTO r;
+END; $$ LANGUAGE plpgsql;
+CREATE PROCEDURE SUM_TWO(i INTEGER, j INTEGER) AS $$
+DECLARE
+ r INTEGER;
+BEGIN
+ SELECT (j + j)::INTEGER INTO r;
+END; $$ LANGUAGE plpgsql;
+SET pg_stat_statements.track = 'top';
+SET pg_stat_statements.track_utility = FALSE;
+SELECT pg_stat_statements_reset();
+ pg_stat_statements_reset
+--------------------------
+
+(1 row)
+
+CALL MINUS_TWO(3);
+CALL MINUS_TWO(7);
+CALL SUM_TWO(3, 8);
+CALL SUM_TWO(7, 5);
+SELECT query, calls, rows FROM pg_stat_statements ORDER BY query COLLATE "C";
+ query
| calls | rows
+------------------------------------------------------------------------------+-------+------
+ CALL MINUS_TWO($1)
| 2 | 0
+ CALL SUM_TWO($1, $2)
| 2 | 0
+ SELECT pg_stat_statements_reset()
| 1 | 1
+ SELECT query, calls, rows FROM pg_stat_statements ORDER BY query COLLATE "C"
| 0 | 0
+(4 rows)
+
+--
+-- pg_stat_statements.track = all
+--
+SET pg_stat_statements.track = 'all';
+SELECT pg_stat_statements_reset();
+ pg_stat_statements_reset
+--------------------------
+
+(1 row)
+
+-- PL/pgSQL procedure and pg_stat_statements.track = all
+-- we drop and recreate the procedures to avoid any caching funnies
+DROP PROCEDURE MINUS_TWO(INTEGER);
+DROP PROCEDURE SUM_TWO(INTEGER, INTEGER);
+CREATE PROCEDURE MINUS_TWO(i INTEGER) AS $$
+DECLARE
+ r INTEGER;
+BEGIN
+ SELECT (i - 1 - 1.0)::INTEGER INTO r;
+END; $$ LANGUAGE plpgsql;
+CREATE PROCEDURE SUM_TWO(i INTEGER, j INTEGER) AS $$
+DECLARE
+ r INTEGER;
+BEGIN
+ SELECT (j + j)::INTEGER INTO r;
+END; $$ LANGUAGE plpgsql;
+CALL MINUS_TWO(3);
+CALL MINUS_TWO(7);
+CALL SUM_TWO(3, 8);
+CALL SUM_TWO(7, 5);
+SELECT query, calls, rows FROM pg_stat_statements ORDER BY query COLLATE "C";
+ query
| calls | rows
+------------------------------------------------------------------------------+-------+------
+ CALL MINUS_TWO($1)
| 2 | 0
+ CALL SUM_TWO($1, $2)
| 2 | 0
+ SELECT (i - $2 - $3)::INTEGER
| 2 | 2
+ SELECT (j + j)::INTEGER
| 2 | 2
+ SELECT pg_stat_statements_reset()
| 1 | 1
+ SELECT query, calls, rows FROM pg_stat_statements ORDER BY query COLLATE "C"
| 0 | 0
+(6 rows)
+
+SET pg_stat_statements.track_utility = TRUE;
+-- SET
+SELECT pg_stat_statements_reset();
+ pg_stat_statements_reset
+--------------------------
+
+(1 row)
+
+set enable_seqscan=false;
+set enable_seqscan=true;
+set seq_page_cost=2.0;
+set seq_page_cost=1.0;
+set enable_seqscan to default;
+set seq_page_cost to default;
+reset seq_page_cost;
+reset enable_seqscan;
+SELECT query, calls, rows FROM pg_stat_statements ORDER BY query COLLATE "C";
+ query
| calls | rows
+------------------------------------------------------------------------------+-------+------
+ SELECT pg_stat_statements_reset()
| 1 | 1
+ SELECT query, calls, rows FROM pg_stat_statements ORDER BY query COLLATE "C"
| 0 | 0
+ reset enable_seqscan
| 1 | 0
+ reset seq_page_cost
| 1 | 0
+ set enable_seqscan to default
| 1 | 0
+ set enable_seqscan=$1
| 2 | 0
+ set seq_page_cost to default
| 1 | 0
+ set seq_page_cost=$1
| 2 | 0
+(8 rows)
+
+SET pg_stat_statements.track_utility = FALSE;
--
-- queries with locking clauses
--
@@ -553,11 +658,49 @@ SELECT query, calls, rows FROM pg_stat_statements ORDER
BY query COLLATE "C";
SELECT query, calls, rows FROM pg_stat_statements ORDER BY query COLLATE "C"
| 0 | 0
(9 rows)
+-- 2PC
+SELECT pg_stat_statements_reset();
+ pg_stat_statements_reset
+--------------------------
+
+(1 row)
+
+create table test_tx (a int);
+begin;
+prepare transaction 'tx1';
+insert into test_tx values (1);
+commit prepared 'tx1';
+begin;
+prepare transaction 'tx2';
+insert into test_tx values (2);
+commit prepared 'tx2';
+begin;
+prepare transaction 'tx3';
+insert into test_tx values (3);
+rollback prepared 'tx3';
+begin;
+prepare transaction 'tx4';
+insert into test_tx values (4);
+rollback prepared 'tx4';
+SELECT query, calls, rows FROM pg_stat_statements ORDER BY query COLLATE "C";
+ query
| calls | rows
+------------------------------------------------------------------------------+-------+------
+ SELECT pg_stat_statements_reset()
| 1 | 1
+ SELECT query, calls, rows FROM pg_stat_statements ORDER BY query COLLATE "C"
| 0 | 0
+ begin
| 4 | 0
+ commit prepared $1
| 2 | 0
+ create table test_tx (a int)
| 1 | 0
+ insert into test_tx values ($1)
| 4 | 4
+ prepare transaction $1
| 4 | 0
+ rollback prepared $1
| 2 | 0
+(8 rows)
+
--
-- Track the total number of rows retrieved or affected by the utility
-- commands of COPY, FETCH, CREATE TABLE AS, CREATE MATERIALIZED VIEW,
-- REFRESH MATERIALIZED VIEW and SELECT INTO
--
+SET pg_stat_statements.track_utility = TRUE;
SELECT pg_stat_statements_reset();
pg_stat_statements_reset
--------------------------
diff --git a/contrib/pg_stat_statements/pg_stat_statements.c
b/contrib/pg_stat_statements/pg_stat_statements.c
index ba868f0de9..8929f0d310 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -106,6 +106,11 @@ static const uint32 PGSS_PG_MAJOR_VERSION = PG_VERSION_NUM
/ 100;
#define PGSS_HANDLED_UTILITY(n) (!IsA(n, ExecuteStmt) && \
!IsA(n,
PrepareStmt) && \
!IsA(n,
DeallocateStmt))
+/*
+ * Force track those utility statements
+ * whatever the value of pgss_track_utility is.
+ */
+#define FORCE_TRACK_UTILITY(n) (IsA(n, CallStmt))
/*
* Extension version number, for supporting older extension versions' objects
@@ -832,9 +837,10 @@ pgss_post_parse_analyze(ParseState *pstate, Query *query,
JumbleState *jstate)
* inherit from the underlying statement's one (except DEALLOCATE which
is
* entirely untracked).
*/
- if (query->utilityStmt)
+ if (query->utilityStmt && !jstate)
{
- if (pgss_track_utility &&
!PGSS_HANDLED_UTILITY(query->utilityStmt))
+ if ((pgss_track_utility ||
FORCE_TRACK_UTILITY(query->utilityStmt)) &&
+ !PGSS_HANDLED_UTILITY(query->utilityStmt))
query->queryId = UINT64CONST(0);
return;
}
@@ -1097,7 +1103,8 @@ pgss_ProcessUtility(PlannedStmt *pstmt, const char
*queryString,
* that user configured another extension to handle utility statements
* only.
*/
- if (pgss_enabled(exec_nested_level) && pgss_track_utility)
+ if (pgss_enabled(exec_nested_level) &&
+ (pgss_track_utility || FORCE_TRACK_UTILITY(parsetree)))
pstmt->queryId = UINT64CONST(0);
/*
@@ -1114,7 +1121,8 @@ pgss_ProcessUtility(PlannedStmt *pstmt, const char
*queryString,
*
* Likewise, we don't track execution of DEALLOCATE.
*/
- if (pgss_track_utility && pgss_enabled(exec_nested_level) &&
+ if ((pgss_track_utility || FORCE_TRACK_UTILITY(parsetree)) &&
+ pgss_enabled(exec_nested_level) &&
PGSS_HANDLED_UTILITY(parsetree))
{
instr_time start;
diff --git a/contrib/pg_stat_statements/sql/pg_stat_statements.sql
b/contrib/pg_stat_statements/sql/pg_stat_statements.sql
index a01f183727..8973138fa0 100644
--- a/contrib/pg_stat_statements/sql/pg_stat_statements.sql
+++ b/contrib/pg_stat_statements/sql/pg_stat_statements.sql
@@ -201,6 +201,81 @@ SELECT PLUS_ONE(1);
SELECT query, calls, rows FROM pg_stat_statements ORDER BY query COLLATE "C";
+-- PL/pgSQL procedure and pg_stat_statements.track = top
+CREATE PROCEDURE MINUS_TWO(i INTEGER) AS $$
+DECLARE
+ r INTEGER;
+BEGIN
+ SELECT (i - 1 - 1.0)::INTEGER INTO r;
+END; $$ LANGUAGE plpgsql;
+
+CREATE PROCEDURE SUM_TWO(i INTEGER, j INTEGER) AS $$
+DECLARE
+ r INTEGER;
+BEGIN
+ SELECT (j + j)::INTEGER INTO r;
+END; $$ LANGUAGE plpgsql;
+
+SET pg_stat_statements.track = 'top';
+SET pg_stat_statements.track_utility = FALSE;
+SELECT pg_stat_statements_reset();
+CALL MINUS_TWO(3);
+CALL MINUS_TWO(7);
+CALL SUM_TWO(3, 8);
+CALL SUM_TWO(7, 5);
+
+SELECT query, calls, rows FROM pg_stat_statements ORDER BY query COLLATE "C";
+
+--
+-- pg_stat_statements.track = all
+--
+SET pg_stat_statements.track = 'all';
+SELECT pg_stat_statements_reset();
+
+-- PL/pgSQL procedure and pg_stat_statements.track = all
+-- we drop and recreate the procedures to avoid any caching funnies
+
+DROP PROCEDURE MINUS_TWO(INTEGER);
+DROP PROCEDURE SUM_TWO(INTEGER, INTEGER);
+
+CREATE PROCEDURE MINUS_TWO(i INTEGER) AS $$
+DECLARE
+ r INTEGER;
+BEGIN
+ SELECT (i - 1 - 1.0)::INTEGER INTO r;
+END; $$ LANGUAGE plpgsql;
+
+CREATE PROCEDURE SUM_TWO(i INTEGER, j INTEGER) AS $$
+DECLARE
+ r INTEGER;
+BEGIN
+ SELECT (j + j)::INTEGER INTO r;
+END; $$ LANGUAGE plpgsql;
+
+CALL MINUS_TWO(3);
+CALL MINUS_TWO(7);
+CALL SUM_TWO(3, 8);
+CALL SUM_TWO(7, 5);
+
+SELECT query, calls, rows FROM pg_stat_statements ORDER BY query COLLATE "C";
+
+SET pg_stat_statements.track_utility = TRUE;
+
+-- SET
+SELECT pg_stat_statements_reset();
+set enable_seqscan=false;
+set enable_seqscan=true;
+set seq_page_cost=2.0;
+set seq_page_cost=1.0;
+set enable_seqscan to default;
+set seq_page_cost to default;
+reset seq_page_cost;
+reset enable_seqscan;
+
+SELECT query, calls, rows FROM pg_stat_statements ORDER BY query COLLATE "C";
+
+SET pg_stat_statements.track_utility = FALSE;
+
--
-- queries with locking clauses
--
@@ -250,11 +325,38 @@ DROP FUNCTION PLUS_TWO(INTEGER);
SELECT query, calls, rows FROM pg_stat_statements ORDER BY query COLLATE "C";
+-- 2PC
+SELECT pg_stat_statements_reset();
+
+create table test_tx (a int);
+begin;
+prepare transaction 'tx1';
+insert into test_tx values (1);
+commit prepared 'tx1';
+
+begin;
+prepare transaction 'tx2';
+insert into test_tx values (2);
+commit prepared 'tx2';
+
+begin;
+prepare transaction 'tx3';
+insert into test_tx values (3);
+rollback prepared 'tx3';
+
+begin;
+prepare transaction 'tx4';
+insert into test_tx values (4);
+rollback prepared 'tx4';
+
+SELECT query, calls, rows FROM pg_stat_statements ORDER BY query COLLATE "C";
+
--
-- Track the total number of rows retrieved or affected by the utility
-- commands of COPY, FETCH, CREATE TABLE AS, CREATE MATERIALIZED VIEW,
-- REFRESH MATERIALIZED VIEW and SELECT INTO
--
+SET pg_stat_statements.track_utility = TRUE;
SELECT pg_stat_statements_reset();
CREATE TABLE pgss_ctas AS SELECT a, 'ctas' b FROM generate_series(1, 10) a;
diff --git a/doc/src/sgml/pgstatstatements.sgml
b/doc/src/sgml/pgstatstatements.sgml
index ecf6cd6bf3..d18f3632e9 100644
--- a/doc/src/sgml/pgstatstatements.sgml
+++ b/doc/src/sgml/pgstatstatements.sgml
@@ -487,13 +487,15 @@
<para>
Plannable queries (that is, <command>SELECT</command>,
<command>INSERT</command>,
- <command>UPDATE</command>, and <command>DELETE</command>) are combined into
a single
- <structname>pg_stat_statements</structname> entry whenever they have
identical query
- structures according to an internal hash calculation. Typically, two
- queries will be considered the same for this purpose if they are
- semantically equivalent except for the values of literal constants
- appearing in the query. Utility commands (that is, all other commands)
- are compared strictly on the basis of their textual query strings, however.
+ <command>UPDATE</command>, and <command>DELETE</command>) as well as
+ <command>CALL</command>, <command>SET</command>, <command>PREPARE
TRANSACTION</command>,
+ <command>COMMIT PREPARED</command> and <command>ROLLBACK PREPARED</command>
+ are combined into a single <structname>pg_stat_statements</structname> entry
+ whenever they have identical query structures according to an internal hash
calculation.
+ Typically, two queries will be considered the same for this purpose if they
are
+ semantically equivalent except for the values of literal constants appearing
+ in the command. All other commands are compared strictly on the basis
+ of their textual query strings, however.
</para>
<note>
@@ -781,9 +783,9 @@
<listitem>
<para>
<varname>pg_stat_statements.track_utility</varname> controls whether
- utility commands are tracked by the module. Utility commands are
+ utility commands are tracked by the module. Tracked utility commands are
all those other than <command>SELECT</command>,
<command>INSERT</command>,
- <command>UPDATE</command> and <command>DELETE</command>.
+ <command>UPDATE</command>, <command>DELETE</command> and
<command>CALL</command>.
The default value is <literal>on</literal>.
Only superusers can change this setting.
</para>
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 0d8d292850..949194c7c6 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -10890,6 +10890,7 @@ TransactionStmt:
n->kind = TRANS_STMT_PREPARE;
n->gid = $3;
+ n->gid_location = @3;
$$ = (Node *) n;
}
| COMMIT PREPARED Sconst
@@ -10898,6 +10899,7 @@ TransactionStmt:
n->kind = TRANS_STMT_COMMIT_PREPARED;
n->gid = $3;
+ n->gid_location = @3;
$$ = (Node *) n;
}
| ROLLBACK PREPARED Sconst
@@ -10906,6 +10908,7 @@ TransactionStmt:
n->kind = TRANS_STMT_ROLLBACK_PREPARED;
n->gid = $3;
+ n->gid_location = @3;
$$ = (Node *) n;
}
;
diff --git a/src/backend/utils/misc/queryjumble.c
b/src/backend/utils/misc/queryjumble.c
index 6e75acda27..dfcef7a414 100644
--- a/src/backend/utils/misc/queryjumble.c
+++ b/src/backend/utils/misc/queryjumble.c
@@ -105,7 +105,7 @@ JumbleQuery(Query *query, const char *querytext)
Assert(IsQueryIdEnabled());
- if (query->utilityStmt)
+ if (query->utilityStmt && !IsJumbleUtilityAllowed(query->utilityStmt))
{
query->queryId = compute_utility_query_id(querytext,
query->stmt_location,
@@ -241,10 +241,11 @@ static void
JumbleQueryInternal(JumbleState *jstate, Query *query)
{
Assert(IsA(query, Query));
- Assert(query->utilityStmt == NULL);
+ Assert(query->utilityStmt == NULL ||
IsJumbleUtilityAllowed(query->utilityStmt));
APP_JUMB(query->commandType);
/* resultRelation is usually predictable from commandType */
+ JumbleExpr(jstate, (Node *) query->utilityStmt);
JumbleExpr(jstate, (Node *) query->cteList);
JumbleRangeTable(jstate, query->rtable);
JumbleExpr(jstate, (Node *) query->jointree);
@@ -384,6 +385,47 @@ JumbleExpr(JumbleState *jstate, Node *node)
APP_JUMB(var->varlevelsup);
}
break;
+ case T_CallStmt:
+ {
+ CallStmt *stmt = (CallStmt *) node;
+ FuncExpr *expr = stmt->funcexpr;
+
+ APP_JUMB(expr->funcid);
+ JumbleExpr(jstate, (Node *) expr->args);
+ }
+ break;
+ case T_VariableSetStmt:
+ {
+ VariableSetStmt *stmt = (VariableSetStmt *)
node;
+
+ /* stmt->name is NULL for RESET ALL */
+ if (stmt->name)
+ {
+ APP_JUMB(stmt->kind);
+ APP_JUMB_STRING(stmt->name);
+ JumbleExpr(jstate, (Node *) stmt->args);
+ }
+ }
+ break;
+ case T_A_Const:
+ {
+ int loc = ((const A_Const
*) node)->location;
+
+ RecordConstLocation(jstate, loc);
+ }
+ break;
+ case T_TransactionStmt:
+ {
+ TransactionStmt *stmt = (TransactionStmt *)
node;
+
+ Assert(stmt->kind == TRANS_STMT_PREPARE ||
+ stmt->kind ==
TRANS_STMT_COMMIT_PREPARED ||
+ stmt->kind ==
TRANS_STMT_ROLLBACK_PREPARED);
+
+ APP_JUMB(stmt->kind);
+ RecordConstLocation(jstate, stmt->gid_location);
+ }
+ break;
case T_Const:
{
Const *c = (Const *) node;
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index aead2afd6e..f59457dcd6 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -3281,6 +3281,7 @@ typedef struct TransactionStmt
char *savepoint_name; /* for savepoint commands */
char *gid; /* for two-phase-commit related
commands */
bool chain; /* AND CHAIN option */
+ int gid_location; /* gid location */
} TransactionStmt;
/* ----------------------
diff --git a/src/include/utils/queryjumble.h b/src/include/utils/queryjumble.h
index 3c2d9beab2..dfbb060f55 100644
--- a/src/include/utils/queryjumble.h
+++ b/src/include/utils/queryjumble.h
@@ -85,4 +85,24 @@ IsQueryIdEnabled(void)
return query_id_enabled;
}
+/*
+ * Jumble those utility statements
+ */
+static inline bool
+IsJumbleUtilityAllowed(Node *n)
+{
+ if (IsA(n, CallStmt) || IsA(n, VariableSetStmt))
+ return true;
+ else if (IsA(n, TransactionStmt))
+ {
+ TransactionStmt *stmt = (TransactionStmt *) n;
+
+ return (stmt->kind == TRANS_STMT_PREPARE ||
+ stmt->kind == TRANS_STMT_COMMIT_PREPARED ||
+ stmt->kind == TRANS_STMT_ROLLBACK_PREPARED);
+ }
+ else
+ return false;
+}
+
#endif /* QUERYJUMBLE_H */
