Re: [HACKERS] Statement timeout behavior in extended queries

[Tatsuo Ishii]

> On 2017-09-10 17:12:19 -0700, Andres Freund wrote:
>> On 2017-09-11 09:10:49 +0900, Tatsuo Ishii wrote:
>> > If you don't mind, can you please commit/push the patch?
>> 
>> Ok, will do so.
> 
> And, done.  Thanks for patch and reminder!

Thanks!
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Statement timeout behavior in extended queries

[Andres Freund]

On 2017-09-10 17:12:19 -0700, Andres Freund wrote:
> On 2017-09-11 09:10:49 +0900, Tatsuo Ishii wrote:
> > If you don't mind, can you please commit/push the patch?
> 
> Ok, will do so.

And, done.  Thanks for patch and reminder!

- Andres


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Statement timeout behavior in extended queries

[Andres Freund]

Hi,

On 2017-09-11 09:10:49 +0900, Tatsuo Ishii wrote:
> If you don't mind, can you please commit/push the patch?

Ok, will do so.

Greetings,

Andres Freund


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Statement timeout behavior in extended queries

[Tatsuo Ishii]

Andres,

If you don't mind, can you please commit/push the patch?

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp

>> 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.
> 
> Attached is your patch just rebased against master branch.
> 
> Also I attached the test cases and results using pgproto on patched
> master branch. All looks good to me.
> 
> Best regards,
> --
> Tatsuo Ishii
> SRA OSS, Inc. Japan
> English: http://www.sraoss.co.jp/index_en.php
> Japanese:http://www.sraoss.co.jp


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Statement timeout behavior in extended queries

[Tatsuo Ishii]

> 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.

Attached is your patch just rebased against master branch.

Also I attached the test cases and results using pgproto on patched
master branch. All looks good to me.

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 8d3fecf..6c53b9c 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -144,6 +144,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.
@@ -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);
 
 
 /* 
@@ -1228,7 +1235,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();
 
@@ -1516,7 +1524,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();
 
@@ -2008,6 +2017,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 */
@@ -2437,25 +2449,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
@@ -4521,3 +4535,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;
+	}
+}
== 001-without-trans ===
FE=> Query(query="SET statement_timeout = '4s'")
<= BE CommandComplete(SET)
<= BE ReadyForQuery(I)
FE=> Parse(stmt="", query="SELECT pg_sleep(3)")
FE=> Bind(stmt="", portal="")
FE=> Execute(portal="")
FE=> Parse(stmt="", query="SELECT pg_sleep(2)")
FE=> Bind(stmt="", portal="")
FE=> Execute(portal="")
FE=> Sync
<= BE ParseComplete
<= BE BindComplete
<= BE 

Re: [HACKERS] Statement timeout behavior in extended queries

[Andres Freund]

On 2017-04-05 09:46:31 -0400, Tom Lane wrote:
> Andres Freund  writes:
> > On 2017-04-05 00:39:51 -0400, Tom Lane wrote:
> >> but
> >> if that's what we're doing, let's make sure we do it consistently.
> >> I haven't read the patch, but the comments in this thread make me fear
> >> that it's introducing some ad-hoc, inconsistent behavior.
> 
> > I'm a bit worried too due to the time constraints here, but I think
> > resetting the clock at Execute too actually makes a fair amount sense.
> 
> Meh.  Two days before feature freeze is not the time to be introducing
> a rushed redefinition of the wire protocol --- and let's not fool
> ourselves, that is what this amounts to.  Let's push this out to v11
> and think about it more carefully.

What I was trying to say is that I think the change makes sense and
isn't particularly ad-hoc, but that I don't think we need to force this
for v10.

Greetings,

Andres Freund


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Statement timeout behavior in extended queries

[Tom Lane]

Andres Freund  writes:
> On 2017-04-05 00:39:51 -0400, Tom Lane wrote:
>> but
>> if that's what we're doing, let's make sure we do it consistently.
>> I haven't read the patch, but the comments in this thread make me fear
>> that it's introducing some ad-hoc, inconsistent behavior.

> I'm a bit worried too due to the time constraints here, but I think
> resetting the clock at Execute too actually makes a fair amount sense.

Meh.  Two days before feature freeze is not the time to be introducing
a rushed redefinition of the wire protocol --- and let's not fool
ourselves, that is what this amounts to.  Let's push this out to v11
and think about it more carefully.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Statement timeout behavior in extended queries

[Tatsuo Ishii]

> Then, the following sequence should have occurred.  The test result is valid.

Yes, I remembered that and was about to make a posting :-)

> # Execute statement which takes 2 seconds.
> 'P'   "S1""SELECT pg_sleep(2)"0
>   -> start transaction T1
> 'B'   "S2""S1"0   0   0

Yes, an extended query automatically starts a transaction if there's
no ongoing transaction.

> 'P'   ""  "SET statement_timeout = '1s'"  0
> 'B'   ""  ""  0   0   0
> 'E'   ""  0
> 
> # Execute statement which takes 2 seconds (statement timeout expected).
> 'E'   "S2"0
>   -> timeout error occurred, T1 aborted

Right. The automatically started transaction is aborted and the effect
of the set statement is canceled.

In summary, as far as I know Andres's patch is working as expected.

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Statement timeout behavior in extended queries

[Andres Freund]

On 2017-04-05 00:39:51 -0400, Tom Lane wrote:
> Andres Freund  writes:
> > On 2017-04-05 10:05:19 +0900, Tatsuo Ishii wrote:
> >> What's your point of the question? What kind of problem do you expect
> >> if the timeout starts only once at the first parse meesage out of
> >> bunch of parse messages?
> 
> > It's perfectly valid to send a lot of Parse messages without
> > interspersed Sync or Bind/Execute message.  There'll be one timeout
> > covering all of those Parse messages, which can thus lead to a timeout,
> > even though nothing actually takes long individually.
> 
> It might well be reasonable to redefine statement_timeout as limiting the
> total time from the first client input to the response to Sync ...

That's actually the current behaviour. start_xact_command arms the timer
in the !xact_started case, and only finish_xact_command() disarms it.
finish_xact_command() is issued for Sync messsages and simple statements.

I brought the case up because what we're discussing is changing things
so Execute resets the clock (more precisely disables it, and have it
reenabled later).  So it'd be "fixed" for pipelined Execute statements,
but not for pipelined Parses.  Changing things so that Parse/Bind also
reset the timing would increase overhead - and it'd not be useful in
practice, because Sync's are sent by the drivers that support
pipelining when statements are prepared.


> but
> if that's what we're doing, let's make sure we do it consistently.
> I haven't read the patch, but the comments in this thread make me fear
> that it's introducing some ad-hoc, inconsistent behavior.

I'm a bit worried too due to the time constraints here, but I think
resetting the clock at Execute too actually makes a fair amount sense.

Greetings,

Andres Freund


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Statement timeout behavior in extended queries

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Tatsuo Ishii
> Since pgproto is a dumb protocol machine, it does not start a transaction
> automatically (user needs to explicitly send a start transaction command
> via either simple or extended query). In this particular case no explicit
> transaction has started.
> 

Then, the following sequence should have occurred.  The test result is valid.

# Execute statement which takes 2 seconds.
'P' "S1""SELECT pg_sleep(2)"0
  -> start transaction T1
'B' "S2""S1"0   0   0

'P' ""  "SET statement_timeout = '1s'"  0
'B' ""  ""  0   0   0
'E' ""  0

# Execute statement which takes 2 seconds (statement timeout expected).
'E' "S2"0
  -> timeout error occurred, T1 aborted

# Issue Sync message
'S'
  -> rolled back T1, so statement_timeout reverted to 3s

# Receive response from backend
'Y'

# Execute statement which takes 2 seconds (statement timeout expected).
'P' "S3""SELECT pg_sleep(2)"0
  -> start transaction T2
'B' "S2""S3"0   0   0
'E' "S2"0

# Issue Sync message
'S'
  -> committed T2

Regards
Takayuki Tsunakawa



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Statement timeout behavior in extended queries

[Tatsuo Ishii]

