From 8408f9929af5cf35bf2dc888777782e476160c4b Mon Sep 17 00:00:00 2001 From: Michael Paquier Date: Wed, 11 Sep 2024 14:15:03 +0900 Subject: [PATCH 2/3] Add sanity checks related to query ID reporting in pg_stat_statements This includes as well tests with the extended query protocol, checking that the query ID is correctly set when going through the executor hooks of the module. These are new tests, so no backpatch is done. --- contrib/pg_stat_statements/Makefile | 2 +- .../pg_stat_statements/expected/extended.out | 62 +++++++++++++++++++ contrib/pg_stat_statements/meson.build | 1 + .../pg_stat_statements/pg_stat_statements.c | 17 +++++ contrib/pg_stat_statements/sql/extended.sql | 17 +++++ 5 files changed, 98 insertions(+), 1 deletion(-) create mode 100644 contrib/pg_stat_statements/expected/extended.out create mode 100644 contrib/pg_stat_statements/sql/extended.sql diff --git a/contrib/pg_stat_statements/Makefile b/contrib/pg_stat_statements/Makefile index c19ccad77e..1622b43ded 100644 --- a/contrib/pg_stat_statements/Makefile +++ b/contrib/pg_stat_statements/Makefile @@ -19,7 +19,7 @@ LDFLAGS_SL += $(filter -lm, $(LIBS)) REGRESS_OPTS = --temp-config $(top_srcdir)/contrib/pg_stat_statements/pg_stat_statements.conf REGRESS = select dml cursors utility level_tracking planning \ - user_activity wal entry_timestamp privileges cleanup \ + user_activity wal entry_timestamp privileges extended cleanup \ oldextversions # Disabled because these tests require "shared_preload_libraries=pg_stat_statements", # which typical installcheck users do not have (e.g. buildfarm clients). diff --git a/contrib/pg_stat_statements/expected/extended.out b/contrib/pg_stat_statements/expected/extended.out new file mode 100644 index 0000000000..09c1f895e5 --- /dev/null +++ b/contrib/pg_stat_statements/expected/extended.out @@ -0,0 +1,62 @@ +-- Tests for extended query protocol +SELECT query_id IS NOT NULL AS query_id_set + FROM pg_stat_activity WHERE pid = pg_backend_pid() \bind \g + query_id_set +-------------- + t +(1 row) + +SELECT $1 \parse stmt1 +SELECT $1, $2 \parse stmt2 +SELECT $1, $2, $3 \parse stmt3 +SELECT pg_stat_statements_reset() IS NOT NULL AS t; + t +--- + t +(1 row) + +\bind_named stmt1 'stmt1_val1' \g + ?column? +------------ + stmt1_val1 +(1 row) + +\bind_named stmt2 'stmt2_val1' 'stmt2_val2' \g + ?column? | ?column? +------------+------------ + stmt2_val1 | stmt2_val2 +(1 row) + +\bind_named stmt3 'stmt3_val1' 'stmt3_val2' 'stmt3_val3' \g + ?column? | ?column? | ?column? +------------+------------+------------ + stmt3_val1 | stmt3_val2 | stmt3_val3 +(1 row) + +\bind_named stmt3 'stmt3_val4' 'stmt3_val5' 'stmt3_val6' \g + ?column? | ?column? | ?column? +------------+------------+------------ + stmt3_val4 | stmt3_val5 | stmt3_val6 +(1 row) + +\bind_named stmt2 'stmt2_val3' 'stmt2_val4' \g + ?column? | ?column? +------------+------------ + stmt2_val3 | stmt2_val4 +(1 row) + +\bind_named stmt1 'stmt1_val1' \g + ?column? +------------ + stmt1_val1 +(1 row) + +SELECT calls, rows, query FROM pg_stat_statements ORDER BY query COLLATE "C"; + calls | rows | query +-------+------+---------------------------------------------------- + 2 | 2 | SELECT $1 + 2 | 2 | SELECT $1, $2 + 2 | 2 | SELECT $1, $2, $3 + 1 | 1 | SELECT pg_stat_statements_reset() IS NOT NULL AS t +(4 rows) + diff --git a/contrib/pg_stat_statements/meson.build b/contrib/pg_stat_statements/meson.build index 5cf926d1f8..e14669ca15 100644 --- a/contrib/pg_stat_statements/meson.build +++ b/contrib/pg_stat_statements/meson.build @@ -51,6 +51,7 @@ tests += { 'wal', 'entry_timestamp', 'privileges', + 'extended', 'cleanup', 'oldextversions', ], diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c index 362d222f63..1ae31b82a8 100644 --- a/contrib/pg_stat_statements/pg_stat_statements.c +++ b/contrib/pg_stat_statements/pg_stat_statements.c @@ -834,6 +834,14 @@ pgss_post_parse_analyze(ParseState *pstate, Query *query, JumbleState *jstate) if (!pgss || !pgss_hash || !pgss_enabled(nesting_level)) return; + /* + * Query ID should be set in the query. + * + * Note that the query ID may be already reported (like in a PREPARE + * statement, hence there is no check based on pgstat_get_my_query_id). + */ + Assert(query->queryId != 0); + /* * If it's EXECUTE, clear the queryId so that stats will accumulate for * the underlying PREPARE. But don't do this if we're not tracking @@ -1022,6 +1030,9 @@ static void pgss_ExecutorRun(QueryDesc *queryDesc, ScanDirection direction, uint64 count, bool execute_once) { + /* Query ID should be reported. */ + Assert(pgstat_get_my_query_id() != 0); + nesting_level++; PG_TRY(); { @@ -1043,6 +1054,9 @@ pgss_ExecutorRun(QueryDesc *queryDesc, ScanDirection direction, uint64 count, static void pgss_ExecutorFinish(QueryDesc *queryDesc) { + /* Query ID should be reported. */ + Assert(pgstat_get_my_query_id() != 0); + nesting_level++; PG_TRY(); { @@ -1066,6 +1080,9 @@ pgss_ExecutorEnd(QueryDesc *queryDesc) { uint64 queryId = queryDesc->plannedstmt->queryId; + /* Query ID should be reported. */ + Assert(pgstat_get_my_query_id() != 0); + if (queryId != UINT64CONST(0) && queryDesc->totaltime && pgss_enabled(nesting_level)) { diff --git a/contrib/pg_stat_statements/sql/extended.sql b/contrib/pg_stat_statements/sql/extended.sql new file mode 100644 index 0000000000..c039c5de01 --- /dev/null +++ b/contrib/pg_stat_statements/sql/extended.sql @@ -0,0 +1,17 @@ +-- Tests for extended query protocol + +SELECT query_id IS NOT NULL AS query_id_set + FROM pg_stat_activity WHERE pid = pg_backend_pid() \bind \g + +SELECT $1 \parse stmt1 +SELECT $1, $2 \parse stmt2 +SELECT $1, $2, $3 \parse stmt3 +SELECT pg_stat_statements_reset() IS NOT NULL AS t; +\bind_named stmt1 'stmt1_val1' \g +\bind_named stmt2 'stmt2_val1' 'stmt2_val2' \g +\bind_named stmt3 'stmt3_val1' 'stmt3_val2' 'stmt3_val3' \g +\bind_named stmt3 'stmt3_val4' 'stmt3_val5' 'stmt3_val6' \g +\bind_named stmt2 'stmt2_val3' 'stmt2_val4' \g +\bind_named stmt1 'stmt1_val1' \g + +SELECT calls, rows, query FROM pg_stat_statements ORDER BY query COLLATE "C"; -- 2.45.2