From 933848d16dc9d9d838c1bc15ed7e48c2f2eed69c Mon Sep 17 00:00:00 2001 From: Michael Paquier Date: Wed, 18 Sep 2024 09:59:09 +0900 Subject: [PATCH] Add missing query ID reporting in extended query protocol This commit adds query ID reports for two code paths when processing extended query protocol messages: - When receiving a bind message, setting it to the first Query retrieved from a cached cache. - When receiving an execute message, setting it to the first PlannedStmt stored in a portal. An advantage of this method is that this is able to cover all the types of portals handled in the extended query protocol, particularly these two when the report done in ExecutorStart() is not enough (neither is an addition in ExecutorRun(), actually, for the second point): - Multiple execute messages, with multiple ExecutorRun(). - Portal with execute/fetch messages, like a query with a RETURNING clause and a fetch size that stores the tuples in a first execute message going though ExecutorStart() and ExecuteRun(), followed by one or more execute messages doing only fetches from the tuplestore created in the first message. This corresponds to the case where execute_is_fetch is set, for example. Note that the query ID reporting done in ExecutorStart() is still necessary, as an EXECUTE requires it. Query ID reporting is optimistic and more calls to pgstat_report_query_id() don't matter as the first report takes priority except if the report is forced. The comment in ExecutorStart() is adjusted to reflect better the reality with the extended query protocol. The test added in pg_stat_statements is a courtesy of Robert Haas. This uses psql's \bind metacommand, hence this part is backpatched down to v16. Reported-by: Kaido Vaikla, Erik Wienhold Author: Sami Imseih Reviewed-by: Jian He, Andrei Lepikhov, Michael Paquier Discussion: https://postgr.es/m/CA+427g8DiW3aZ6pOpVgkPbqK97ouBdf18VLiHFesea2jUk3XoQ@mail.gmail.com Discussion: https://postgr.es/m/CA+TgmoZxtnf_jZ=VqBSyaU8hfUkkwoJCJ6ufy4LGpXaunKrjrg@mail.gmail.com Discussion: https://postgr.es/m/1391613709.939460.1684777418070@office.mailbox.org Backpatch-through: 14 --- .../pg_stat_statements/expected/extended.out | 8 +++++++ contrib/pg_stat_statements/sql/extended.sql | 4 ++++ src/backend/executor/execMain.c | 10 ++++---- src/backend/tcop/postgres.c | 24 +++++++++++++++++++ 4 files changed, 42 insertions(+), 4 deletions(-) diff --git a/contrib/pg_stat_statements/expected/extended.out b/contrib/pg_stat_statements/expected/extended.out index bc8cb3f1416..04a05943372 100644 --- a/contrib/pg_stat_statements/expected/extended.out +++ b/contrib/pg_stat_statements/expected/extended.out @@ -1,5 +1,13 @@ -- Tests with extended query protocol SET pg_stat_statements.track_utility = FALSE; +-- This test checks that an execute message sets a query ID. +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 pg_stat_statements_reset() IS NOT NULL AS t; t --- diff --git a/contrib/pg_stat_statements/sql/extended.sql b/contrib/pg_stat_statements/sql/extended.sql index 5ba0678e636..1af0711020c 100644 --- a/contrib/pg_stat_statements/sql/extended.sql +++ b/contrib/pg_stat_statements/sql/extended.sql @@ -2,6 +2,10 @@ SET pg_stat_statements.track_utility = FALSE; +-- This test checks that an execute message sets a query ID. +SELECT query_id IS NOT NULL AS query_id_set + FROM pg_stat_activity WHERE pid = pg_backend_pid() \bind \g + SELECT pg_stat_statements_reset() IS NOT NULL AS t; SELECT $1 \parse stmt1 SELECT $1, $2 \parse stmt2 diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c index 29e186fa73d..7042ca6c600 100644 --- a/src/backend/executor/execMain.c +++ b/src/backend/executor/execMain.c @@ -119,10 +119,12 @@ void ExecutorStart(QueryDesc *queryDesc, int eflags) { /* - * In some cases (e.g. an EXECUTE statement) a query execution will skip - * parse analysis, which means that the query_id won't be reported. Note - * that it's harmless to report the query_id multiple times, as the call - * will be ignored if the top level query_id has already been reported. + * In some cases (e.g. an EXECUTE statement or an execute message with the + * extended query protocol) the query_id won't be reported, so do it now. + * + * Note that it's harmless to report the query_id multiple times, as the + * call will be ignored if the top level query_id has already been + * reported. */ pgstat_report_query_id(queryDesc->plannedstmt->queryId, false); diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c index 8bc6bea1135..e394f1419ad 100644 --- a/src/backend/tcop/postgres.c +++ b/src/backend/tcop/postgres.c @@ -1654,6 +1654,7 @@ exec_bind_message(StringInfo input_message) char msec_str[32]; ParamsErrorCbData params_data; ErrorContextCallback params_errcxt; + ListCell *lc; /* Get the fixed part of the message */ portal_name = pq_getmsgstring(input_message); @@ -1689,6 +1690,17 @@ exec_bind_message(StringInfo input_message) pgstat_report_activity(STATE_RUNNING, psrc->query_string); + foreach(lc, psrc->query_list) + { + Query *query = lfirst_node(Query, lc); + + if (query->queryId != UINT64CONST(0)) + { + pgstat_report_query_id(query->queryId, false); + break; + } + } + set_ps_display("BIND"); if (save_log_statement_stats) @@ -2111,6 +2123,7 @@ exec_execute_message(const char *portal_name, long max_rows) ErrorContextCallback params_errcxt; const char *cmdtagname; size_t cmdtaglen; + ListCell *lc; /* Adjust destination to tell printtup.c what to do */ dest = whereToSendOutput; @@ -2157,6 +2170,17 @@ exec_execute_message(const char *portal_name, long max_rows) pgstat_report_activity(STATE_RUNNING, sourceText); + foreach(lc, portal->stmts) + { + PlannedStmt *stmt = lfirst_node(PlannedStmt, lc); + + if (stmt->queryId != UINT64CONST(0)) + { + pgstat_report_query_id(stmt->queryId, false); + break; + } + } + cmdtagname = GetCommandTagNameAndLen(portal->commandTag, &cmdtaglen); set_ps_display_with_len(cmdtagname, cmdtaglen); -- 2.30.2