> From: pgsql-hackers-ow...@postgresql.org
>> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Tatsuo Ishii
>> I have done tests using pgproto. One thing I noticed a strange behavior.
>> Below is an output of pgproto. The test first set the timeout to 3 seconds,
>> and parse/bind for "SELECT pg_sleep(2)" then set timeout to 1 second using
>> extended query. Subsequent Execute emits a statement timeout error as
>> expected, but next "SELECT pg_sleep(2)"
>> call using extended query does not emit a statement error. The test for
>> this is "007-timeout-twice". Attached is the test cases including this.
> 
> What's the handling of transactions like in pgproto?  I guess the first 
> statement timeout error rolled back the effect of "SET statement_timeout = 
> '1s'", and the timeout reverted to 3s or some other value.

Since pgproto is a dumb protocol machine, it does not start a
transaction automatically (user needs to explicitly send a start
transaction command via either simple or extended query). In this
particular case no explicit transaction has started.

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Statement timeout behavior in extended queries

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Tatsuo Ishii
> I have done tests using pgproto. One thing I noticed a strange behavior.
> Below is an output of pgproto. The test first set the timeout to 3 seconds,
> and parse/bind for "SELECT pg_sleep(2)" then set timeout to 1 second using
> extended query. Subsequent Execute emits a statement timeout error as
> expected, but next "SELECT pg_sleep(2)"
> call using extended query does not emit a statement error. The test for
> this is "007-timeout-twice". Attached is the test cases including this.

What's the handling of transactions like in pgproto?  I guess the first 
statement timeout error rolled back the effect of "SET statement_timeout = 
'1s'", and the timeout reverted to 3s or some other value.

Regards
Takayuki Tsunakawa




-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Statement timeout behavior in extended queries

[Tatsuo Ishii]

>> 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.
> 
> The patch looks good and cleaner.  It looks like the code works as expected.  
> As before, I ran one INSERT statement with PgJDBC, with gdb's breakpoints set 
> on enable_timeout() and disable_timeout().  I confirmed that enable_timeout() 
> is called just once at Parse message, and disable_timeout() is called just 
> once at Execute message.
> 
> I'd like to wait for Tatsuo-san's thorough testing with pgproto.

I have done tests using pgproto. One thing I noticed a strange
behavior. Below is an output of pgproto. The test first set the
timeout to 3 seconds, and parse/bind for "SELECT pg_sleep(2)" then set
timeout to 1 second using extended query. Subsequent Execute emits a
statement timeout error as expected, but next "SELECT pg_sleep(2)"
call using extended query does not emit a statement error. The test
for this is "007-timeout-twice". Attached is the test cases including
this.

FE=> Query(query="SET statement_timeout = '3s'")
<= BE CommandComplete(SET)
<= BE ReadyForQuery(I)
FE=> Parse(stmt="S1", query="SELECT pg_sleep(2)")
FE=> Bind(stmt="S1", portal="S2")
FE=> Parse(stmt="", query="SET statement_timeout = '1s'")
FE=> Bind(stmt="", portal="")
FE=> Execute(portal="")
FE=> Execute(portal="S2")
FE=> Sync
<= BE ParseComplete
<= BE BindComplete
<= BE ParseComplete
<= BE BindComplete
<= BE CommandComplete(SET)
<= BE ErrorResponse(S ERROR V ERROR C 57014 M canceling statement due to 
statement timeout F postgres.c L 2968 R ProcessInterrupts )
<= BE ReadyForQuery(I)
FE=> Parse(stmt="S3", query="SELECT pg_sleep(2)")
FE=> Bind(stmt="S3", portal="S2")
FE=> Execute(portal="S2")
FE=> Sync
<= BE ParseComplete
<= BE BindComplete
<= BE DataRow
<= BE CommandComplete(SELECT 1)
<= BE ReadyForQuery(I)
FE=> Terminate



tests.tar.gz
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Statement timeout behavior in extended queries

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of 'Andres Freund'
> 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.

The patch looks good and cleaner.  It looks like the code works as expected.  
As before, I ran one INSERT statement with PgJDBC, with gdb's breakpoints set 
on enable_timeout() and disable_timeout().  I confirmed that enable_timeout() 
is called just once at Parse message, and disable_timeout() is called just once 
at Execute message.

I'd like to wait for Tatsuo-san's thorough testing with pgproto.

Regards
Takayuki Tsunakawa




-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Statement timeout behavior in extended queries

[Tom Lane]

Andres Freund  writes:
> On 2017-04-05 10:05:19 +0900, Tatsuo Ishii wrote:
>> What's your point of the question? What kind of problem do you expect
>> if the timeout starts only once at the first parse meesage out of
>> bunch of parse messages?

> It's perfectly valid to send a lot of Parse messages without
> interspersed Sync or Bind/Execute message.  There'll be one timeout
> covering all of those Parse messages, which can thus lead to a timeout,
> even though nothing actually takes long individually.

It might well be reasonable to redefine statement_timeout as limiting the
total time from the first client input to the response to Sync ... but
if that's what we're doing, let's make sure we do it consistently.

I haven't read the patch, but the comments in this thread make me fear
that it's introducing some ad-hoc, inconsistent behavior.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Statement timeout behavior in extended queries

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Tatsuo Ishii
> Hmm. IMO, that could happen even with the current statement timeout
> implementation as well.
> 
> Or we could start/stop the timeout in exec_execute_message() only. This
> could avoid the problem above. Also this is more consistent with
> log_duration/log_min_duration_statement behavior than now.

I think it's better to include Parse and Bind as now.  Parse or Bind could take 
long time when the table has many partitions, the query is complex and/or very 
long, some DDL statement is running against a target table, or the system load 
is high.

Firing statement timeout during or after many Parses is not a problem, because 
the first Parse started running some statement and it's not finished.  Plus, 
Andres confirmed that major client drivers don't use such a pattern.  There may 
be an odd behavior purely from the perspective of E.Q.P, that's a compromise, 
which Andres meant by "not perfect but."

Regards
Takayuki Tsunakawa



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Statement timeout behavior in extended queries

[Tatsuo Ishii]

> On 2017-04-04 16:56:26 -0700, 'Andres Freund' wrote:
>> On 2017-04-04 23:52:28 +, 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.

Ok, I will look into your patch and test it out using pgproto.

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Statement timeout behavior in extended queries

['Andres Freund']

On 2017-04-04 16:56:26 -0700, 'Andres Freund' wrote:
> On 2017-04-04 23:52:28 +, 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 
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 */
-		

Re: [HACKERS] Statement timeout behavior in extended queries

[Tatsuo Ishii]

>> What's your point of the question? What kind of problem do you expect
>> if the timeout starts only once at the first parse meesage out of
>> bunch of parse messages?
> 
> It's perfectly valid to send a lot of Parse messages without
> interspersed Sync or Bind/Execute message.  There'll be one timeout
> covering all of those Parse messages, which can thus lead to a timeout,
> even though nothing actually takes long individually.

Hmm. IMO, that could happen even with the current statement timeout
implementation as well.

Or we could start/stop the timeout in exec_execute_message()
only. This could avoid the problem above. Also this is more consistent
with log_duration/log_min_duration_statement behavior than now.

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Statement timeout behavior in extended queries

[Andres Freund]

On 2017-04-05 10:05:19 +0900, Tatsuo Ishii wrote:
> > Hm. I started to edit it, but I'm halfway coming back to my previous
> > view that this isn't necessarily ready.
> > 
> > If a client were to to prepare a large number of prepared statements
> > (i.e. a lot of parse messages), this'll only start the timeout once, at
> > the first statement sent.  It's not an issue for libpq which sends a
> > sync message after each PQprepare, nor does it look to be an issue for
> > pgjdbc based on a quick look.
> > 
> > Does anybody think there might be a driver out there that sends a bunch
> > of 'parse' messages, without syncs?
> 
> What's your point of the question? What kind of problem do you expect
> if the timeout starts only once at the first parse meesage out of
> bunch of parse messages?

It's perfectly valid to send a lot of Parse messages without
interspersed Sync or Bind/Execute message.  There'll be one timeout
covering all of those Parse messages, which can thus lead to a timeout,
even though nothing actually takes long individually.

Greetings,

Andres Freund


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Statement timeout behavior in extended queries

[Tatsuo Ishii]

