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
--
Sent via pgsql-hackers mailing list ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
