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

diff --git a/contrib/pg_stat_statements/Makefile b/contrib/pg_stat_statements/Makefile
index 414a30856e46f71aa8d5a4206cbc014ae1690d85..72c7994bb3cdc759d60e55ee332fce7267451b44 100644 (file)
--- a/contrib/pg_stat_statements/Makefile
+++ b/contrib/pg_stat_statements/Makefile
@@ -19,7 +19,8 @@ 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 cleanup oldextversions
+       user_activity wal entry_timestamp 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).
 NO_INSTALLCHECK = 1

diff --git a/contrib/pg_stat_statements/expected/extended.out b/contrib/pg_stat_statements/expected/extended.out
new file mode 100644 (file)
index 0000000..dbc7868
--- /dev/null
+++ b/contrib/pg_stat_statements/expected/extended.out
@@ -0,0 +1,10 @@
+-- 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)
+

diff --git a/contrib/pg_stat_statements/meson.build b/contrib/pg_stat_statements/meson.build
index 9bfc9657e1adae6faa47d581e1837358cbcb2f7b..a99468e870baf3d04627d64bf0819358559b88d0 100644 (file)
--- a/contrib/pg_stat_statements/meson.build
+++ b/contrib/pg_stat_statements/meson.build
@@ -50,6 +50,7 @@ tests += {
       'user_activity',
       'wal',
       'entry_timestamp',
+      'extended',
       'cleanup',
       'oldextversions',
     ],

diff --git a/contrib/pg_stat_statements/sql/extended.sql b/contrib/pg_stat_statements/sql/extended.sql
new file mode 100644 (file)
index 0000000..07b6c5a
--- /dev/null
+++ b/contrib/pg_stat_statements/sql/extended.sql
@@ -0,0 +1,7 @@
+-- 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

diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index 4d7c92d63c19800977cd2567c2e494c79edbcd3e..2365c6861be147a93f654fc794f4a3428c9cae76 100644 (file)
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -124,10 +124,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 7fe14c5402c51f9489dcfb20dbb33fedfb27bffb..a750dc800b6afcf4d1682723a545d3d8cfd08954 100644 (file)
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -1648,6 +1648,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);
@@ -1683,6 +1684,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)
@@ -2105,6 +2117,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;
@@ -2151,6 +2164,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);