> Hm. I started to edit it, but I'm halfway coming back to my previous
> view that this isn't necessarily ready.
> 
> If a client were to to prepare a large number of prepared statements
> (i.e. a lot of parse messages), this'll only start the timeout once, at
> the first statement sent.  It's not an issue for libpq which sends a
> sync message after each PQprepare, nor does it look to be an issue for
> pgjdbc based on a quick look.
> 
> Does anybody think there might be a driver out there that sends a bunch
> of 'parse' messages, without syncs?

What's your point of the question? What kind of problem do you expect
if the timeout starts only once at the first parse meesage out of
bunch of parse messages?

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Statement timeout behavior in extended queries

['Andres Freund']

On 2017-04-04 23:52:28 +, 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.

- Andres


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Statement timeout behavior in extended queries

[Tsunakawa, Takayuki]

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?

Regards
Takayuki Tsunakawa




-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Statement timeout behavior in extended queries

[Andres Freund]

On 2017-04-04 16:38:53 -0700, Andres Freund wrote:
> On 2017-04-04 16:10:32 +0900, Tatsuo Ishii wrote:
> > >> If what Tatsuo-san said to Tom is correct (i.e. each Parse/Bind/Execute 
> > >> starts and stops the timer), then it's a concern and the patch should 
> > >> not be ready for committer.  However, the current patch is not like that 
> > >> -- it seems to do what others in this thread are expecting.
> > > 
> > > Oh, interesting - I kind of took the author's statement as, uh,
> > > authoritative ;).  A quick look over the patch confirms your
> > > understanding.
> > 
> > Yes, Tsunakawa-san is correct. Sorry for confusion.
> > 
> > > 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.
> 
> Hm. I started to edit it, but I'm halfway coming back to my previous
> view that this isn't necessarily ready.
> 
> If a client were to to prepare a large number of prepared statements
> (i.e. a lot of parse messages), this'll only start the timeout once, at
> the first statement sent.  It's not an issue for libpq which sends a
> sync message after each PQprepare, nor does it look to be an issue for
> pgjdbc based on a quick look.
> 
> Does anybody think there might be a driver out there that sends a bunch
> of 'parse' messages, without syncs?

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.

- Andres


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Statement timeout behavior in extended queries

[Andres Freund]

On 2017-04-05 08:34:43 +0900, Tatsuo Ishii wrote:
> 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).

I'm working on the patch, and I've edited it more heavily, so please
hold off.

Changes:
I don't think the debugging statements are a good idea, the
!xact_started should be an assert, and disable_timeout should be called
from within enable_statement_timeout independent of stmt_timer_started.


But more importantly I had just sent a question that I think merits
discussion.


- Andres


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Statement timeout behavior in extended queries

[Andres Freund]

On 2017-04-04 16:10:32 +0900, Tatsuo Ishii wrote:
> >> If what Tatsuo-san said to Tom is correct (i.e. each Parse/Bind/Execute 
> >> starts and stops the timer), then it's a concern and the patch should not 
> >> be ready for committer.  However, the current patch is not like that -- it 
> >> seems to do what others in this thread are expecting.
> > 
> > Oh, interesting - I kind of took the author's statement as, uh,
> > authoritative ;).  A quick look over the patch confirms your
> > understanding.
> 
> Yes, Tsunakawa-san is correct. Sorry for confusion.
> 
> > 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.

Hm. I started to edit it, but I'm halfway coming back to my previous
view that this isn't necessarily ready.

If a client were to to prepare a large number of prepared statements
(i.e. a lot of parse messages), this'll only start the timeout once, at
the first statement sent.  It's not an issue for libpq which sends a
sync message after each PQprepare, nor does it look to be an issue for
pgjdbc based on a quick look.

Does anybody think there might be a driver out there that sends a bunch
of 'parse' messages, without syncs?

- Andres


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Statement timeout behavior in extended queries

[Tatsuo Ishii]

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);
 static void 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 on whether 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 more rows 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 end a 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 before committing */
-		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, 

Re: [HACKERS] Statement timeout behavior in extended queries

[Tatsuo Ishii]

>> If what Tatsuo-san said to Tom is correct (i.e. each Parse/Bind/Execute 
>> starts and stops the timer), then it's a concern and the patch should not be 
>> ready for committer.  However, the current patch is not like that -- it 
>> seems to do what others in this thread are expecting.
> 
> Oh, interesting - I kind of took the author's statement as, uh,
> authoritative ;).  A quick look over the patch confirms your
> understanding.

Yes, Tsunakawa-san is correct. Sorry for confusion.

> 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.

> Unfortunately I can't move the patch back to the current CF, but I guess
> we can just mark it as committed in the next.

That will be great.

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Statement timeout behavior in extended queries

['Andres Freund']

On 2017-04-04 06:35:00 +, Tsunakawa, Takayuki wrote:
> From: Andres Freund [mailto:and...@anarazel.de]
> Given the concern raised in
> > http://archives.postgresql.org/message-id/12207.1491228316%40sss.pgh.p
> > a.us
> > I don't see this being ready for committer.
> 
> If what Tatsuo-san said to Tom is correct (i.e. each Parse/Bind/Execute 
> starts and stops the timer), then it's a concern and the patch should not be 
> ready for committer.  However, the current patch is not like that -- it seems 
> to do what others in this thread are expecting.

Oh, interesting - I kind of took the author's statement as, uh,
authoritative ;).  A quick look over the patch confirms your
understanding.

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.

Unfortunately I can't move the patch back to the current CF, but I guess
we can just mark it as committed in the next.

- Andres


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Statement timeout behavior in extended queries

[Tsunakawa, Takayuki]

From: Andres Freund [mailto:and...@anarazel.de]
Given the concern raised in
> http://archives.postgresql.org/message-id/12207.1491228316%40sss.pgh.p
> a.us
> I don't see this being ready for committer.

If what Tatsuo-san said to Tom is correct (i.e. each Parse/Bind/Execute starts 
and stops the timer), then it's a concern and the patch should not be ready for 
committer.  However, the current patch is not like that -- it seems to do what 
others in this thread are expecting.

Regards
Takayuki Tsunakawa



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Statement timeout behavior in extended queries

[Andres Freund]

On 2017-04-04 06:18:04 +, Tsunakawa, Takayuki wrote:
> From: Tatsuo Ishii [mailto:is...@sraoss.co.jp]
> > It's too late. Someone has already moved the patch to the next CF (for
> > PostgreSQL 11).
> 
> Yes, but this patch will be necessary by the final release of PG 10 if the 
> libpq batch/pipelining is committed in PG 10.
> 
> I marked this as ready for committer in the next CF, so that some committer 
> can pick up this patch and consider putting it in PG 10.  If you decide to 
> modify the patch, please change the status.

Given the concern raised in 
http://archives.postgresql.org/message-id/12207.1491228316%40sss.pgh.pa.us
I don't see this being ready for committer.

- Andres


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Statement timeout behavior in extended queries

[Tsunakawa, Takayuki]

From: Tatsuo Ishii [mailto:is...@sraoss.co.jp]
> It's too late. Someone has already moved the patch to the next CF (for
> PostgreSQL 11).

Yes, but this patch will be necessary by the final release of PG 10 if the 
libpq batch/pipelining is committed in PG 10.

I marked this as ready for committer in the next CF, so that some committer can 
pick up this patch and consider putting it in PG 10.  If you decide to modify 
the patch, please change the status.

Regards
Takayuki Tsunakawa





-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Statement timeout behavior in extended queries

[Tatsuo Ishii]

> The patch doesn't seem to behave like that.  The Parse message calls 
> start_xact_command() -> enable_statement_timeout() -> enable_timeout(), and 
> set stmt_timer_started to true.  Subsequent Bind and Execute messages call 
> enable_statement_timeout(), but enable_statement_timeout() doesn't call 
> enable_timeout() because stmt_timer_started is already true.
> 
> 
>> > It looks like the patch does the following.  I think this is desirable,
>> because starting and stopping the timer for each message may be costly as
>> Tom said.
>> > Parse(statement1)
>> >   start timer
>> > Bind(statement1, portal1)
>> > Execute(portal1)
>> >   stop timer
>> > Sync
> 
> I ran one INSERT statement using JDBC with log_min_messages = DEBUG3, and the 
> server log shows what I said.
> 
> DEBUG:  parse : insert into a values(2)
> DEBUG:  Set statement timeout
> LOG:  duration: 0.431 ms  parse : insert into a values(2)
> DEBUG:  bind  to 
> LOG:  duration: 0.127 ms  bind : insert into a values(2)
> DEBUG:  Disable statement timeout
> LOG:  duration: 0.184 ms  execute : insert into a values(2)
> DEBUG:  snapshot of 1+0 running transaction ids (lsn 0/163BF28 oldest xid 561 
> latest complete 560 next xid 562)

