On Thu, Sep 12, 2024 at 03:58:27PM -0500, Sami Imseih wrote: > Yes, adding the asserts in execMain.c is better, but there is complications > there due to the issue you mention. I think the issue you are bumping into > is when pg_stat_statements.track_utility = on ( default ), the assert in > ExecutorStart will fail on EXECUTE. I believe it's because ( need to verify ) > pg_stat_statements.c sets the queryId = 0 in the ProcessUtility hook [1].
Yes. > I am now thinking it's better to Just keep the tests in pg_stat_statements. > Having test coverage in pg_stat_statements is better than nothing, and > check-world ( or similar ) will be able to catch such failures. I have begun things by applying a patch to add new tests in pg_stat_statements. It is just something that is useful on its own, and we had nothing of the kind. Then, please see attached two lightly-updated patches. 0001 is for a backpatch down to v14. This is yours to force things in the exec and bind messages for all portal types, with the test (placed elsewhere in 14~15 branches). 0002 is for HEAD to add some sanity checks, blowing up the tests of pg_stat_statements if one is not careful with the query ID reporting. I'm planning to look at that again at the beginning of next week. -- Michael
From 6921a7b6e25c2471d64f05ce136af741598381c0 Mon Sep 17 00:00:00 2001 From: Michael Paquier <mich...@paquier.xyz> Date: Fri, 13 Sep 2024 14:54:17 +0900 Subject: [PATCH v6 1/2] Add missing query ID reporting in extended query protocol This commit adds query ID reports for two code paths for the extended query protocol: - 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: - 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. 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. 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. Author: Sami Imseih Discussion: https://postgr.es/m/ca+427g8diw3az6popvgkpbqk97oubdf18vlihfesea2juk3...@mail.gmail.com Discussion: https://postgr.es/m/CA+TgmoZxtnf_jZ=vqbsyau8hfukkwojcj6ufy4lgpxaunkr...@mail.gmail.com Backpatch-through: 14 --- src/backend/executor/execMain.c | 10 ++++---- src/backend/tcop/postgres.c | 24 +++++++++++++++++++ .../pg_stat_statements/expected/extended.out | 8 +++++++ contrib/pg_stat_statements/sql/extended.sql | 4 ++++ 4 files changed, 42 insertions(+), 4 deletions(-) diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c index 29e186fa73..7042ca6c60 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 8bc6bea113..e394f1419a 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); diff --git a/contrib/pg_stat_statements/expected/extended.out b/contrib/pg_stat_statements/expected/extended.out index bc8cb3f141..04a0594337 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 5ba0678e63..1af0711020 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 -- 2.45.2
From b7f0dd8b23ae6a42ce94ee4c3856f9ff22cc8eb2 Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Fri, 13 Sep 2024 14:56:18 +0900
Subject: [PATCH v6 2/2] Add sanity checks in executor for query ID
This causes the test added by the previous commit to blow up on sight,
as would a removal of the query ID reporting in ExecuteStart() blow up
when running the tests of pg_stat_statements.
No backpatck done here.
---
src/backend/executor/execMain.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index 7042ca6c60..77b8d4762d 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -50,6 +50,7 @@
#include "foreign/fdwapi.h"
#include "mb/pg_wchar.h"
#include "miscadmin.h"
+#include "nodes/queryjumble.h"
#include "parser/parse_relation.h"
#include "rewrite/rewriteHandler.h"
#include "tcop/utility.h"
@@ -295,6 +296,9 @@ ExecutorRun(QueryDesc *queryDesc,
ScanDirection direction, uint64 count,
bool execute_once)
{
+ /* If enabled, the query ID should be set. */
+ Assert(!IsQueryIdEnabled() || pgstat_get_my_query_id() != 0);
+
if (ExecutorRun_hook)
(*ExecutorRun_hook) (queryDesc, direction, count, execute_once);
else
@@ -403,6 +407,9 @@ standard_ExecutorRun(QueryDesc *queryDesc,
void
ExecutorFinish(QueryDesc *queryDesc)
{
+ /* If enabled, the query ID should be set. */
+ Assert(!IsQueryIdEnabled() || pgstat_get_my_query_id() != 0);
+
if (ExecutorFinish_hook)
(*ExecutorFinish_hook) (queryDesc);
else
@@ -418,6 +425,9 @@ standard_ExecutorFinish(QueryDesc *queryDesc)
/* sanity checks */
Assert(queryDesc != NULL);
+ /* If enabled, the query ID should be set. */
+ Assert(!IsQueryIdEnabled() || pgstat_get_my_query_id() != 0);
+
estate = queryDesc->estate;
Assert(estate != NULL);
--
2.45.2
signature.asc
Description: PGP signature
