Re: Statement timeout behavior in extended queries

Hello,


I've reviewed the patch.  My understanding is as follows.  Please correct me if I'm wrong.

* The difference is that the Execute message stops the statement_timeout timer, so that the execution of one statement doesn't shorten the timeout period of subsequent statements when they are run in batch followed by a single Sync message.

* This patch is also necessary (or desirable) for the feature "Pipelining/batch mode support for libpq," which is being developed for PG 10.  This patch enables correct timeout handling for each statement execution in a batch.  Without this patch, the entire batch of statements is subject to statement_timeout.  That's different from what the manual describes about statement_timeout.  statement_timeout should measure execution of each statement.


Below are what I found in the patch.


(1)
+static bool st_timeout = false;

I think the variable name would better be stmt_timeout_enabled or stmt_timer_started, because other existing names use stmt to abbreviate statement, and thhis variable represents a state (see xact_started for example.)


(2)
+static void enable_statement_timeout(void)
+{

+static void disable_statement_timeout(void)
+{

"static void" and the remaining line should be on different lines, as other functions do.



(3)
+			/*
+			 * Sanity check
+			 */
+			if (!xact_started)
+			{
+				ereport(ERROR,
+						(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+						 errmsg("Transaction must have been already started to set statement timeout")));
+			}

I think this fragment can be deleted, because enable/disable_timeout() is used only at limited places in postgres.c, so I don't see the chance of misuse.


Regards
Takayuki Tsunakawa