Check.

>> This doesn't work in general use cases. Following pattern appears frequently
>> in applications.
>> 
>> Parse(statement1)
>> Bind(statement1, portal1)
>> Execute(portal1)
>> Bind(statement1, portal1)
>> Execute(portal1)
>> :
>> :
>> Sync
> 
> It works.  The first Parse-Bind-Execute is measured as one unit, then 
> subsequent Bind-Execute pairs are measured as other units.  That's because 
> each Execute ends the statement_timeout timer and the Bind message starts it 
> again.  I think this is desirable, so the current patch looks good.  May I 
> mark this as ready for committer?  FYI, make check-world passed successfully.

It's too late. Someone has already moved the patch to the next CF (for
PostgreSQL 11).

>> Also what would happen if client just send a parse message and does nothing
>> after that?
> 
> It's correct to trigger the statement timeout in this case, because the first 
> SQL statement started (with the Parse message) and its execution is not 
> finished (with Execute message.)
> 
> 
> Regards
> Takayuki Tsunakawa
> 
> 
> 


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Statement timeout behavior in extended queries

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Tatsuo Ishii
> Actually the statement timer is replaced with new statement timer value
> in enable_statement_timeout().

The patch doesn't seem to behave like that.  The Parse message calls 
start_xact_command() -> enable_statement_timeout() -> enable_timeout(), and set 
stmt_timer_started to true.  Subsequent Bind and Execute messages call 
enable_statement_timeout(), but enable_statement_timeout() doesn't call 
enable_timeout() because stmt_timer_started is already true.


> > It looks like the patch does the following.  I think this is desirable,
> because starting and stopping the timer for each message may be costly as
> Tom said.
> > Parse(statement1)
> >   start timer
> > Bind(statement1, portal1)
> > Execute(portal1)
> >   stop timer
> > Sync

I ran one INSERT statement using JDBC with log_min_messages = DEBUG3, and the 
server log shows what I said.

DEBUG:  parse : insert into a values(2)
DEBUG:  Set statement timeout
LOG:  duration: 0.431 ms  parse : insert into a values(2)
DEBUG:  bind  to 
LOG:  duration: 0.127 ms  bind : insert into a values(2)
DEBUG:  Disable statement timeout
LOG:  duration: 0.184 ms  execute : insert into a values(2)
DEBUG:  snapshot of 1+0 running transaction ids (lsn 0/163BF28 oldest xid 561 
latest complete 560 next xid 562)


> This doesn't work in general use cases. Following pattern appears frequently
> in applications.
> 
> Parse(statement1)
> Bind(statement1, portal1)
> Execute(portal1)
> Bind(statement1, portal1)
> Execute(portal1)
> :
> :
> Sync

It works.  The first Parse-Bind-Execute is measured as one unit, then 
subsequent Bind-Execute pairs are measured as other units.  That's because each 
Execute ends the statement_timeout timer and the Bind message starts it again.  
I think this is desirable, so the current patch looks good.  May I mark this as 
ready for committer?  FYI, make check-world passed successfully.


> Also what would happen if client just send a parse message and does nothing
> after that?

It's correct to trigger the statement timeout in this case, because the first 
SQL statement started (with the Parse message) and its execution is not 
finished (with Execute message.)


Regards
Takayuki Tsunakawa





-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Statement timeout behavior in extended queries

[Tatsuo Ishii]

> Where is the statement_timeout timer stopped when processing Parse and Bind 
> messages?

Actually the statement timer is replaced with new statement timer
value in enable_statement_timeout().

> Do you mean the following sequence of operations are performed in this patch?
> 
> Parse(statement1)
>   start timer
>   stop timer
> Bind(statement1, portal1)
>   start timer
>   stop timer
> Execute(portal1)
>   start timer
>   stop timer
> Sync

Yes.

> It looks like the patch does the following.  I think this is desirable, 
> because starting and stopping the timer for each message may be costly as Tom 
> said.
> Parse(statement1)
>   start timer
> Bind(statement1, portal1)
> Execute(portal1)
>   stop timer
> Sync

This doesn't work in general use cases. Following pattern appears
frequently in applications.

Parse(statement1)
Bind(statement1, portal1)
Execute(portal1)
Bind(statement1, portal1)
Execute(portal1)
:
:
Sync

Also what would happen if client just send a parse message and does
nothing after that?

So I think following is better:

Parse(statement1)
Bind(statement1, portal1)
Execute(portal1)
  start timer
  stop timer
Bind(statement1, portal1)
Execute(portal1)
  start timer
  stop timer
:
:
Sync

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Statement timeout behavior in extended queries

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Tatsuo Ishii
> No. Parse, bind and Execute message indivually stops and starts the
> statement_timeout timer with the patch. Unless there's a lock conflict,
> parse and bind will take very short time. So actually users could only
> observe the timeout in execute message though.

Where is the statement_timeout timer stopped when processing Parse and Bind 
messages?  Do you mean the following sequence of operations are performed in 
this patch?

Parse(statement1)
  start timer
  stop timer
Bind(statement1, portal1)
  start timer
  stop timer
Execute(portal1)
  start timer
  stop timer
Sync

It looks like the patch does the following.  I think this is desirable, because 
starting and stopping the timer for each message may be costly as Tom said.

Parse(statement1)
  start timer
Bind(statement1, portal1)
Execute(portal1)
  stop timer
Sync


> > (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.
> 
> I'd suggest leave it as it is. Because it might be possible that the function
> is used in different place in the future. Or at least we should document
> the pre-condition as a comment.

OK, I favor documenting to reduce code, but I don't have a strong opinion.  I'd 
like to leave this to the committer.  One thing to note is that you can remove 
{ and } in the following code like other places.

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

Regards
Takayuki Tsunakawa



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Statement timeout behavior in extended queries

[Andres Freund]

On 2017-04-03 23:31:59 +0900, Tatsuo Ishii wrote:
> > That seems like it could represent quite a lot of added overhead,
> > on machines where gettimeofday() is slow ... which is still a lot
> > of them, unfortunately.
> 
> Maybe. I think we could eliminate restarting the timer for parse and
> bind.

I've moved this patch to the next CF - it's been submitted fairly late
in the v10 cycle and there's obviously design work ongoing.

- Andres


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Statement timeout behavior in extended queries

[Tatsuo Ishii]

> That seems like it could represent quite a lot of added overhead,
> on machines where gettimeofday() is slow ... which is still a lot
> of them, unfortunately.

Maybe. I think we could eliminate restarting the timer for parse and
bind.

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Statement timeout behavior in extended queries

[Tom Lane]

Tatsuo Ishii  writes:
>> * The difference is that the Execute message stops the statement_timeout 
>> timer, 

> No. Parse, bind and Execute message indivually stops and starts the
> statement_timeout timer with the patch.

That seems like it could represent quite a lot of added overhead,
on machines where gettimeofday() is slow ... which is still a lot
of them, unfortunately.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Statement timeout behavior in extended queries

[Tatsuo Ishii]

Thank you for reviewing my patch!

> 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, 

No. Parse, bind and Execute message indivually stops and starts the
statement_timeout timer with the patch. Unless there's a lock
conflict, parse and bind will take very short time. So actually users
could only observe the timeout in execute message though.

> 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 is true. Currently multiple set of parse/bind/execute will not
trigger statement timeout until sync message is sent to
backend. Suppose statement_timeout is set to 3 seconds, combo A
(parse/bind/execute) takes 2 seconds and combo B (parse/bind/execute)
takes 2 seconds then a sync message is followed. Currently statement
timeout is fired in the run of combo B (assuming that parse and bind
take almost 0 seconds). With my patch, no statement timeout will be
fired because both combo A and combo B runs within 3 seconds.

> * 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.

True.

> 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.)

