On 2017-04-04 16:56:26 -0700, 'Andres Freund' wrote: > On 2017-04-04 23:52:28 +0000, Tsunakawa, Takayuki wrote: > > From: Andres Freund [mailto:and...@anarazel.de] > > > Looks to me like npgsql doesn't do that either. None of libpq, pgjdbs and > > > npgsql doing it seems like some evidence that it's ok. > > > > And psqlODBC now uses always libpq. > > > > Now time for final decision? > > I'll send an updated patch in a bit, and then will wait till tomorrow to > push. Giving others a chance to chime in seems fair.
Attached. I did not like that the previous patch had the timeout handling duplicated in the individual functions, I instead centralized it into start_xact_command(). Given that it already activated the timeout in the most common cases, that seems to make more sense to me. In your version we'd have called enable_statement_timeout() twice consecutively (which behaviourally is harmless). What do you think? I've not really tested this with the extended protocol, so I'd appreciate if you could rerun your test from the older thread. - Andres
>From c47d904725f3ef0272a4540b6b5bcf0be5c1dea6 Mon Sep 17 00:00:00 2001 From: Andres Freund <and...@anarazel.de> Date: Tue, 4 Apr 2017 18:44:42 -0700 Subject: [PATCH] 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(). Author: Tatsuo Ishii, editorialized by Andres Freund Reviewed-By: Takayuki Tsunakawa, Andres Freund Discussion: https://postgr.es/m/20170222.115044.1665674502985097185.t-is...@sraoss.co.jp --- src/backend/tcop/postgres.c | 77 ++++++++++++++++++++++++++++++++++++++------- 1 file changed, 65 insertions(+), 12 deletions(-) diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c index a2282058c0..4ab3bd7f07 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 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 * 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); static void log_disconnections(int code, Datum arg); +static void enable_statement_timeout(void); +static void disable_statement_timeout(void); /* ---------------------------------------------------------------- @@ -1234,7 +1241,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(); @@ -1522,7 +1530,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(); @@ -2014,6 +2023,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 */ @@ -2443,25 +2455,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 @@ -4511,3 +4525,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; + } +} -- 2.12.0.264.gd6db3f2165.dirty
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
