Re: Statement timeout behavior in extended queries - Mailing list pgsql-hackers

Andres,

>> I think the code needs a few clarifying comments around this, but
>> otherwise seems good.  Not restarting the timeout in those cases
>> obviously isn't entirely "perfect"/"correct", but a tradeoff - the
>> comments should note that.
>> 
>> Tatsuo-san, do you want to change those, and push?  I can otherwise.
> 
> Andres,
> 
> If you don't mind, could you please fix the comments and push it.

I have changed the comments as you suggested. If it's ok, I can push
the patch myself (today I have time to work on this).

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index a2282058..1fd93359 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -149,6 +149,11 @@ static bool doing_extended_query_message = false;static bool ignore_till_sync = false;/*
+ * Flag to keep track of whether we have started statement timeout timer.
+ */
+static bool stmt_timer_started = false;
+
+/* * If an unnamed prepared statement exists, it's stored here. * We keep it separate from the hashtable kept by
commands/prepare.c* in order to reduce overhead for short-lived queries.
 
@@ -188,6 +193,8 @@ static bool IsTransactionStmtList(List *pstmts);static void drop_unnamed_stmt(void);static void
SigHupHandler(SIGNAL_ARGS);staticvoid log_disconnections(int code, Datum arg);
 
+static void enable_statement_timeout(void);
+static void disable_statement_timeout(void);/* ----------------------------------------------------------------
@@ -1239,6 +1246,15 @@ exec_parse_message(const char *query_string,    /* string to execute */    start_xact_command();
  /*
 
+     * Set statement timeout running if it's not already set. The timer will
+     * be reset in a subsequent execute message. Ideally the timer should be
+     * reset in this function so that each parse/bind/execute message catches
+     * the timeout, but it needs gettimeofday() call for each, which is too
+     * expensive.
+     */
+    enable_statement_timeout();
+
+    /*     * Switch to appropriate context for constructing parsetrees.     *     * We have two strategies depending
onwhether the prepared statement is
 
@@ -1526,6 +1542,15 @@ exec_bind_message(StringInfo input_message)     */    start_xact_command();
+    /*
+     * Set statement timeout running if it's not already set. The timer will
+     * be reset in a subsequent execute message. Ideally the timer should be
+     * reset in this function so that each parse/bind/execute message catches
+     * the timeout, but it needs gettimeofday() call for each, which is too
+     * expensive.
+     */
+    enable_statement_timeout();
+    /* Switch back to message context */    MemoryContextSwitchTo(MessageContext);
@@ -1942,6 +1967,11 @@ exec_execute_message(const char *portal_name, long max_rows)    start_xact_command();    /*
+     * Set statement timeout running if it's not already set.
+     */
+    enable_statement_timeout();
+
+    /*     * If we re-issue an Execute protocol request against an existing portal,     * then we are only fetching
morerows rather than completely re-executing     * the query from the start. atStart is never reset for a v3 portal, so
we
@@ -2014,6 +2044,11 @@ exec_execute_message(const char *portal_name, long max_rows)             * those that start or
enda transaction block.             */            CommandCounterIncrement();
 
+
+            /*
+             * We need to reset statement timeout if already set.
+             */
+            disable_statement_timeout();        }        /* Send appropriate CommandComplete to client */
@@ -2443,14 +2478,10 @@ 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;
+
+        /* Set statement timeout running, if any */
+        enable_statement_timeout();    }}
@@ -2460,7 +2491,7 @@ finish_xact_command(void)    if (xact_started)    {        /* Cancel any active statement timeout
beforecommitting */
 
-        disable_timeout(STATEMENT_TIMEOUT, false);
+        disable_statement_timeout();        CommitTransactionCommand();
@@ -4511,3 +4542,53 @@ log_disconnections(int code, Datum arg)                    port->user_name, port->database_name,
port->remote_host,                 port->remote_port[0] ? " port=" : "", port->remote_port)));}
 
+
+/*
+ * Set statement timeout if any.
+ */
+static void
+enable_statement_timeout(void)
+{
+    if (!stmt_timer_started)
+    {
+        if (StatementTimeout > 0)
+        {
+
+            /*
+             * Sanity check
+             */
+            if (!xact_started)
+            {
+                ereport(ERROR,
+                        (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+                         errmsg("Transaction must have been already started to set statement timeout")));
+            }
+
+            ereport(DEBUG3,
+                    (errmsg_internal("Set statement timeout")));
+
+            enable_timeout_after(STATEMENT_TIMEOUT, StatementTimeout);
+            stmt_timer_started = true;
+        }
+        else
+            disable_timeout(STATEMENT_TIMEOUT, false);
+    }
+}
+
+/*
+ * Reset statement timeout if any.
+ */
+static void
+disable_statement_timeout(void)
+{
+    if (stmt_timer_started)
+    {
+        ereport(DEBUG3,
+                    (errmsg_internal("Disable statement timeout")));
+
+        /* Cancel any active statement timeout */
+        disable_timeout(STATEMENT_TIMEOUT, false);
+
+        stmt_timer_started = false;
+    }
+}