Agreed. Chaged to stmt_timer_started.

> (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.

Fixed.

> (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.

I'd suggest leave it as it is. Because it might be possible that the
function is used in different place in the future. Or at least we
should document the pre-condition as a comment.

revised patch attached.
--
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..68a739e 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);
 static void log_disconnections(int code, Datum arg);
+static void enable_statement_timeout(void);
+static void disable_statement_timeout(void);
 
 
 /* 
@@ -1239,6 +1246,11 @@ exec_parse_message(const char *query_string,	/* string to execute */
 	start_xact_command();
 
 	/*
+	 * Set statement timeout running, if any
+	 */
+	enable_statement_timeout();
+
+	/*
 	 * Switch to appropriate context for constructing parsetrees.
 	 *
 	 * We have two strategies depending on whether the prepared statement is
@@ -1526,6 +1538,11 @@ exec_bind_message(StringInfo input_message)
 	 */
 	start_xact_command();
 
+	/*
+	 * Set statement timeout running, if any
+	 */
+	enable_statement_timeout();
+
 	/* Switch back to message context */
 	MemoryContextSwitchTo(MessageContext);
 
@@ -1942,6 +1959,11 @@ exec_execute_message(const char *portal_name, long max_rows)
 	start_xact_command();
 
 	/*
+	 * Set statement timeout running, if any
+	 */
+	enable_statement_timeout();
+
+	/*
 	 * 

Re: [HACKERS] Statement timeout behavior in extended queries

[Tsunakawa, Takayuki]

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 (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Statement timeout behavior in extended queries

[Tatsuo Ishii]

> On Wed, Feb 22, 2017 at 11:50:44AM +0900, Tatsuo Ishii wrote:
>> Last year I have proposed an enhancement regarding behavior of the
>> statement timeout in extended queries.
>> 
>> https://www.postgresql.org/message-id/20160528.220442.1489791680347556026.t-ishii%40sraoss.co.jp
>> 
>> IMO the current behavior is counter intuitive and I would like to
>> change it toward PostgreSQL 10.0.
>> 
>> For example, suppose that the timeout is set to 4 seconds and the
>> first query takes 2 seconds and the second query takes 3 seconds. Then
>> the statement timeout is triggered if a sync message is sent to
>> backend after the second query.
>> 
>> Moreover, log_duration or log_min_duration_statement shows that each
>> query took 2 or 3 seconds of course, which is not very consistent with
>> the statement timeout IMO.
>> 
>> Attached patch tries to change the behavior, by checking statement
>> timeout against each phase of an extended query.
>> 
>> To test the patch, I have created a small tool called "pgproto", which
>> can issue arbitrary sequence of frontend/backend message, reading from a
>> text file.
>> 
>> https://github.com/tatsuo-ishii/pgproto
>> (to build the program, you need C compiler and libpq)
> 
> Does it seem reasonable to start making this into a regression test
> and/or fuzz test for the protocol itself?

I personally think the regression tests ought to include tests for
extended query protocols and pgproto could be an useful tool to
implement that. Of course if we are going for that direction, pgproto
needs to be a contrib module first.

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Statement timeout behavior in extended queries

[David Fetter]

On Wed, Feb 22, 2017 at 11:50:44AM +0900, Tatsuo Ishii wrote:
> Last year I have proposed an enhancement regarding behavior of the
> statement timeout in extended queries.
> 
> https://www.postgresql.org/message-id/20160528.220442.1489791680347556026.t-ishii%40sraoss.co.jp
> 
> IMO the current behavior is counter intuitive and I would like to
> change it toward PostgreSQL 10.0.
> 
> For example, suppose that the timeout is set to 4 seconds and the
> first query takes 2 seconds and the second query takes 3 seconds. Then
> the statement timeout is triggered if a sync message is sent to
> backend after the second query.
> 
> Moreover, log_duration or log_min_duration_statement shows that each
> query took 2 or 3 seconds of course, which is not very consistent with
> the statement timeout IMO.
> 
> Attached patch tries to change the behavior, by checking statement
> timeout against each phase of an extended query.
> 
> To test the patch, I have created a small tool called "pgproto", which
> can issue arbitrary sequence of frontend/backend message, reading from a
> text file.
> 
> https://github.com/tatsuo-ishii/pgproto
> (to build the program, you need C compiler and libpq)

Does it seem reasonable to start making this into a regression test
and/or fuzz test for the protocol itself?

Best,
David.
-- 
David Fetter  http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter  XMPP: david(dot)fetter(at)gmail(dot)com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Statement timeout

[Tatsuo Ishii]

>> BTW, aren't you missing a re-enable of the timeout for statements after
>> the first one?
> 
> Will check.

You are right. Here is the revised patch.

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 b185c1b..88f5c54 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 st_timeout = 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 *parseTrees);
 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);
 
 
 /* 
@@ -1227,6 +1234,11 @@ exec_parse_message(const char *query_string,	/* string to execute */
 	start_xact_command();
 
 	/*
+	 * Set statement timeout running, if any
+	 */
+	enable_statement_timeout();
+
+	/*
 	 * Switch to appropriate context for constructing parsetrees.
 	 *
 	 * We have two strategies depending on whether the prepared statement is
@@ -1516,6 +1528,11 @@ exec_bind_message(StringInfo input_message)
 	 */
 	start_xact_command();
 
+	/*
+	 * Set statement timeout running, if any
+	 */
+	enable_statement_timeout();
+
 	/* Switch back to message context */
 	MemoryContextSwitchTo(MessageContext);
 
@@ -1931,6 +1948,11 @@ exec_execute_message(const char *portal_name, long max_rows)
 	start_xact_command();
 
 	/*
+	 * Set statement timeout running, if any
+	 */
+	enable_statement_timeout();
+
+	/*
 	 * If we re-issue an Execute protocol request against an existing portal,
 	 * then we are only fetching more rows rather than completely re-executing
 	 * the query from the start. atStart is never reset for a v3 portal, so we
@@ -2002,6 +2024,11 @@ exec_execute_message(const char *portal_name, long max_rows)
 			 * those that start or end a transaction block.
 			 */
 			CommandCounterIncrement();
+
+			/*
+			 * We need to reset statement timeout if already set.
+			 */
+			disable_statement_timeout();
 		}
 
 		/* Send appropriate CommandComplete to client */
@@ -2433,14 +2460,10 @@ start_xact_command(void)
 (errmsg_internal("StartTransactionCommand")));
 		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();
 	}
 }
 
@@ -2450,7 +2473,7 @@ finish_xact_command(void)
 	if (xact_started)
 	{
 		/* Cancel any active statement timeout before committing */
-		disable_timeout(STATEMENT_TIMEOUT, false);
+		disable_statement_timeout();
 
 		/* Now commit the command */
 		ereport(DEBUG3,
@@ -4510,3 +4533,51 @@ 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 (!st_timeout)
+	{
+		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);
+			st_timeout = true;
+		}
+		else
+			disable_timeout(STATEMENT_TIMEOUT, false);
+	}
+}
+
+/*
+ * Reset statement timeout if any.
+ */
+static void disable_statement_timeout(void)
+{
+	if (st_timeout)
+	{
+		ereport(DEBUG3,
+	(errmsg_internal("Disable statement timeout")));
+
+		/* Cancel any active statement timeout */
+		disable_timeout(STATEMENT_TIMEOUT, false);
+
+		st_timeout = false;
+	}
+}

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Statement timeout

[Tatsuo Ishii]

>> I didn't noticed it. Could you give me the message id or URL?
>>
>>
> https://commitfest.postgresql.org/10/634/
> 
> https://www.postgresql.org/message-id/flat/CAMsr+YFUjJytRyV4J-16bEoiZyH=4nj+sQ7JP9ajwz=b4dm...@mail.gmail.com#CAMsr+YFUjJytRyV4J-16bEoiZyH=4nj+sQ7JP9ajwz=b4dm...@mail.gmail.com

Thanks.

