Rearm statement_timeout after each executed query.

Previously statement_timeout, in the extended protocol, affected all
messages till a Sync message.  For clients that pipeline/batch query
execution that's problematic.

Instead disable timeout after each Execute message, and enable, if
necessary, the timer in start_xact_command(). As that's done only for
Execute and not Parse / Bind, pipelining the latter two could still
cause undesirable timeouts. But a survey of protocol implementations
shows that all drivers issue Sync messages when preparing, and adding
timeout rearming to both is fairly expensive for the common parse /
bind / execute sequence.

Author: Tatsuo Ishii, editorialized by Andres Freund
Reviewed-By: Takayuki Tsunakawa, Andres Freund
Discussion: https://postgr.es/m/20170222.115044.1665674502985097185.t-ishii@sraoss.co.jp

diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index dfd52b3c87eb0b2bfcf0f5e2a31538d1b4d47d63..c807b00b0bef402f47bcf1bf192328b33dd57db7 100644 (file)
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -143,6 +143,11 @@ static bool DoingCommandRead = false;
 static bool doing_extended_query_message = false;
 static bool ignore_till_sync = false;
 
+/*
+ * Flag to keep track of whether statement timeout timer is active.
+ */
+static bool stmt_timeout_active = false;
+
 /*
  * If an unnamed prepared statement exists, it's stored here.
  * We keep it separate from the hashtable kept by commands/prepare.c
@@ -182,6 +187,8 @@ static bool IsTransactionExitStmtList(List *pstmts);
 static bool IsTransactionStmtList(List *pstmts);
 static void drop_unnamed_stmt(void);
 static void log_disconnections(int code, Datum arg);
+static void enable_statement_timeout(void);
+static void disable_statement_timeout(void);
 
 
 /* ----------------------------------------------------------------
@@ -1241,7 +1248,8 @@ exec_parse_message(const char *query_string,      /* string to execute */
        /*
         * Start up a transaction command so we can run parse analysis etc. (Note
         * that this will normally change current memory context.) Nothing happens
-        * if we are already in one.
+        * if we are already in one.  This also arms the statement timeout if
+        * necessary.
         */
        start_xact_command();
 
@@ -1529,7 +1537,8 @@ exec_bind_message(StringInfo input_message)
        /*
         * Start up a transaction command so we can call functions etc. (Note that
         * this will normally change current memory context.) Nothing happens if
-        * we are already in one.
+        * we are already in one.  This also arms the statement timeout if
+        * necessary.
         */
        start_xact_command();
 
@@ -2021,6 +2030,9 @@ exec_execute_message(const char *portal_name, long max_rows)
                         * those that start or end a transaction block.
                         */
                        CommandCounterIncrement();
+
+                       /* full command has been executed, reset timeout */
+                       disable_statement_timeout();
                }
 
                /* Send appropriate CommandComplete to client */
@@ -2450,25 +2462,27 @@ start_xact_command(void)
        {
                StartTransactionCommand();
 
-               /* Set statement timeout running, if any */
-               /* NB: this mustn't be enabled until we are within an xact */
-               if (StatementTimeout > 0)
-                       enable_timeout_after(STATEMENT_TIMEOUT, StatementTimeout);
-               else
-                       disable_timeout(STATEMENT_TIMEOUT, false);
-
                xact_started = true;
        }
+
+       /*
+        * Start statement timeout if necessary.  Note that this'll intentionally
+        * not reset the clock on an already started timeout, to avoid the timing
+        * overhead when start_xact_command() is invoked repeatedly, without an
+        * interceding finish_xact_command() (e.g. parse/bind/execute).  If that's
+        * not desired, the timeout has to be disabled explicitly.
+        */
+       enable_statement_timeout();
 }
 
 static void
 finish_xact_command(void)
 {
+       /* cancel active statement timeout after each command */
+       disable_statement_timeout();
+
        if (xact_started)
        {
-               /* Cancel any active statement timeout before committing */
-               disable_timeout(STATEMENT_TIMEOUT, false);
-
                CommitTransactionCommand();
 
 #ifdef MEMORY_CONTEXT_CHECKING
@@ -4537,3 +4551,42 @@ log_disconnections(int code, Datum arg)
                                        port->user_name, port->database_name, port->remote_host,
                                        port->remote_port[0] ? " port=" : "", port->remote_port)));
 }
+
+/*
+ * Start statement timeout timer, if enabled.
+ *
+ * If there's already a timeout running, don't restart the timer.  That
+ * enables compromises between accuracy of timeouts and cost of starting a
+ * timeout.
+ */
+static void
+enable_statement_timeout(void)
+{
+       /* must be within an xact */
+       Assert(xact_started);
+
+       if (StatementTimeout > 0)
+       {
+               if (!stmt_timeout_active)
+               {
+                       enable_timeout_after(STATEMENT_TIMEOUT, StatementTimeout);
+                       stmt_timeout_active = true;
+               }
+       }
+       else
+               disable_timeout(STATEMENT_TIMEOUT, false);
+}
+
+/*
+ * Disable statement timeout, if active.
+ */
+static void
+disable_statement_timeout(void)
+{
+       if (stmt_timeout_active)
+       {
+               disable_timeout(STATEMENT_TIMEOUT, false);
+
+               stmt_timeout_active = false;
+       }
+}