From ccfb60ccc87ef070b73a28d5528447cf8acb8808 Mon Sep 17 00:00:00 2001 From: Zsolt Parragi Date: Tue, 3 Jun 2025 22:08:01 +0100 Subject: [PATCH] PG-1621: fix cmd_type mostly showing 0 values This was actually caused by two bugs internally: * cmd_type was only set in some codepaths, other parts of the code never set a value. Depending on which query / how was executed, it was possibly never changed (after a reset to 0) * the update first set the cmd_type, then reset all counters. As the cmd_type is stored within the counters for some reason, this reset its value to 0 in most execution paths, even if it was corretly set before. And according to this the fix is simple: * cmd_type is now set in all codepaths except for failing queries, as we only have the error string in this case, without the type. * in the update logic, we again overwrite cmd_type with the proper value after a reset --- Makefile | 2 +- meson.build | 1 + pg_stat_monitor--2.1--2.2.sql | 21 +++++++++++++++++++++ pg_stat_monitor.c | 20 ++++++++++++++------ pg_stat_monitor.control | 2 +- regression/expected/cmd_type.out | 10 +++++----- regression/expected/cmd_type_1.out | 8 +++++++- regression/expected/version.out | 2 +- 8 files changed, 51 insertions(+), 15 deletions(-) create mode 100644 pg_stat_monitor--2.1--2.2.sql diff --git a/Makefile b/Makefile index de2682e..e93283b 100644 --- a/Makefile +++ b/Makefile @@ -4,7 +4,7 @@ MODULE_big = pg_stat_monitor OBJS = hash_query.o guc.o pg_stat_monitor.o $(WIN32RES) EXTENSION = pg_stat_monitor -DATA = pg_stat_monitor--2.0.sql pg_stat_monitor--1.0--2.0.sql pg_stat_monitor--2.0--2.1.sql +DATA = pg_stat_monitor--2.0.sql pg_stat_monitor--1.0--2.0.sql pg_stat_monitor--2.0--2.1.sql pg_stat_monitor--2.1--2.2.sql PGFILEDESC = "pg_stat_monitor - execution statistics of SQL statements" diff --git a/meson.build b/meson.build index 104ecd5..639883f 100644 --- a/meson.build +++ b/meson.build @@ -17,6 +17,7 @@ install_data( 'pg_stat_monitor--2.0.sql', 'pg_stat_monitor--1.0--2.0.sql', 'pg_stat_monitor--2.0--2.1.sql', + 'pg_stat_monitor--2.1--2.2.sql', kwargs: contrib_data_args, ) diff --git a/pg_stat_monitor--2.1--2.2.sql b/pg_stat_monitor--2.1--2.2.sql new file mode 100644 index 0000000..837dbbf --- /dev/null +++ b/pg_stat_monitor--2.1--2.2.sql @@ -0,0 +1,21 @@ +/* contrib/pg_stat_monitor/pg_stat_monitor--2.1--2.2.sql */ + +-- complain if script is sourced in psql, rather than via CREATE EXTENSION +\echo Use "ALTER EXTENSION pg_stat_monitor" to load this file. \quit + +CREATE OR REPLACE FUNCTION get_cmd_type (cmd_type INTEGER) RETURNS TEXT AS +$$ +SELECT + CASE + WHEN cmd_type = 1 THEN 'SELECT' + WHEN cmd_type = 2 THEN 'UPDATE' + WHEN cmd_type = 3 THEN 'INSERT' + WHEN cmd_type = 4 THEN 'DELETE' + WHEN cmd_type = 5 AND current_setting('server_version_num')::int >= 150000 THEN 'MERGE' + WHEN cmd_type = 5 AND current_setting('server_version_num')::int < 150000 THEN 'UTILITY' + WHEN cmd_type = 6 AND current_setting('server_version_num')::int >= 150000 THEN 'UTILITY' + WHEN cmd_type = 6 AND current_setting('server_version_num')::int < 150000 THEN 'NOTHING' + WHEN cmd_type = 7 THEN 'NOTHING' + END +$$ +LANGUAGE SQL PARALLEL SAFE; diff --git a/pg_stat_monitor.c b/pg_stat_monitor.c index 5e9c77c..75ca49e 100644 --- a/pg_stat_monitor.c +++ b/pg_stat_monitor.c @@ -37,7 +37,7 @@ typedef enum pgsmVersion PG_MODULE_MAGIC; -#define BUILD_VERSION "2.1.1" +#define BUILD_VERSION "2.2.0" /* Number of output arguments (columns) for various API versions */ #define PG_STAT_MONITOR_COLS_V1_0 52 @@ -203,7 +203,7 @@ char *unpack_sql_state(int sql_state); static pgsmEntry *pgsm_create_hash_entry(uint64 bucket_id, uint64 queryid, PlanInfo *plan_info); static void pgsm_add_to_list(pgsmEntry *entry, char *query_text, int query_len); -static pgsmEntry *pgsm_get_entry_for_query(uint64 queryid, PlanInfo *plan_info, const char *query_text, int query_len, bool create); +static pgsmEntry *pgsm_get_entry_for_query(uint64 queryid, PlanInfo *plan_info, const char *query_text, int query_len, bool create, CmdType cmd_type); static uint64 get_pgsm_query_id_hash(const char *norm_query, int len); static void pgsm_cleanup_callback(void *arg); @@ -736,7 +736,7 @@ pgsm_ExecutorEnd(QueryDesc *queryDesc) if (queryId != UINT64CONST(0) && queryDesc->totaltime && pgsm_enabled(nesting_level)) { - entry = pgsm_get_entry_for_query(queryId, plan_ptr, (char *) queryDesc->sourceText, strlen(queryDesc->sourceText), true); + entry = pgsm_get_entry_for_query(queryId, plan_ptr, (char *) queryDesc->sourceText, strlen(queryDesc->sourceText), true, queryDesc->operation); if (!entry) { elog(DEBUG2, "[pg_stat_monitor] pgsm_ExecutorEnd: Failed to find entry for [%lu] %s.", queryId, queryDesc->sourceText); @@ -763,6 +763,8 @@ pgsm_ExecutorEnd(QueryDesc *queryDesc) sys_info.stime = time_diff(rusage_end.ru_stime, rusage_start.ru_stime); } + entry->counters.info.cmd_type = queryDesc->operation; + pgsm_update_entry(entry, /* entry */ NULL, /* query */ NULL, /* comments */ @@ -908,7 +910,7 @@ pgsm_planner_hook(Query *parse, const char *query_string, int cursorOptions, Par INSTR_TIME_SET_CURRENT(start); if (MemoryContextIsValid(MessageContext)) - entry = pgsm_get_entry_for_query(parse->queryId, NULL, query_string, strlen(query_string), true); + entry = pgsm_get_entry_for_query(parse->queryId, NULL, query_string, strlen(query_string), true, parse->commandType); #if PG_VERSION_NUM >= 170000 nesting_level++; @@ -1199,7 +1201,7 @@ pgsm_ProcessUtility(PlannedStmt *pstmt, const char *queryString, query_text = (char *) CleanQuerytext(queryString, &location, &query_len); entry->pgsm_query_id = get_pgsm_query_id_hash(query_text, query_len); - entry->counters.info.cmd_type = 0; + entry->counters.info.cmd_type = pstmt->commandType; pgsm_add_to_list(entry, query_text, query_len); @@ -1741,7 +1743,7 @@ pgsm_add_to_list(pgsmEntry *entry, char *query_text, int query_len) } static pgsmEntry * -pgsm_get_entry_for_query(uint64 queryid, PlanInfo *plan_info, const char *query_text, int query_len, bool create) +pgsm_get_entry_for_query(uint64 queryid, PlanInfo *plan_info, const char *query_text, int query_len, bool create, CmdType cmd_type) { pgsmEntry *entry = NULL; ListCell *lc = NULL; @@ -1778,6 +1780,7 @@ pgsm_get_entry_for_query(uint64 queryid, PlanInfo *plan_info, const char *query_ * worry about these. */ entry->pgsm_query_id = get_pgsm_query_id_hash(query_text, query_len); + entry->counters.info.cmd_type = cmd_type; pgsm_add_to_list(entry, (char *) query_text, query_len); } @@ -2087,6 +2090,11 @@ pgsm_store(pgsmEntry *entry) reset, /* reset */ PGSM_STORE); + if (reset) + { + shared_hash_entry->counters.info.cmd_type = entry->counters.info.cmd_type; + } + memset(&entry->counters, 0, sizeof(entry->counters)); pgsm_lock_release(pgsm); } diff --git a/pg_stat_monitor.control b/pg_stat_monitor.control index b52a4de..b1fe5b1 100644 --- a/pg_stat_monitor.control +++ b/pg_stat_monitor.control @@ -1,5 +1,5 @@ # pg_stat_monitor extension comment = 'The pg_stat_monitor is a PostgreSQL Query Performance Monitoring tool, based on PostgreSQL contrib module pg_stat_statements. pg_stat_monitor provides aggregated statistics, client information, plan details including plan, and histogram information.' -default_version = '2.1' +default_version = '2.2' module_pathname = '$libdir/pg_stat_monitor' relocatable = true diff --git a/regression/expected/cmd_type.out b/regression/expected/cmd_type.out index b006af1..28b1b11 100644 --- a/regression/expected/cmd_type.out +++ b/regression/expected/cmd_type.out @@ -27,16 +27,16 @@ DROP TABLE t2; SELECT query, cmd_type, cmd_type_text FROM pg_stat_monitor ORDER BY query COLLATE "C"; query | cmd_type | cmd_type_text --------------------------------+----------+--------------- - CREATE TABLE t1 (a INTEGER) | 0 | - CREATE TABLE t2 (b INTEGER) | 0 | + CREATE TABLE t1 (a INTEGER) | 5 | UTILITY + CREATE TABLE t2 (b INTEGER) | 5 | UTILITY DELETE FROM t1 | 4 | DELETE - DROP TABLE t1 | 0 | - DROP TABLE t2 | 0 | + DROP TABLE t1 | 5 | UTILITY + DROP TABLE t2 | 5 | UTILITY INSERT INTO t1 VALUES(1) | 3 | INSERT SELECT a FROM t1 | 1 | SELECT SELECT b FROM t2 FOR UPDATE | 1 | SELECT SELECT pg_stat_monitor_reset() | 1 | SELECT - TRUNCATE t1 | 0 | + TRUNCATE t1 | 5 | UTILITY UPDATE t1 SET a = 2 | 2 | UPDATE (11 rows) diff --git a/regression/expected/cmd_type_1.out b/regression/expected/cmd_type_1.out index b5b4c80..ccd6440 100644 --- a/regression/expected/cmd_type_1.out +++ b/regression/expected/cmd_type_1.out @@ -23,16 +23,22 @@ SELECT b FROM t2 FOR UPDATE; TRUNCATE t1; DROP TABLE t1; +DROP TABLE t2; SELECT query, cmd_type, cmd_type_text FROM pg_stat_monitor ORDER BY query COLLATE "C"; query | cmd_type | cmd_type_text --------------------------------+----------+--------------- + CREATE TABLE t1 (a INTEGER) | 6 | UTILITY + CREATE TABLE t2 (b INTEGER) | 6 | UTILITY DELETE FROM t1 | 4 | DELETE + DROP TABLE t1 | 6 | UTILITY + DROP TABLE t2 | 6 | UTILITY INSERT INTO t1 VALUES(1) | 3 | INSERT SELECT a FROM t1 | 1 | SELECT SELECT b FROM t2 FOR UPDATE | 1 | SELECT SELECT pg_stat_monitor_reset() | 1 | SELECT + TRUNCATE t1 | 6 | UTILITY UPDATE t1 SET a = 2 | 2 | UPDATE -(6 rows) +(11 rows) SELECT pg_stat_monitor_reset(); pg_stat_monitor_reset diff --git a/regression/expected/version.out b/regression/expected/version.out index 208800b..e0636a6 100644 --- a/regression/expected/version.out +++ b/regression/expected/version.out @@ -2,7 +2,7 @@ CREATE EXTENSION pg_stat_monitor; SELECT pg_stat_monitor_version(); pg_stat_monitor_version ------------------------- - 2.1.1 + 2.2.0 (1 row) DROP EXTENSION pg_stat_monitor;