>> Another issue is inconsistency with log duration, which shows the the
>> elapsed time for each execute message. I think statement timeout
>> should be consistent with statement duration. Otherwise users will be
>> confused.
>>
> 
> While I agree that's confusing, I think that's actualyl a problem with
> log_duration.
> 
> log_duration is really more of an internal trace parameter that should be
> named debug_log_duration or something IMO. It also fails to print the
> message type, which makes it even more confusing since it for a typical
> extended protocl query it usually logs 3 durations with no indication of
> which is what.

It's definitely a poor design.

> Users should be using log_min_duration_statement. You know, the confusingly
> named one. Or is it log_duration_min_statement or
> log_statement_min_duration or ...?
> 
> Yeah, log_duration is confusing to the point I think it needs a comment
> like "To record query run-time you probably want
> log_min_duration_statement, not log_duration".

I'm confused. Regarding the timing whether duration is emitted at sync
or each message, log_duration and log_min_duration_statement behave
exactly same, no? If so, log_min_duration_statement is not consistent
with statement_timeout, anyway.

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Statement timeout

[Craig Ringer]

On 3 June 2016 at 09:45, Tatsuo Ishii  wrote:

> > Well, multiple parse/bind/execute messages before a sync are definitely
> > used by PgJDBC and nPgSQL for batching,
>
> Yes, I realized in JDBC.
>
> > and I just posted a patch for it
> > for libpq.
>
> I didn't noticed it. Could you give me the message id or URL?
>
>
https://commitfest.postgresql.org/10/634/

https://www.postgresql.org/message-id/flat/CAMsr+YFUjJytRyV4J-16bEoiZyH=4nj+sQ7JP9ajwz=b4dm...@mail.gmail.com#CAMsr+YFUjJytRyV4J-16bEoiZyH=4nj+sQ7JP9ajwz=b4dm...@mail.gmail.com



> > I am very surprised to find out that statement_timeout tracks the total
> > time and isn't reset by a new statement, but I guess it makes sense -
> what,
> > exactly, delimits a "query" in extended query mode? statement_timeout in
> > simple-query mode starts at parse time and runs until the end of execute.
> > In e.q.p. there might be only one parse, then a series of Bind and
> Execute
> > messages, or there may be repeated Parse messages.
>
> Another issue is inconsistency with log duration, which shows the the
> elapsed time for each execute message. I think statement timeout
> should be consistent with statement duration. Otherwise users will be
> confused.
>

While I agree that's confusing, I think that's actualyl a problem with
log_duration.

log_duration is really more of an internal trace parameter that should be
named debug_log_duration or something IMO. It also fails to print the
message type, which makes it even more confusing since it for a typical
extended protocl query it usually logs 3 durations with no indication of
which is what.

Users should be using log_min_duration_statement. You know, the confusingly
named one. Or is it log_duration_min_statement or
log_statement_min_duration or ...?

Yeah, log_duration is confusing to the point I think it needs a comment
like "To record query run-time you probably want
log_min_duration_statement, not log_duration".

-- 
 Craig Ringer   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Re: [HACKERS] Statement timeout

[Tatsuo Ishii]

> Well, multiple parse/bind/execute messages before a sync are definitely
> used by PgJDBC and nPgSQL for batching,

Yes, I realized in JDBC.

> and I just posted a patch for it
> for libpq.

I didn't noticed it. Could you give me the message id or URL?

I wouldn't have considered it to simulate multi-statement
> simple-protocol queries, but I guess there are some parallels.
> 
> I am very surprised to find out that statement_timeout tracks the total
> time and isn't reset by a new statement, but I guess it makes sense - what,
> exactly, delimits a "query" in extended query mode? statement_timeout in
> simple-query mode starts at parse time and runs until the end of execute.
> In e.q.p. there might be only one parse, then a series of Bind and Execute
> messages, or there may be repeated Parse messages.

Another issue is inconsistency with log duration, which shows the the
elapsed time for each execute message. I think statement timeout
should be consistent with statement duration. Otherwise users will be
confused.

> Personally I'd be fairly happy with statement-timeout applying per-message
> in the extended query protocol. That would mean that it behaves slightly
> differently, and a query with a long slow parse and bind phase followed by
> quick execution might fail to time out in the extended query protocol where
> it would time out as a simple query.
> It'd behave as if the query was
> PREPAREd then separately EXECUTEd in simple-query protocol. I'm not hugely
> bothered by that, but if it's really a concern I'd ideally like to add a
> new protocol message that resets the statement timeout counter, so the
> client can define what delimits a statement. Not practical in the near term.
> 
> For now I'd be OK with documenting this as a quirk/limitation of
> statement_timeout, that it applies to a whole extended-query-protocol
> batch.

Probably we should apply the code change for 10.0 if any. (of course
this will not be applied if many uses are getting angry with current
behavior of statement timeout).

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Statement timeout

[Craig Ringer]

On 1 June 2016 at 08:33, Tatsuo Ishii  wrote:

> > FWIW, I think the existing behavior is just fine.  It corresponds to what
> > PQexec has always done with multi-statement query strings; that is,
> > statement_timeout governs the total time to execute the transaction (the
> > whole query string, unless you put transaction control commands in
> there).
> > In extended query mode, since you can only put one textual query per
> Parse
> > message, that maps to statement_timeout governing the total time from
> > initial Parse to Sync.  Which is what it does.
>
> I've never thought about that. And I cannot imagine anyone is using
> that way in extended protocol to simulate multi-statement queries. Is
> that documented somewhere?
>

Well, multiple parse/bind/execute messages before a sync are definitely
used by PgJDBC and nPgSQL for batching, and I just posted a patch for it
for libpq. I wouldn't have considered it to simulate multi-statement
simple-protocol queries, but I guess there are some parallels.

I am very surprised to find out that statement_timeout tracks the total
time and isn't reset by a new statement, but I guess it makes sense - what,
exactly, delimits a "query" in extended query mode? statement_timeout in
simple-query mode starts at parse time and runs until the end of execute.
In e.q.p. there might be only one parse, then a series of Bind and Execute
messages, or there may be repeated Parse messages.

Personally I'd be fairly happy with statement-timeout applying per-message
in the extended query protocol. That would mean that it behaves slightly
differently, and a query with a long slow parse and bind phase followed by
quick execution might fail to time out in the extended query protocol where
it would time out as a simple query. It'd behave as if the query was
PREPAREd then separately EXECUTEd in simple-query protocol. I'm not hugely
bothered by that, but if it's really a concern I'd ideally like to add a
new protocol message that resets the statement timeout counter, so the
client can define what delimits a statement. Not practical in the near term.

For now I'd be OK with documenting this as a quirk/limitation of
statement_timeout, that it applies to a whole extended-query-protocol
batch.


-- 
 Craig Ringer   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Re: [HACKERS] Statement timeout

[Amit Kapila]

On Wed, Jun 1, 2016 at 6:03 AM, Tatsuo Ishii  wrote:
>
> From the document about statement_timeout (config.sgml):
>
> Abort any statement that takes more than the specified number of
> milliseconds, starting from the time the command arrives at the
server
> from the client.  If log_min_error_statement is set to
> ERROR or lower, the statement that timed out will
also be
> logged.  A value of zero (the default) turns this off.
>
> Apparently "starting from the time the command arrives at the server
> from the client" referrers to the timing of execute message arrives to
> server the in my example. And I think current behavior of our code is
> doing different from what he document advertises. Or at least counter
> intuitive to users.
>

It seems to me the above documentation is more relevant with respect to
simple query.  However, I agree that it is better if statement_timeout is
the timeout for each execution of the parsed statement.


With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Re: [HACKERS] Statement timeout

[Tatsuo Ishii]

> FWIW, I think the existing behavior is just fine.  It corresponds to what
> PQexec has always done with multi-statement query strings; that is,
> statement_timeout governs the total time to execute the transaction (the
> whole query string, unless you put transaction control commands in there).
> In extended query mode, since you can only put one textual query per Parse
> message, that maps to statement_timeout governing the total time from
> initial Parse to Sync.  Which is what it does.

I've never thought about that. And I cannot imagine anyone is using
that way in extended protocol to simulate multi-statement queries. Is
that documented somewhere?

> What you propose here is not a bug fix but a redefinition of what
> statement_timeout does; and you've made it inconsistent with simple query
> mode.  I don't really think it's an improvement.

>From the document about statement_timeout (config.sgml):

Abort any statement that takes more than the specified number of
milliseconds, starting from the time the command arrives at the server
from the client.  If log_min_error_statement is set to
ERROR or lower, the statement that timed out will also be
logged.  A value of zero (the default) turns this off.

Apparently "starting from the time the command arrives at the server
from the client" referrers to the timing of execute message arrives to
server the in my example. And I think current behavior of our code is
doing different from what he document advertises. Or at least counter
intuitive to users.

> BTW, aren't you missing a re-enable of the timeout for statements after
> the first one?

Will check.

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Statement timeout

[Tom Lane]

Tatsuo Ishii  writes:
>> Oops. Previous example was not appropriate. Here are revised
>> examples. (in any case, the time consumed at parse and bind are small,
>> and I omit the CHECK_FOR_INTERRUPTS after these commands)

FWIW, I think the existing behavior is just fine.  It corresponds to what
PQexec has always done with multi-statement query strings; that is,
statement_timeout governs the total time to execute the transaction (the
whole query string, unless you put transaction control commands in there).
In extended query mode, since you can only put one textual query per Parse
message, that maps to statement_timeout governing the total time from
initial Parse to Sync.  Which is what it does.

What you propose here is not a bug fix but a redefinition of what
statement_timeout does; and you've made it inconsistent with simple query
mode.  I don't really think it's an improvement.

BTW, aren't you missing a re-enable of the timeout for statements after
the first one?

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Statement timeout

[Tatsuo Ishii]

> Oops. Previous example was not appropriate. Here are revised
> examples. (in any case, the time consumed at parse and bind are small,
> and I omit the CHECK_FOR_INTERRUPTS after these commands)
> 
> [example 1]
> 
> statement_timeout = 3s
> 
> parse(statement1) -- time 0. set statement timout timer
> bind(statement1, portal1)
> execute(portal1) -- took 2 seconds
> CHECK_FOR_INTERRUPTS -- 2 seconds passed since time 0. statement timeout does 
> not fire
> parse(statement2)
> bind(statement2, portal2)
> execute(portal2) -- took 2 seconds
> CHECK_FOR_INTERRUPTS -- 4 seconds passed since time 0. the statement timeout 
> fires!
> 
> Another example.
> 
> [example 2]
> 
> statement_timeout = 3s
> 
> parse(statement1) -- time 0. set statement timout timer
> bind(statement1, portal1)
> execute(portal1) -- took 1 second
> CHECK_FOR_INTERRUPTS -- 1 second passed since time 0. statement timeout does 
> not fire
> parse(statement2)
> bind(statement2, portal2)
> execute(portal2) -- took 1 second
> CHECK_FOR_INTERRUPTS -- 2 seconds passed since time 0. the statement timeout 
> fires!
> [client is idle for 2 seconds]
> sync
> CHECK_FOR_INTERRUPTS -- 4 seconds passed since time 0. the statement timeout 
> fires!
> 
> I think current code is good in detecting statement timeout if each
> command execution time actually exceeds the specified timeout. However
> it could report false statement timeout in some cases like above.

Here is the patch against master branch to deal with the problem. I
create new functions in tcop/postgres.c:

static void enable_statement_timeout(void);
static void disable_statement_timeout(void);

Before the code was in start_xact_command() and finish_xact_command()
but I want to manage to disable timeout in exec_execute_command, so I
separated the code into the new functions.

Also start_xact_command() and finish_xact_command() were modified to
use these new functions to avoid code duplication.

I tested the patch using small C program linked with modified libpq
(PQsendQueryParams was modified to issue "flush" message instead of
"sync" message). The statement timeout was set to 3 seconds. With
these test programs, client sends pg_sleep(2) and flush message then
sleep 10 seconds. With current PostgreSQL, this triggers statement
timeout while the client is sleeping. With patched PostgreSQL, this
does not trigger the timeout as expected.

All regression tests passed.

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 b185c1b..564855a 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 st_timeout = 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 *parseTrees);
 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);
 
 
 /* 
@@ -2002,6 +2009,11 @@ exec_execute_message(const char *portal_name, long max_rows)
 			 * those that start or end a transaction block.
 			 */
 			CommandCounterIncrement();
+
+			/*
+			 * We need to reset statement timeout if already set.
+			 */
+			disable_statement_timeout();
 		}
 
 		/* Send appropriate CommandComplete to client */
@@ -2433,14 +2445,10 @@ start_xact_command(void)
 (errmsg_internal("StartTransactionCommand")));
 		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();
 	}
 }
 
@@ -2450,7 +2458,7 @@ finish_xact_command(void)
 	if (xact_started)
 	{
 		/* Cancel any active statement timeout before committing */
-		disable_timeout(STATEMENT_TIMEOUT, false);
+		disable_statement_timeout();
 
 		/* Now commit the command */
 		ereport(DEBUG3,
@@ -4510,3 +4518,51 @@ 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 (!st_timeout)
+	{
+		if (StatementTimeout > 0)
+		{
+
+			/*
+			 * Sanity check

Re: [HACKERS] Statement timeout

[Tatsuo Ishii]

> Really?  It should get reported at some execution of CHECK_FOR_INTERRUPTS
> after we pass the 2-second mark in execute(portal1).  If that's not what
> you're observing, maybe you've found a code path that's missing some
> CHECK_FOR_INTERRUPTS call(s).

Oops. Previous example was not appropriate. Here are revised
examples. (in any case, the time consumed at parse and bind are small,
and I omit the CHECK_FOR_INTERRUPTS after these commands)

[example 1]

statement_timeout = 3s

parse(statement1) -- time 0. set statement timout timer
bind(statement1, portal1)
execute(portal1) -- took 2 seconds
CHECK_FOR_INTERRUPTS -- 2 seconds passed since time 0. statement timeout does 
not fire
parse(statement2)
bind(statement2, portal2)
execute(portal2) -- took 2 seconds
CHECK_FOR_INTERRUPTS -- 4 seconds passed since time 0. the statement timeout 
fires!

Another example.

[example 2]

statement_timeout = 3s

parse(statement1) -- time 0. set statement timout timer
bind(statement1, portal1)
execute(portal1) -- took 1 second
CHECK_FOR_INTERRUPTS -- 1 second passed since time 0. statement timeout does 
not fire
parse(statement2)
bind(statement2, portal2)
execute(portal2) -- took 1 second
CHECK_FOR_INTERRUPTS -- 2 seconds passed since time 0. the statement timeout 
fires!
[client is idle for 2 seconds]
sync
CHECK_FOR_INTERRUPTS -- 4 seconds passed since time 0. the statement timeout 
fires!

I think current code is good in detecting statement timeout if each
command execution time actually exceeds the specified timeout. However
it could report false statement timeout in some cases like above.

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Statement timeout

[Tom Lane]

Tatsuo Ishii  writes:
> When extended query protocol message is used, statement timeout is not
> checked until a sync message is received (more precisely, statement
> timeout timer is canceled while processing the sync message, and
> actual checking timeout is done in CHECK_FOR_INTERRUPTS). Example:

> parse(statement1)
> bind(statement1, portal1)
> execute(portal1)
> parse(statement2)
> bind(statement2, portal2)
> execute(portal2)
> sync

> Suppose statement_timeout = 2s. If execute(portal1) takes 3 seconds,
> and execute(portal2) takes 1 second, the statement timeout is reported
> at the time when the sync message is processed which is right after
> execute(portal2).

Really?  It should get reported at some execution of CHECK_FOR_INTERRUPTS
after we pass the 2-second mark in execute(portal1).  If that's not what
you're observing, maybe you've found a code path that's missing some
CHECK_FOR_INTERRUPTS call(s).

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Statement timeout logging

[Thom Brown]

On 6 June 2013 17:28, Pavel Stehule pavel.steh...@gmail.com wrote:

 2013/6/6 Thom Brown t...@linux.com:
  Hi,
 
  When a statement is cancelled due to it running for long enough for
  statement_timeout to take effect, it logs a message:
 
  ERROR:  canceling statement due to statement timeout
 
  However, it doesn't log what the timeout was at the time of the
  cancellation.  This may be set in postgresql.conf, the database, or on
  the role, but unless log_line_prefix is set to show the database name
  and the user name, there's no reliable way of finding out what context
  the configuration applied from.  Setting log_duration won't help
  either because that only logs the duration of completed queries.
 
  Should we output the statement_timeout value when a query is cancelled?

 +1

 we use same feature in GoodData. Our long queries are cancelled by
 users and we should to known how much a users would to wait.


It seems there are a couple more errors that could share this sort of
information too:

canceling authentication due to timeout
canceling statement due to lock timeout

Admittedly the first of those two isn't really an issue.
-- 
Thom

Re: [HACKERS] Statement timeout logging

[Pavel Stehule]

2013/6/6 Thom Brown t...@linux.com:
 Hi,

 When a statement is cancelled due to it running for long enough for
 statement_timeout to take effect, it logs a message:

 ERROR:  canceling statement due to statement timeout

 However, it doesn't log what the timeout was at the time of the
 cancellation.  This may be set in postgresql.conf, the database, or on
 the role, but unless log_line_prefix is set to show the database name
 and the user name, there's no reliable way of finding out what context
 the configuration applied from.  Setting log_duration won't help
 either because that only logs the duration of completed queries.

 Should we output the statement_timeout value when a query is cancelled?

+1

we use same feature in GoodData. Our long queries are cancelled by
users and we should to known how much a users would to wait.

Regards

Pavel


 --
 Thom


 --
 Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
 To make changes to your subscription:
 http://www.postgresql.org/mailpref/pgsql-hackers


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] statement timeout vs dump/restore

[Robert Treat]

On Monday 05 May 2008 09:01:25 Andrew Dunstan wrote:
 Zeugswetter Andreas OSB sIT wrote:
  Do we want the following:
 
  1. pg_dump issues set statement_timeout = 0; to the
 
  database prior to
 
  taking its copy of data (yes/no/default-but-switchable)
  2. pg_dump/pg_restore issue set statement_timeout = 0; in
 
  text mode
 
  output (yes/no/default-but-switchable)
  3. pg_restore issues set statement_timeout = 0; to the
 
  database in
 
  restore mode (yes/no/default-but-switchable)
 
  I think yes for all three.  There was some handwaving about someone
  maybe not wanting it, but an utter lack of convincing use-cases; so
  I see no point in going to the effort of providing a switch.
 

ISTR being unconvinced by the pg_restore arguments, but as I think about it 
some more, for someone to set statement_timeout on a production system, and 
then have that be blindly overridden by any random pg_dump user seems a bit 
unfair.  pg_dump is not only used as a backup tool, it is also used as a 
general user tool (for example, pgadmin calls pg_dump if you want to see a 
tables schema). imho pg_dump should not set it by default, but have an option 
to set it, specifically for the backup scenario. 

-- 
Robert Treat
Build A Brighter LAMP :: Linux Apache {middleware} PostgreSQL

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] statement timeout vs dump/restore

[Tom Lane]

Robert Treat [EMAIL PROTECTED] writes:
 ISTR being unconvinced by the pg_restore arguments, but as I think about it 
 some more, for someone to set statement_timeout on a production system, and 
 then have that be blindly overridden by any random pg_dump user seems a bit 
 unfair.  pg_dump is not only used as a backup tool, it is also used as a 
 general user tool (for example, pgadmin calls pg_dump if you want to see a 
 tables schema).

So?  In those usages, it's not going to run long enough to have a
statement_timeout problem anyway.

When there is a data dump involved, you still have to defend the
proposition that it's okay for pg_dump to deliver a bad dump if
statement_timeout hits it.  I can't accept that.

regards, tom lane

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] statement timeout vs dump/restore

[Andrew Dunstan]

Tom Lane wrote:

Robert Treat [EMAIL PROTECTED] writes:
  
ISTR being unconvinced by the pg_restore arguments, but as I think about it 
some more, for someone to set statement_timeout on a production system, and 
then have that be blindly overridden by any random pg_dump user seems a bit 
unfair.  pg_dump is not only used as a backup tool, it is also used as a 
general user tool (for example, pgadmin calls pg_dump if you want to see a 
tables schema).



So?  In those usages, it's not going to run long enough to have a
statement_timeout problem anyway.

When there is a data dump involved, you still have to defend the
proposition that it's okay for pg_dump to deliver a bad dump if
statement_timeout hits it.  I can't accept that.


  


I agree.

What is more, the solution to the non-dump uses of pg_dump is to put 
that functionality in a library where clients can call it directly 
rather than using pg_dump.


cheers

andrew

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] statement timeout vs dump/restore

[Zeugswetter Andreas OSB sIT]

  Do we want the following:
 
  1. pg_dump issues set statement_timeout = 0; to the 
 database prior to 
  taking its copy of data (yes/no/default-but-switchable)
  2. pg_dump/pg_restore issue set statement_timeout = 0; in 
 text mode 
  output (yes/no/default-but-switchable)
  3. pg_restore issues set statement_timeout = 0; to the 
 database in 
  restore mode (yes/no/default-but-switchable)
 
 I think yes for all three.  There was some handwaving about someone
 maybe not wanting it, but an utter lack of convincing use-cases; so
 I see no point in going to the effort of providing a switch.
 
 Note that 2 and 3 are actually the same thing (if you think they are
 not, then you are putting the behavior in the wrong place).

I thought a proper fix for 3 would not depend on 2 ?

Andreas

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] statement timeout vs dump/restore

[Andrew Dunstan]

Zeugswetter Andreas OSB sIT wrote:

Do we want the following:
  
1. pg_dump issues set statement_timeout = 0; to the 
  
database prior to 


taking its copy of data (yes/no/default-but-switchable)
2. pg_dump/pg_restore issue set statement_timeout = 0; in 
  
text mode 


output (yes/no/default-but-switchable)
3. pg_restore issues set statement_timeout = 0; to the 
  
database in 


restore mode (yes/no/default-but-switchable)
  

I think yes for all three.  There was some handwaving about someone
maybe not wanting it, but an utter lack of convincing use-cases; so
I see no point in going to the effort of providing a switch.

Note that 2 and 3 are actually the same thing (if you think they are
not, then you are putting the behavior in the wrong place).



I thought a proper fix for 3 would not depend on 2 ?


  


I'm sure we could separate the two if we wanted to. Since we don't it's 
been put in the most natural spot, which does both.


cheers

andrew

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] statement timeout vs dump/restore

[Tom Lane]

Andrew Dunstan [EMAIL PROTECTED] writes:
 Do we want the following:

 1. pg_dump issues set statement_timeout = 0; to the database prior to 
 taking its copy of data (yes/no/default-but-switchable)
 2. pg_dump/pg_restore issue set statement_timeout = 0; in text mode 
 output (yes/no/default-but-switchable)
 3. pg_restore issues set statement_timeout = 0; to the database in 
 restore mode (yes/no/default-but-switchable)

I think yes for all three.  There was some handwaving about someone
maybe not wanting it, but an utter lack of convincing use-cases; so
I see no point in going to the effort of providing a switch.

Note that 2 and 3 are actually the same thing (if you think they are
not, then you are putting the behavior in the wrong place).

regards, tom lane

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] statement timeout vs dump/restore

[Joshua D. Drake]

Tom Lane wrote:

Andrew Dunstan [EMAIL PROTECTED] writes:

Do we want the following:


1. pg_dump issues set statement_timeout = 0; to the database prior to 
taking its copy of data (yes/no/default-but-switchable)
2. pg_dump/pg_restore issue set statement_timeout = 0; in text mode 
output (yes/no/default-but-switchable)
3. pg_restore issues set statement_timeout = 0; to the database in 
restore mode (yes/no/default-but-switchable)


I think yes for all three.  There was some handwaving about someone
maybe not wanting it, but an utter lack of convincing use-cases; so
I see no point in going to the effort of providing a switch.

Note that 2 and 3 are actually the same thing (if you think they are
not, then you are putting the behavior in the wrong place).


Right, pg_restore just using the output from pg_dump. The dump has the 
statement_timeout. That way it works regardless of output (e.g; for psql 
text based restores).


Sincerely,

Joshua D. Drake



regards, tom lane




--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers