Subject: Re: Transaction timeout

From: "Andrey M. Borodin" <[email-masked]>
Date: Mon, 18 Dec 2023 10:49:23 +0500

> On 16 Dec 2023, at 05:58, Japin Li <[email-masked]> wrote:
> 
> 
> On Fri, 15 Dec 2023 at 17:51, Andrey M. Borodin <[email-masked]> wrote:
>>> On 8 Dec 2023, at 15:29, Japin Li <[email-masked]> wrote:
>>> 
>>> Thanks for updating the patch. LGTM.
>> 
>> PFA v9. Changes:
>> 1. Added tests for idle_in_transaction_timeout
>> 2. Suppress statement_timeout if it’s shorter than transaction_timeout
>> 
> +       if (StatementTimeout > 0
> +               && IdleInTransactionSessionTimeout < TransactionTimeout)
>                   ^
> 
> Should be StatementTimeout?
Yes, that’s an oversight. I’ve adjusted tests so they catch this problem.

> Maybe we should add documentation to describe this behavior.

I've added a paragraph about it to config.sgml, but I'm not sure about the comprehensiveness of the wording.

Best regards, Andrey Borodin.

---

From: Japin Li <[email-masked]>
Date: Mon, 18 Dec 2023 17:32:22 +0800

> I've added a paragraph about it to config.sgml, but I'm not sure about the comprehensiveness of the wording.

Thanks for updating the patch, no objections.

--
Regrads,
Japin Li
ChengDu WenWu Information Technology Co., Ltd.

---

From: "Andrey M. Borodin" <[email-masked]>
Date: Mon, 18 Dec 2023 14:40:31 +0500

> On 18 Dec 2023, at 14:32, Japin Li <[email-masked]> wrote:
> 
> 
> Thanks for updating the patch

Sorry for the noise, but commitfest bot found one more bug in handling statement timeout. PFA v11.

---

From: Japin Li <[email-masked]>
Date: Tue, 19 Dec 2023 09:25:27 +0800

> Sorry for the noise, but commitfest bot found one more bug in handling statement timeout. PFA v11.

On Windows, there still have an error:

diff -w -U3 C:/cirrus/src/test/isolation/expected/timeouts.out C:/cirrus/build/testrun/isolation/isolation/results/timeouts.out
--- C:/cirrus/src/test/isolation/expected/timeouts.out	2023-12-18 10:22:21.772537200 +0000
+++ C:/cirrus/build/testrun/isolation/isolation/results/timeouts.out	2023-12-18 10:26:08.039831800 +0000
@@ -103,24 +103,7 @@
     0
 (1 row)

-step stt2_check: SELECT 1;
-FATAL:  terminating connection due to transaction timeout
-server closed the connection unexpectedly
+PQconsumeInput failed: server closed the connection unexpectedly
 	This probably means the server terminated abnormally
 	before or while processing the request.

-step itt4_set: SET idle_in_transaction_session_timeout = '1ms'; SET statement_timeout = '10s'; SET lock_timeout = '10s'; SET transaction_timeout = '10s';
-step itt4_begin: BEGIN ISOLATION LEVEL READ COMMITTED;
-step sleep_there: SELECT pg_sleep(0.01);
-pg_sleep
---------
-
-(1 row)
-
-step stt3_check_itt4: SELECT count(*) FROM pg_stat_activity WHERE application_name = 'isolation/timeouts/itt4' <waiting ...>
-step stt3_check_itt4: <... completed>
-count
------
-    0
-(1 row)
-

---

From: "Andrey M. Borodin" <[email-masked]>
Date: Tue, 19 Dec 2023 13:26:15 +0500

> On 19 Dec 2023, at 06:25, Japin Li <[email-masked]> wrote:
> 

Uhhmm, yes. Connection termination looks different on windows machine.
I’ve checked how this looks in relication slot tests and removed select that was observing connection failure.
I don’t have Windows machine, so I hope CF bot will pick this.

---

From: "Andrey M. Borodin" <[email-masked]>
Date: Tue, 19 Dec 2023 15:27:23 +0500

> On 19 Dec 2023, at 13:26, Andrey M. Borodin <[email-masked]> wrote:
> 

I used Github CI to produce version of tests that seems to be is stable on Windows.
Sorry for the noise.

---

From: Japin Li <[email-masked]>
Date: Tue, 19 Dec 2023 22:06:21 +0800

> I used Github CI to produce version of tests that seems to be is stable on Windows.

It still failed on Windows Server 2019 [1].

diff -w -U3 C:/cirrus/src/test/isolation/expected/timeouts.out C:/cirrus/build/testrun/isolation/isolation/results/timeouts.out
--- C:/cirrus/src/test/isolation/expected/timeouts.out	2023-12-19 10:34:30.354721100 +0000
+++ C:/cirrus/build/testrun/isolation/isolation/results/timeouts.out	2023-12-19 10:38:25.877981600 +0000
@@ -100,7 +100,7 @@
 step stt3_check_stt2: SELECT count(*) FROM pg_stat_activity WHERE application_name = 'isolation/timeouts/stt2'
 count
 -----
-    0
+    1
 (1 row)

 step itt4_set: SET idle_in_transaction_session_timeout = '1ms'; SET statement_timeout = '10s'; SET lock_timeout = '10s'; SET transaction_timeout = '10s';

[1] https://api.cirrus-ci.com/v1/artifact/task/4707530400595968/testrun/build/testrun/isolation/isolation/regression.diffs

---

From: Junwang Zhao <[email-masked]>
Date: Tue, 19 Dec 2023 22:51:03 +0800

> Best regards, Andrey Borodin.

+       <para>
+        If <varname>transaction_timeout</varname> is shorter than
+        <varname>idle_in_transaction_session_timeout</varname> or
<varname>statement_timeout</varname>
+        <varname>transaction_timeout</varname> will invalidate longer timeout.
+       </para>

When transaction_timeout is *equal* to idle_in_transaction_session_timeout
or statement_timeout, idle_in_transaction_session_timeout and statement_timeout
will also be invalidated, the logic in the code seems right, though
this document
is a little bit inaccurate.

-- 
Regards
Junwang Zhao

---

From: Junwang Zhao <[email-masked]>
Date: Wed, 20 Dec 2023 09:48:59 +0800

> is a little bit inaccurate.
       <para>
        Unlike <varname>statement_timeout</varname>, this timeout can only occur
        while waiting for locks.  Note that if
<varname>statement_timeout</varname>
        is nonzero, it is rather pointless to set
<varname>lock_timeout</varname> to
        the same or larger value, since the statement timeout would always
        trigger first.  If <varname>log_min_error_statement</varname> is set to
        <literal>ERROR</literal> or lower, the statement that timed out will be
        logged.
       </para>

There is a note about statement_timeout and lock_timeout, set both
and lock_timeout >= statement_timeout is pointless, but this logic seems not
implemented in the code. I am wondering if lock_timeout >= transaction_timeout,
should we invalidate lock_timeout? Or maybe just document this.

-- 
Regards
Junwang Zhao

---

From: Thomas wen <[email-masked]>
Date: Wed, 20 Dec 2023 01:58:47 +0000

Hi Junwang Zhao
     #should we invalidate lock_timeout? Or maybe just document this.
I think you mean when lock_time is greater than trasaction-time invalidate lock_timeout or  needs to be logged ?

Best whish
________________________________
发件人: Junwang Zhao <[email-masked]>
发送时间: 2023年12月20日 9:48
收件人: Andrey M. Borodin <[email-masked]>
抄送: Japin Li <[email-masked]>; 邱宇航 <[email-masked]>; Fujii Masao <[email-masked]>; Andrey Borodin <[email-masked]>; Andres Freund <[email-masked]>; Michael Paquier <[email-masked]>; Nikolay Samokhvalov <[email-masked]>; pgsql-hackers <[email-masked]>; [email-masked] <[email-masked]>
主题: Re: Transaction timeout

       <para>

--
Regards
Junwang Zhao

---

From: Junwang Zhao <[email-masked]>
Date: Wed, 20 Dec 2023 10:35:16 +0800

> I think you mean when lock_time is greater than trasaction-time invalidate lock_timeout or  needs to be logged ?
I mean the interleaving of the gucs, which is lock_timeout and the new
introduced transaction_timeout,
if lock_timeout >= transaction_timeout, seems no need to enable lock_timeout.

-- 
Regards
Junwang Zhao

---

From: wenhui qiu <[email-masked]>
Date: Wed, 20 Dec 2023 20:23:13 +0800

Hi Junwang Zhao
   Agree +1

Best whish

Junwang Zhao <[email-masked]> 于2023年12月20日周三 10:35写道：

> > I think you mean when lock_time is greater than trasaction-time
> invalidate lock_timeout or  needs to be logged ?
> >
> > 抄送: Japin Li <[email-masked]>; 邱宇航 <[email-masked]>; Fujii Masao
> <[email-masked]>; Andrey Borodin <[email-masked]>;
> Andres Freund <[email-masked]>; Michael Paquier <
> [email-masked]>; Nikolay Samokhvalov <[email-masked]>;
> pgsql-hackers <[email-masked]>;
> [email-masked] <[email-masked]>

---

From: Japin Li <[email-masked]>
Date: Fri, 22 Dec 2023 13:39:27 +0800

> +++ C:/cirrus/build/testrun/isolation/isolation/results/timeouts.out	2023-12-19 10:38:25.877981600 +0000

Hi,

I try to split the test for transaction timeout, and all passed on my CI [1].

OTOH, I find if I set transaction_timeout in a transaction, it will not take
effect immediately.  For example:

[local]:2049802 postgres=# BEGIN;
BEGIN
[local]:2049802 postgres=*# SET transaction_timeout TO '1s';
SET
[local]:2049802 postgres=*# SELECT relname FROM pg_class LIMIT 1;  -- wait 10s
   relname
--------------
 pg_statistic
(1 row)

[local]:2049802 postgres=*# SELECT relname FROM pg_class LIMIT 1;
FATAL:  terminating connection due to transaction timeout
server closed the connection unexpectedly
        This probably means the server terminated abnormally
        before or while processing the request.
The connection to the server was lost. Attempting reset: Succeeded.

It looks odd.  Does this is expected? I'm not read all the threads,
am I missing something?

[1] https://cirrus-ci.com/build/6574686130143232

>From fb87e5fe2ea5ced51a7e443243cdd40115423449 Mon Sep 17 00:00:00 2001
From: "Andrey M. Borodin" <[email-masked]>
Date: Sun, 3 Dec 2023 23:18:00 +0500
Subject: [PATCH v13 1/1] Introduce transaction_timeout

This commit adds timeout that is expected to be used as a prevention
of long-running queries. Any session within transaction will be
terminated after spanning longer than this timeout.

However, this timeout is not applied to prepared transactions.
Only transactions with user connections are affected.

Author: Andrey Borodin <[email-masked]>
Reviewed-by: Nikolay Samokhvalov <[email-masked]>
Reviewed-by: Andres Freund <[email-masked]>
Reviewed-by: Fujii Masao <[email-masked]>
Reviewed-by: bt23nguyent <[email-masked]>
Reviewed-by: Yuhang Qiu <[email-masked]>
Reviewed-by: Japin Li <[email-masked]>

Discussion: https://postgr.es/m/CAAhFRxiQsRs2Eq5kCo9nXE3HTugsAAJdSQSmxncivebAxdmBjQ%40mail.gmail.com
---
 doc/src/sgml/config.sgml                      | 35 +++++++++++
 src/backend/postmaster/autovacuum.c           |  2 +
 src/backend/storage/lmgr/proc.c               |  1 +
 src/backend/tcop/postgres.c                   | 27 +++++++-
 src/backend/utils/errcodes.txt                |  1 +
 src/backend/utils/init/globals.c              |  1 +
 src/backend/utils/init/postinit.c             | 10 +++
 src/backend/utils/misc/guc_tables.c           | 11 ++++
 src/backend/utils/misc/postgresql.conf.sample |  1 +
 src/bin/pg_dump/pg_backup_archiver.c          |  2 +
 src/bin/pg_dump/pg_dump.c                     |  2 +
 src/bin/pg_rewind/libpq_source.c              |  1 +
 src/include/miscadmin.h                       |  1 +
 src/include/storage/proc.h                    |  1 +
 src/include/utils/timeout.h                   |  1 +
 src/test/isolation/Makefile                   |  5 +-
 src/test/isolation/expected/timeouts.out      | 63 ++++++++++++++++++-
 src/test/isolation/specs/timeouts.spec        | 30 +++++++++
 18 files changed, 190 insertions(+), 5 deletions(-)

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index b5624ca884..d62edcf83b 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -9134,6 +9134,41 @@ COPY postgres_log FROM '/full/path/to/logfile.csv' WITH csv;
       </listitem>
      </varlistentry>
 
+     <varlistentry id="guc-transaction-timeout" xreflabel="transaction_timeout">
+      <term><varname>transaction_timeout</varname> (<type>integer</type>)
+      <indexterm>
+       <primary><varname>transaction_timeout</varname> configuration parameter</primary>
+      </indexterm>
+      </term>
+      <listitem>
+       <para>
+        Terminate any session that spans longer than the specified amount of
+        time in transaction. The limit applies both to explicit transactions
+        (started with <command>BEGIN</command>) and to implicitly started
+        transaction corresponding to single statement. But this limit is not
+        applied to prepared transactions.
+        If this value is specified without units, it is taken as milliseconds.
+        A value of zero (the default) disables the timeout.
+       </para>
+
+       <para>
+        If <varname>transaction_timeout</varname> is shorter than
+        <varname>idle_in_transaction_session_timeout</varname> or <varname>statement_timeout</varname>
+        <varname>transaction_timeout</varname> will invalidate longer timeout.
+       </para>
+
+       <para>
+        Setting <varname>transaction_timeout</varname> in
+        <filename>postgresql.conf</filename> is not recommended because it would
+        affect all sessions.
+       </para>
+
+       <para>
+        Prepared transactions are not subject for this timeout.
+       </para>
+      </listitem>
+     </varlistentry>
+
      <varlistentry id="guc-lock-timeout" xreflabel="lock_timeout">
       <term><varname>lock_timeout</varname> (<type>integer</type>)
       <indexterm>
diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index b04fcfc8c8..e6fa1cfdc2 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -586,6 +586,7 @@ AutoVacLauncherMain(int argc, char *argv[])
 	 * regular maintenance from being executed.
 	 */
 	SetConfigOption("statement_timeout", "0", PGC_SUSET, PGC_S_OVERRIDE);
+	SetConfigOption("transaction_timeout", "0", PGC_SUSET, PGC_S_OVERRIDE);
 	SetConfigOption("lock_timeout", "0", PGC_SUSET, PGC_S_OVERRIDE);
 	SetConfigOption("idle_in_transaction_session_timeout", "0",
 					PGC_SUSET, PGC_S_OVERRIDE);
@@ -1591,6 +1592,7 @@ AutoVacWorkerMain(int argc, char *argv[])
 	 * regular maintenance from being executed.
 	 */
 	SetConfigOption("statement_timeout", "0", PGC_SUSET, PGC_S_OVERRIDE);
+	SetConfigOption("transaction_timeout", "0", PGC_SUSET, PGC_S_OVERRIDE);
 	SetConfigOption("lock_timeout", "0", PGC_SUSET, PGC_S_OVERRIDE);
 	SetConfigOption("idle_in_transaction_session_timeout", "0",
 					PGC_SUSET, PGC_S_OVERRIDE);
diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c
index b6451d9d08..4be06c1e5d 100644
--- a/src/backend/storage/lmgr/proc.c
+++ b/src/backend/storage/lmgr/proc.c
@@ -59,6 +59,7 @@ int			DeadlockTimeout = 1000;
 int			StatementTimeout = 0;
 int			LockTimeout = 0;
 int			IdleInTransactionSessionTimeout = 0;
+int			TransactionTimeout = 0;
 int			IdleSessionTimeout = 0;
 bool		log_lock_waits = false;
 
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 7298a187d1..a2611cf8e6 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -2745,6 +2745,10 @@ start_xact_command(void)
 	{
 		StartTransactionCommand();
 
+		/* Schedule or reschedule transaction timeout */
+		if (TransactionTimeout > 0)
+			enable_timeout_after(TRANSACTION_TIMEOUT, TransactionTimeout);
+
 		xact_started = true;
 	}
 
@@ -3426,6 +3430,17 @@ ProcessInterrupts(void)
 			IdleInTransactionSessionTimeoutPending = false;
 	}
 
+	if (TransactionTimeoutPending)
+	{
+		/* As above, ignore the signal if the GUC has been reset to zero. */
+		if (TransactionTimeout > 0)
+			ereport(FATAL,
+					(errcode(ERRCODE_TRANSACTION_TIMEOUT),
+					 errmsg("terminating connection due to transaction timeout")));
+		else
+			TransactionTimeoutPending = false;
+	}
+
 	if (IdleSessionTimeoutPending)
 	{
 		/* As above, ignore the signal if the GUC has been reset to zero. */
@@ -4491,7 +4506,8 @@ PostgresMain(const char *dbname, const char *username)
 				pgstat_report_activity(STATE_IDLEINTRANSACTION_ABORTED, NULL);
 
 				/* Start the idle-in-transaction timer */
-				if (IdleInTransactionSessionTimeout > 0)
+				if (IdleInTransactionSessionTimeout > 0
+					&& (IdleInTransactionSessionTimeout < TransactionTimeout || TransactionTimeout == 0))
 				{
 					idle_in_transaction_timeout_enabled = true;
 					enable_timeout_after(IDLE_IN_TRANSACTION_SESSION_TIMEOUT,
@@ -4504,7 +4520,8 @@ PostgresMain(const char *dbname, const char *username)
 				pgstat_report_activity(STATE_IDLEINTRANSACTION, NULL);
 
 				/* Start the idle-in-transaction timer */
-				if (IdleInTransactionSessionTimeout > 0)
+				if (IdleInTransactionSessionTimeout > 0
+					&& (IdleInTransactionSessionTimeout < TransactionTimeout || TransactionTimeout == 0))
 				{
 					idle_in_transaction_timeout_enabled = true;
 					enable_timeout_after(IDLE_IN_TRANSACTION_SESSION_TIMEOUT,
@@ -4562,6 +4579,9 @@ PostgresMain(const char *dbname, const char *username)
 					enable_timeout_after(IDLE_SESSION_TIMEOUT,
 										 IdleSessionTimeout);
 				}
+
+				if (get_timeout_active(TRANSACTION_TIMEOUT))
+					disable_timeout(TRANSACTION_TIMEOUT, false);
 			}
 
 			/* Report any recently-changed GUC options */
@@ -5120,7 +5140,8 @@ enable_statement_timeout(void)
 	/* must be within an xact */
 	Assert(xact_started);
 
-	if (StatementTimeout > 0)
+	if (StatementTimeout > 0
+		&& (StatementTimeout < TransactionTimeout || TransactionTimeout == 0))
 	{
 		if (!get_timeout_active(STATEMENT_TIMEOUT))
 			enable_timeout_after(STATEMENT_TIMEOUT, StatementTimeout);
diff --git a/src/backend/utils/errcodes.txt b/src/backend/utils/errcodes.txt
index 8e97a0150f..8f1157afee 100644
--- a/src/backend/utils/errcodes.txt
+++ b/src/backend/utils/errcodes.txt
@@ -252,6 +252,7 @@ Section: Class 25 - Invalid Transaction State
 25P01    E    ERRCODE_NO_ACTIVE_SQL_TRANSACTION                              no_active_sql_transaction
 25P02    E    ERRCODE_IN_FAILED_SQL_TRANSACTION                              in_failed_sql_transaction
 25P03    E    ERRCODE_IDLE_IN_TRANSACTION_SESSION_TIMEOUT                    idle_in_transaction_session_timeout
+25P04    E    ERRCODE_TRANSACTION_TIMEOUT                                    transaction_timeout
 
 Section: Class 26 - Invalid SQL Statement Name
 
diff --git a/src/backend/utils/init/globals.c b/src/backend/utils/init/globals.c
index 60bc1217fb..fd586c193c 100644
--- a/src/backend/utils/init/globals.c
+++ b/src/backend/utils/init/globals.c
@@ -33,6 +33,7 @@ volatile sig_atomic_t ProcDiePending = false;
 volatile sig_atomic_t CheckClientConnectionPending = false;
 volatile sig_atomic_t ClientConnectionLost = false;
 volatile sig_atomic_t IdleInTransactionSessionTimeoutPending = false;
+volatile sig_atomic_t TransactionTimeoutPending = false;
 volatile sig_atomic_t IdleSessionTimeoutPending = false;
 volatile sig_atomic_t ProcSignalBarrierPending = false;
 volatile sig_atomic_t LogMemoryContextPending = false;
diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c
index 552cf9d950..64be4de0c7 100644
--- a/src/backend/utils/init/postinit.c
+++ b/src/backend/utils/init/postinit.c
@@ -75,6 +75,7 @@ static void ShutdownPostgres(int code, Datum arg);
 static void StatementTimeoutHandler(void);
 static void LockTimeoutHandler(void);
 static void IdleInTransactionSessionTimeoutHandler(void);
+static void TransactionTimeoutHandler(void);
 static void IdleSessionTimeoutHandler(void);
 static void IdleStatsUpdateTimeoutHandler(void);
 static void ClientCheckTimeoutHandler(void);
@@ -764,6 +765,7 @@ InitPostgres(const char *in_dbname, Oid dboid,
 		RegisterTimeout(LOCK_TIMEOUT, LockTimeoutHandler);
 		RegisterTimeout(IDLE_IN_TRANSACTION_SESSION_TIMEOUT,
 						IdleInTransactionSessionTimeoutHandler);
+		RegisterTimeout(TRANSACTION_TIMEOUT, TransactionTimeoutHandler);
 		RegisterTimeout(IDLE_SESSION_TIMEOUT, IdleSessionTimeoutHandler);
 		RegisterTimeout(CLIENT_CONNECTION_CHECK_TIMEOUT, ClientCheckTimeoutHandler);
 		RegisterTimeout(IDLE_STATS_UPDATE_TIMEOUT,
@@ -1395,6 +1397,14 @@ LockTimeoutHandler(void)
 	kill(MyProcPid, SIGINT);
 }
 
+static void
+TransactionTimeoutHandler(void)
+{
+	TransactionTimeoutPending = true;
+	InterruptPending = true;
+	SetLatch(MyLatch);
+}
+
 static void
 IdleInTransactionSessionTimeoutHandler(void)
 {
diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c
index 9f59440526..c5cfbef02b 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -2556,6 +2556,17 @@ struct config_int ConfigureNamesInt[] =
 		NULL, NULL, NULL
 	},
 
+	{
+		{"transaction_timeout", PGC_USERSET, CLIENT_CONN_STATEMENT,
+			gettext_noop("Sets the maximum allowed time in a transaction with session (not a prepared transaction)."),
+			gettext_noop("A value of 0 turns off the timeout."),
+			GUC_UNIT_MS
+		},
+		&TransactionTimeout,
+		0, 0, INT_MAX,
+		NULL, NULL, NULL
+	},
+
 	{
 		{"idle_session_timeout", PGC_USERSET, CLIENT_CONN_STATEMENT,
 			gettext_noop("Sets the maximum allowed idle time between queries, when not in a transaction."),
diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample
index b2809c711a..0b37117eb7 100644
--- a/src/backend/utils/misc/postgresql.conf.sample
+++ b/src/backend/utils/misc/postgresql.conf.sample
@@ -700,6 +700,7 @@
 #default_transaction_deferrable = off
 #session_replication_role = 'origin'
 #statement_timeout = 0				# in milliseconds, 0 is disabled
+#transaction_timeout = 0			# in milliseconds, 0 is disabled
 #lock_timeout = 0				# in milliseconds, 0 is disabled
 #idle_in_transaction_session_timeout = 0	# in milliseconds, 0 is disabled
 #idle_session_timeout = 0			# in milliseconds, 0 is disabled
diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c
index 256d1e35a4..3342971bd0 100644
--- a/src/bin/pg_dump/pg_backup_archiver.c
+++ b/src/bin/pg_dump/pg_backup_archiver.c
@@ -3115,6 +3115,8 @@ _doSetFixedOutputState(ArchiveHandle *AH)
 	ahprintf(AH, "SET statement_timeout = 0;\n");
 	ahprintf(AH, "SET lock_timeout = 0;\n");
 	ahprintf(AH, "SET idle_in_transaction_session_timeout = 0;\n");
+	// TODO: AB: do we need spacial handling for this?
+	ahprintf(AH, "SET transaction_timeout = 0;\n");
 
 	/* Select the correct character set encoding */
 	ahprintf(AH, "SET client_encoding = '%s';\n",
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 8c0b5486b9..21bd16ef00 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -1251,6 +1251,8 @@ setup_connection(Archive *AH, const char *dumpencoding,
 		ExecuteSqlStatement(AH, "SET lock_timeout = 0");
 	if (AH->remoteVersion >= 90600)
 		ExecuteSqlStatement(AH, "SET idle_in_transaction_session_timeout = 0");
+	if (AH->remoteVersion >= 170000)
+		ExecuteSqlStatement(AH, "SET transaction_timeout = 0");
 
 	/*
 	 * Quote all identifiers, if requested.
diff --git a/src/bin/pg_rewind/libpq_source.c b/src/bin/pg_rewind/libpq_source.c
index 417c74cfef..9cda3f3667 100644
--- a/src/bin/pg_rewind/libpq_source.c
+++ b/src/bin/pg_rewind/libpq_source.c
@@ -117,6 +117,7 @@ init_libpq_conn(PGconn *conn)
 	run_simple_command(conn, "SET statement_timeout = 0");
 	run_simple_command(conn, "SET lock_timeout = 0");
 	run_simple_command(conn, "SET idle_in_transaction_session_timeout = 0");
+	run_simple_command(conn, "SET transaction_timeout = 0");
 
 	/*
 	 * we don't intend to do any updates, put the connection in read-only mode
diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h
index 74bc2f97cb..b2d0f84252 100644
--- a/src/include/miscadmin.h
+++ b/src/include/miscadmin.h
@@ -91,6 +91,7 @@ extern PGDLLIMPORT volatile sig_atomic_t InterruptPending;
 extern PGDLLIMPORT volatile sig_atomic_t QueryCancelPending;
 extern PGDLLIMPORT volatile sig_atomic_t ProcDiePending;
 extern PGDLLIMPORT volatile sig_atomic_t IdleInTransactionSessionTimeoutPending;
+extern PGDLLIMPORT volatile sig_atomic_t TransactionTimeoutPending;
 extern PGDLLIMPORT volatile sig_atomic_t IdleSessionTimeoutPending;
 extern PGDLLIMPORT volatile sig_atomic_t ProcSignalBarrierPending;
 extern PGDLLIMPORT volatile sig_atomic_t LogMemoryContextPending;
diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
index e87fd25d64..9dde9cbfdd 100644
--- a/src/include/storage/proc.h
+++ b/src/include/storage/proc.h
@@ -429,6 +429,7 @@ extern PGDLLIMPORT int DeadlockTimeout;
 extern PGDLLIMPORT int StatementTimeout;
 extern PGDLLIMPORT int LockTimeout;
 extern PGDLLIMPORT int IdleInTransactionSessionTimeout;
+extern PGDLLIMPORT int TransactionTimeout;
 extern PGDLLIMPORT int IdleSessionTimeout;
 extern PGDLLIMPORT bool log_lock_waits;
 
diff --git a/src/include/utils/timeout.h b/src/include/utils/timeout.h
index 8a61853371..608a83d5a8 100644
--- a/src/include/utils/timeout.h
+++ b/src/include/utils/timeout.h
@@ -31,6 +31,7 @@ typedef enum TimeoutId
 	STANDBY_TIMEOUT,
 	STANDBY_LOCK_TIMEOUT,
 	IDLE_IN_TRANSACTION_SESSION_TIMEOUT,
+	TRANSACTION_TIMEOUT,
 	IDLE_SESSION_TIMEOUT,
 	IDLE_STATS_UPDATE_TIMEOUT,
 	CLIENT_CONNECTION_CHECK_TIMEOUT,
diff --git a/src/test/isolation/Makefile b/src/test/isolation/Makefile
index ade2256ed3..2bd06f8f15 100644
--- a/src/test/isolation/Makefile
+++ b/src/test/isolation/Makefile
@@ -62,7 +62,7 @@ installcheck: all
 	$(pg_isolation_regress_installcheck) --schedule=$(srcdir)/isolation_schedule
 
 check: all
-	$(pg_isolation_regress_check) --schedule=$(srcdir)/isolation_schedule
+	$(pg_isolation_regress_check) timeouts
 
 # Non-default tests.  It only makes sense to run these if set up to use
 # prepared transactions, via TEMP_CONFIG for the check case, or via the
@@ -72,3 +72,6 @@ installcheck-prepared-txns: all temp-install
 
 check-prepared-txns: all temp-install
 	$(pg_isolation_regress_check) --schedule=$(srcdir)/isolation_schedule prepared-transactions prepared-transactions-cic
+
+check-timeouts: all temp-install
+	$(pg_isolation_regress_check) timeouts
diff --git a/src/test/isolation/expected/timeouts.out b/src/test/isolation/expected/timeouts.out
index 9328676f1c..5b06148cee 100644
--- a/src/test/isolation/expected/timeouts.out
+++ b/src/test/isolation/expected/timeouts.out
@@ -1,4 +1,4 @@
-Parsed test spec with 2 sessions
+Parsed test spec with 6 sessions
 
 starting permutation: rdtbl sto locktbl
 step rdtbl: SELECT * FROM accounts;
@@ -79,3 +79,64 @@ step slto: SET lock_timeout = '10s'; SET statement_timeout = '10ms';
 step update: DELETE FROM accounts WHERE accountid = 'checking'; <waiting ...>
 step update: <... completed>
 ERROR:  canceling statement due to statement timeout
+
+starting permutation: stto s3_begin sleep s3_check abort
+step stto: SET statement_timeout = '1ms'; SET transaction_timeout = '1s';
+step s3_begin: BEGIN ISOLATION LEVEL READ COMMITTED;
+step sleep: SELECT pg_sleep(0.01);
+ERROR:  canceling statement due to statement timeout
+step s3_check: SELECT count(*) FROM pg_stat_activity WHERE application_name = 'isolation/timeouts/s3';
+count
+-----
+    1
+(1 row)
+
+step abort: ABORT;
+
+starting permutation: tsto s3_begin wait_check s3_check
+step tsto: SET statement_timeout = '1s'; SET transaction_timeout = '1ms';
+step s3_begin: BEGIN ISOLATION LEVEL READ COMMITTED;
+step wait_check: SELECT pg_sleep(0.01);
+pg_sleep
+--------
+        
+(1 row)
+
+step s3_check: SELECT count(*) FROM pg_stat_activity WHERE application_name = 'isolation/timeouts/s3';
+count
+-----
+    0
+(1 row)
+
+
+starting permutation: itto s4_begin wait_check s4_check
+step itto: SET idle_in_transaction_session_timeout = '1ms'; SET transaction_timeout = '1s';
+step s4_begin: BEGIN ISOLATION LEVEL READ COMMITTED;
+step wait_check: SELECT pg_sleep(0.01);
+pg_sleep
+--------
+        
+(1 row)
+
+step s4_check: SELECT count(*) FROM pg_stat_activity WHERE application_name = 'isolation/timeouts/s4';
+count
+-----
+    0
+(1 row)
+
+
+starting permutation: tito s5_begin wait_check s5_check
+step tito: SET idle_in_transaction_session_timeout = '1s'; SET transaction_timeout = '1ms';
+step s5_begin: BEGIN ISOLATION LEVEL READ COMMITTED;
+step wait_check: SELECT pg_sleep(0.01);
+pg_sleep
+--------
+        
+(1 row)
+
+step s5_check: SELECT count(*) FROM pg_stat_activity WHERE application_name = 'isolation/timeouts/s5';
+count
+-----
+    0
+(1 row)
+
diff --git a/src/test/isolation/specs/timeouts.spec b/src/test/isolation/specs/timeouts.spec
index c747b4ae28..0cca6ff147 100644
--- a/src/test/isolation/specs/timeouts.spec
+++ b/src/test/isolation/specs/timeouts.spec
@@ -27,6 +27,27 @@ step locktbl	{ LOCK TABLE accounts; }
 step update	{ DELETE FROM accounts WHERE accountid = 'checking'; }
 teardown	{ ABORT; }
 
+session s3
+step s3_begin	{ BEGIN ISOLATION LEVEL READ COMMITTED; }
+step stto	{ SET statement_timeout = '1ms'; SET transaction_timeout = '1s'; }
+step tsto	{ SET statement_timeout = '1s'; SET transaction_timeout = '1ms'; }
+step sleep	{ SELECT pg_sleep(0.01); }
+step abort	{ ABORT; }
+
+session s4
+step s4_begin	{ BEGIN ISOLATION LEVEL READ COMMITTED; }
+step itto	{ SET idle_in_transaction_session_timeout = '1ms'; SET transaction_timeout = '1s'; }
+
+session s5
+step s5_begin	{ BEGIN ISOLATION LEVEL READ COMMITTED; }
+step tito	{ SET idle_in_transaction_session_timeout = '1s'; SET transaction_timeout = '1ms'; }
+
+session s6
+step wait_check	{ SELECT pg_sleep(0.01); }
+step s3_check	{ SELECT count(*) FROM pg_stat_activity WHERE application_name = 'isolation/timeouts/s3'; }
+step s4_check	{ SELECT count(*) FROM pg_stat_activity WHERE application_name = 'isolation/timeouts/s4'; }
+step s5_check	{ SELECT count(*) FROM pg_stat_activity WHERE application_name = 'isolation/timeouts/s5'; }
+
 # It's possible that the isolation tester will not observe the final
 # steps as "waiting", thanks to the relatively short timeouts we use.
 # We can ensure consistent test output by marking those steps with (*).
@@ -47,3 +68,12 @@ permutation wrtbl lto update(*)
 permutation wrtbl lsto update(*)
 # statement timeout expires first, row-level lock
 permutation wrtbl slto update(*)
+
+# statement timeout expires first
+permutation stto s3_begin sleep s3_check abort
+# transaction timeout expires first, session s3 FATAL-out
+permutation tsto s3_begin wait_check s3_check
+# idle in transaction timeout expires first, session s4 FATAL-out
+permutation itto s4_begin wait_check s4_check
+# transaction timeout expires first, session s5 FATAL-out
+permutation tito s5_begin wait_check s5_check
-- 
2.34.1

---

From: Junwang Zhao <[email-masked]>
Date: Fri, 22 Dec 2023 20:29:58 +0800

> [local]:2049802 postgres=*# SET transaction_timeout TO '1s';
when this execute, TransactionTimeout is still 0, this command will
not set timeout
> [local]:2049802 postgres=*# SELECT relname FROM pg_class LIMIT 1;  -- wait 10s
when this command get execute, start_xact_command will enable the timer
>    relname

I think this is by design, if you debug statement_timeout, it's the same
behaviour, the timeout will be set for each command after the second
command was called, you just aren't aware of this.

I doubt people will set this in a transaction.

-- 
Regards
Junwang Zhao

---

From: Japin Li <[email-masked]>
Date: Fri, 22 Dec 2023 22:25:37 +0800

> when this command get execute, start_xact_command will enable the timer

Thanks for your exaplantion, got it.

> command was called, you just aren't aware of this.

I try to set idle_in_transaction_session_timeout after begin transaction,
it changes immediately, so I think transaction_timeout should also be take
immediately.

> I doubt people will set this in a transaction.

Maybe not,

---

From: Junwang Zhao <[email-masked]>
Date: Fri, 22 Dec 2023 22:37:40 +0800

> it changes immediately, so I think transaction_timeout should also be take

Ah, right, idle_in_transaction_session_timeout is set after the set
command finishes and before the backend send *ready for query*
to the client, so the value of the GUC is already set before
next command.

I bet you must have checked this ;)

-- 
Regards
Junwang Zhao

---

From: Japin Li <[email-masked]>
Date: Fri, 22 Dec 2023 22:44:27 +0800

> to the client, so the value of the GUC is already set before

I mean, is it possible to set transaction_timeout before next comand?

---

From: Junwang Zhao <[email-masked]>
Date: Fri, 22 Dec 2023 23:30:56 +0800

> I mean, is it possible to set transaction_timeout before next comand?
Yeah, it's possible, set transaction_timeout in the when it first
goes into *idle in transaction* mode, see the attached files.

-- 
Regards
Junwang Zhao

---

From: Japin Li <[email-masked]>
Date: Sat, 23 Dec 2023 08:32:26 +0800

> goes into *idle in transaction* mode, see the attached files.

Thanks for updating the patch, LGTM.

---

From: Japin Li <[email-masked]>
Date: Sat, 23 Dec 2023 10:40:32 +0800

> Thanks for updating the patch, LGTM.

Sorry for the noise!

Read the previous threads, I find why the author enable transaction_timeout
in start_xact_command().

The v15 patch cannot handle COMMIT AND CHAIN, see [1]. For example:

SET transaction_timeout TO '2s'; BEGIN; SELECT 1, pg_sleep(1); COMMIT AND CHAIN; SELECT 2, pg_sleep(1); COMMIT;

The transaction_timeout do not reset when executing COMMIT AND CHAIN.

[1] https://www.postgresql.org/message-id/a906dea1-76a1-4f26-76c5-a7efad3ef5b8%40oss.nttdata.com

---

From: Junwang Zhao <[email-masked]>
Date: Sat, 23 Dec 2023 11:08:33 +0800

> The v15 patch cannot handle COMMIT AND CHAIN, see [1]. For example:

I didn't read the previous threads, sorry for that, let's stick to v14.

-- 
Regards
Junwang Zhao

---

From: Japin Li <[email-masked]>
Date: Sat, 23 Dec 2023 11:17:14 +0800

a
> [1] https://www.postgresql.org/message-id/a906dea1-76a1-4f26-76c5-a7efad3ef5b8%40oss.nttdata.com

Attach v16 to solve this. Any suggestions?

>From a9d79c5a013da8fa707556f87a34ff3ade779729 Mon Sep 17 00:00:00 2001
From: "Andrey M. Borodin" <[email-masked]>
Date: Sun, 3 Dec 2023 23:18:00 +0500
Subject: [PATCH v16 1/2] Introduce transaction_timeout

 

 

 

 

+static void
 
+	{

 	/*
 
 	/*

 check: all

 
 
+session s3

base-commit: 3e2e0d5ad7fcb89d18a71cbfc885ef184e1b6f2e
-- 
2.41.0

>From 95133524aaeaf84733d6bb12a42d3e27aea85c16 Mon Sep 17 00:00:00 2001
From: japinli <[email-masked]>
Date: Sat, 23 Dec 2023 11:04:25 +0800
Subject: [PATCH v16 2/2] Try to enable transaction_timeout before next command

---
 src/backend/tcop/postgres.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index a2611cf8e6..96161eb7ab 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -4513,6 +4513,11 @@ PostgresMain(const char *dbname, const char *username)
 					enable_timeout_after(IDLE_IN_TRANSACTION_SESSION_TIMEOUT,
 										 IdleInTransactionSessionTimeout);
 				}
+
+				/* Schedule or reschedule transaction timeout */
+				if (TransactionTimeout > 0 && !get_timeout_active(TRANSACTION_TIMEOUT))
+					enable_timeout_after(TRANSACTION_TIMEOUT,
+										 TransactionTimeout);
 			}
 			else if (IsTransactionOrTransactionBlock())
 			{
@@ -4527,6 +4532,11 @@ PostgresMain(const char *dbname, const char *username)
 					enable_timeout_after(IDLE_IN_TRANSACTION_SESSION_TIMEOUT,
 										 IdleInTransactionSessionTimeout);
 				}
+
+				/* Schedule or reschedule transaction timeout */
+				if (TransactionTimeout > 0 && !get_timeout_active(TRANSACTION_TIMEOUT))
+					enable_timeout_after(TRANSACTION_TIMEOUT,
+										 TransactionTimeout);
 			}
 			else
 			{
-- 
2.41.0

---

From: Junwang Zhao <[email-masked]>
Date: Sat, 23 Dec 2023 11:35:17 +0800

> Attach v16 to solve this. Any suggestions?

I've checked this with *COMMIT AND CHAIN* and *ABORT AND CHAIN*,
both work as expected. Thanks for the update.

-- 
Regards
Junwang Zhao

---

From: Li Japin <[email-masked]>
Date: Sat, 23 Dec 2023 06:48:14 +0000

> 在 2023年12月23日，11:35，Junwang Zhao <[email-masked]> 写道：
> 

Thanks for your testing and reviewing!

---

From: "Andrey M. Borodin" <[email-masked]>
Date: Sat, 23 Dec 2023 22:14:35 +0500

> On 22 Dec 2023, at 10:39, Japin Li <[email-masked]> wrote:
> 
> 

I like the refactoring you did in timeout.spec. I thought it is impossible, because permutations would try to reinitialize FATALed sessions. But, obviously, tests work the way you refactored it.
However I don't think ignoring test failures on Windows without understanding root cause is a good idea.
Let's get back to v13 version of tests, understand why it failed, apply your test refactorings afterwards. BTW are you sure that v14 refactorings are functional equivalent of v13 tests?

To go with this plan I attach slightly modified version of v13 tests in v16 patchset. The only change is timing in "sleep_there" step. I suspect that failure was induced by more coarse timer granularity on Windows. Tests were giving only 9 milliseconds for a timeout to entirely wipe away backend from pg_stat_activity. This saves testing time, but might induce false positive test flaps. So I've raised wait times to 100ms. This seems too much, but I do not have other ideas how to ensure tests stability. Maybe 50ms would be enough, I do not know. Isolation runs ~50 seconds now. I'm tempted to say that 200ms for timeouts worth it.

As to 2nd step "Try to enable transaction_timeout during transaction", I think this makes sense. But if we are doing so, shouldn't we also allow to enable idle_in_transaction timeout in a same manner? Currently we only allow to disable other timeouts... Also, if we are already in transaction, shouldn't we also subtract current transaction span from timeout?
I think making this functionality as another step of the patchset was a good idea.

Thanks!

---

From: Junwang Zhao <[email-masked]>
Date: Mon, 25 Dec 2023 10:17:50 +0800

Hi Andrey,

> As to 2nd step "Try to enable transaction_timeout during transaction", I think this makes sense. But if we are doing so, shouldn't we also allow to enable idle_in_transaction timeout in a same mann…
idle_in_transaction_session_timeout is already the behavior Japin suggested,
it is enabled before backend sends *read for query* to client.

-- 
Regards
Junwang Zhao

---

From: Japin Li <[email-masked]>
Date: Mon, 25 Dec 2023 10:27:32 +0800

> However I don't think ignoring test failures on Windows without understanding root cause is a good idea.

Yeah.

> Let's get back to v13 version of tests, understand why it failed, apply your test refactorings afterwards. BTW are you sure that v14 refactorings are functional equivalent of v13 tests?
I think it is equivalent.  Maybe I missing something.  Please let me known
if they are not equivalent.

> To go with this plan I attach slightly modified version of v13 tests in v16 patchset. The only change is timing in "sleep_there" step. I suspect that failure was induced by more coarse timer granul…
So this is caused by Windows timer granularity?

> As to 2nd step "Try to enable transaction_timeout during transaction", I think this makes sense. But if we are doing so, shouldn't we also allow to enable idle_in_transaction timeout in a same manner?

I think the current idle_in_transaction_session_timeout work correctly.

> Currently we only allow to disable other timeouts... Also, if we are already in transaction, shouldn't we also subtract current transaction span from timeout?

Agreed.

---

From: Junwang Zhao <[email-masked]>
Date: Fri, 29 Dec 2023 00:02:36 +0800

Hey Andrey,

> I think making this functionality as another step of the patchset was a good idea.
Seems V5~V17 doesn't work as expected for Nikolay's case:

postgres=# set transaction_timeout to '2s';
SET
postgres=# begin; select pg_sleep(1); select pg_sleep(1); select
pg_sleep(1); select pg_sleep(1); select pg_sleep(1); commit;
BEGIN

The reason for this seems the timer has been refreshed for each
command, xact_started along can not indicate it's a new
transaction or not, there is a TransactionState contains some
infos.

So I propose the following change, what do you think?

diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index a2611cf8e6..cffd2c44d0 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -2746,7 +2746,7 @@ start_xact_command(void)
                StartTransactionCommand();

                /* Schedule or reschedule transaction timeout */
-               if (TransactionTimeout > 0)
+               if (TransactionTimeout > 0 &&
!get_timeout_active(TRANSACTION_TIMEOUT))
                        enable_timeout_after(TRANSACTION_TIMEOUT,
TransactionTimeout);

                xact_started = true;

-- 
Regards
Junwang Zhao

---

From: "Andrey M. Borodin" <[email-masked]>
Date: Fri, 29 Dec 2023 15:00:12 +0500

> On 28 Dec 2023, at 21:02, Junwang Zhao <[email-masked]> wrote:
> 

Yeah, that's a problem.
> So I propose the following change, what do you think?
This breaks COMMIT AND CHAIN.

PFA v18: I've added a test for Nik's case and for COMMIT AND CHAIN. Now we need to fix stuff to pass this tests (I've crafted output).
We also need test for patchset step "Try to enable transaction_timeout before next command".

Thanks!

---

From: Junwang Zhao <[email-masked]>
Date: Fri, 29 Dec 2023 19:00:10 +0800

> We also need test for patchset step "Try to enable transaction_timeout before next command".

After exploring the code, I found scheduling the timeout in
`StartTransaction` might be a reasonable idea, all the chain
commands will call this function.

What concerns me is that it is also called by StartParallelWorkerTransaction,
I'm not sure if we should enable this timeout for parallel execution.

Thought?

-- 
Regards
Junwang Zhao

---

From: "Andrey M. Borodin" <[email-masked]>
Date: Fri, 29 Dec 2023 16:15:00 +0500

> On 29 Dec 2023, at 16:00, Junwang Zhao <[email-masked]> wrote:
> 

I think for parallel workers we should mimic statement_timeout. Because these workers have per-statemenent lifetime.

---

From: "Andrey M. Borodin" <[email-masked]>
Date: Mon, 1 Jan 2024 19:28:33 +0500

> On 29 Dec 2023, at 16:15, Andrey M. Borodin <[email-masked]> wrote:

PFA v20. Code steps are intact.

Further refactored tests:
    1. Check termination of active and idle queries (previously tests from Li were testing only termination of idle query)
    2. Check timeout reschedule (even when last active query was 'SET transaction_timeout')
    3. Check that timeout is not rescheduled by new queries (Nik's case)

Do we have any other open items?
I've left 'make check-timeouts' in isolation directory, it's for development purposes. I think we should remove this before committing. Obviously, all patch steps are expected to be squashed before commit.

---

From: "Andrey M. Borodin" <[email-masked]>
Date: Wed, 3 Jan 2024 11:39:44 +0500

> On 1 Jan 2024, at 19:28, Andrey M. Borodin <[email-masked]> wrote:
> 

The test of Nik's case was not stable enough together with COMMIT AND CHAIN. So I've separated these cases into different permutations.
Looking through CI logs it seems variation in sleeps and actual timeouts easily reach 30+ms. I'm not entirely sure we can reach 100% stable tests without too big timeouts.

---

From: "Andrey M. Borodin" <[email-masked]>
Date: Wed, 3 Jan 2024 16:46:36 +0500

> On 3 Jan 2024, at 11:39, Andrey M. Borodin <[email-masked]> wrote:
> 
> 
> 
>> On 1 Jan 2024, at 19:28, Andrey M. Borodin <[email-masked] <mailto:[email-masked]>> wrote:
>> 
> <v21-0001-Introduce-transaction_timeout.patch>
> <v21-0002-Add-better-tests-for-transaction_timeout.patch>
> <v21-0003-Try-to-enable-transaction_timeout-before-next-co.patch>
> <v21-0004-fix-reschedule-timeout-for-each-commmand.patch>
 I do not understand why, but mailing list did not pick patches that I sent. I'll retry.

---

From: "Andrey M. Borodin" <[email-masked]>
Date: Wed, 3 Jan 2024 17:04:41 +0500

> On 3 Jan 2024, at 16:46, Andrey M. Borodin <[email-masked]> wrote:
> 

Sorry for the noise. Seems like Apple updated something in Mail.App couple of days ago and it started to use strange "Apple-Mail" stuff by default.
I see patches were attached, but were not recognized by mailing list archives and CFbot.
Now I've flipped everything to "plain text by default" everywhere. Hope that helps.

---

From: Japin Li <[email-masked]>
Date: Thu, 04 Jan 2024 10:14:34 +0800

> Now I've flipped everything to "plain text by default" everywhere. Hope that helps.

Thanks for updating the patch, I find the test on Debian with mason failed [1].

Does the timeout is too short for testing? I see the timeouts for lock_timeout
and statement_timeout is more bigger than transaction_timeout.

[1] https://api.cirrus-ci.com/v1/artifact/task/5490718928535552/testrun/build-32/testrun/isolation/isolation/regression.diffs

---

From: "Andrey M. Borodin" <[email-masked]>
Date: Thu, 4 Jan 2024 13:41:43 +0500

> On 4 Jan 2024, at 07:14, Japin Li <[email-masked]> wrote:
> 

Makes sense. Done. I've also put some effort into fine-tuning timeouts Nik's case tests. To have 100ms gap between check, false positive and actual bug we had I had to use transaction_timeout = 300ms. Currently all tests take more than 1000ms!
But I do not see a way to make these tests both stable and short.

---

From: Peter Smith <[email-masked]>
Date: Mon, 22 Jan 2024 17:23:51 +1100

2024-01 Commitfest.

Hi, This patch has a CF status of "Needs Review" [1], but it seems
there was a CFbot test failure last time it was run [2]. Please have a
look and post an updated version if necessary.

======
[1] https://commitfest.postgresql.org/46/4040/
[2] https://cirrus-ci.com/task/4721191139672064

Kind Regards,
Peter Smith.

---

From: "Andrey M. Borodin" <[email-masked]>
Date: Fri, 26 Jan 2024 11:44:02 +0500

> On 22 Jan 2024, at 11:23, Peter Smith <[email-masked]> wrote:
> 
Thanks Peter!

I’ve inspected CI fails and they were caused by two different problems:
1. It’s unsafe for isaoltion tester to await transaction_timeout within a query. Usually it gets
FATAL: terminating connection due to transaction timeout
But if VM is a bit slow it can get occasional
PQconsumeInput failed: server closed the connection unexpectedly
So, currently all tests use “passive waiting”, in a session that will not timeout.

2. In some cases pg_sleep(0.1) were sleeping up to 200 ms. That was making s7 and s8 fail, because they rely on this margin.
I’ve separated these tests into different test timeouts-long and increased margin to 300ms. Now tests run horrible 2431 ms. Moreover I’m afraid that on buildfarm we can have much randomly-slower machines so this test might be excluded.
This test checks COMMIT AND CHAIN and flow of small queries (Nik’s case).

Also I’ve verified that every "enable_timeout_after(TRANSACTION_TIMEOUT)” and “disable_timeout(TRANSACTION_TIMEOUT)” is necessary and found that case of aborting "idle in transaction (aborted)” is not covered by tests. I’m not sure we need a test for this.
Japin, Junwang, what do you think?

Thanks!

---

From: "Andrey M. Borodin" <[email-masked]>
Date: Fri, 26 Jan 2024 11:46:43 +0500

> On 26 Jan 2024, at 11:44, Andrey M. Borodin <[email-masked]> wrote:
> 
> 

Oops, sorry, I’ve accidentally sent version without this fix.
Here it is.

---

From: Japin Li <[email-masked]>
Date: Fri, 26 Jan 2024 22:58:34 +0800

>> look and post an updated version if necessary.

Thanks for updating the patch.  Here are some comments for v24.

+       <para>
+        Terminate any session that spans longer than the specified amount of
+        time in transaction. The limit applies both to explicit transactions
+        (started with <command>BEGIN</command>) and to implicitly started
+        transaction corresponding to single statement. But this limit is not
+        applied to prepared transactions.
+        If this value is specified without units, it is taken as milliseconds.
+        A value of zero (the default) disables the timeout.
+       </para>
The sentence "But this limit is not applied to prepared transactions" is redundant,
since we have a paragraph to describe this later.

+
+       <para>

Since we are already try to disable the timeouts, should we try to disable
them even if they are equal.

+
+       <para>

Maybe wrap this with <note> is a good idea.

> 2. In some cases pg_sleep(0.1) were sleeping up to 200 ms. That was making s7 and s8 fail, because they rely on this margin.

I'm curious why this happened.

> Also I’ve verified that every "enable_timeout_after(TRANSACTION_TIMEOUT)” and “disable_timeout(TRANSACTION_TIMEOUT)” is necessary and found that case of aborting "idle in transaction (aborted)” is…

I see there is a test about idle_in_transaction_timeout and transaction_timeout.

Both of them only check the session, but don't check the reason, so we cannot
distinguish the reason they are terminated.  Right?

> Japin, Junwang, what do you think?

However, checking the reason on the timeout session may cause regression test
failed (as you point in 1), I don't strongly insist on it.

--
Best regards,
Japin Li.

---

From: "Andrey M. Borodin" <[email-masked]>
Date: Tue, 30 Jan 2024 11:22:51 +0500

> On 26 Jan 2024, at 19:58, Japin Li <[email-masked]> wrote:
> 
Fixed.
> them even if they are equal.

Well, we disable timeouts on equality. Fixed docs.

> Maybe wrap this with <note> is a good idea.
Done.

> I'm curious why this happened.
I think pg_sleep() cannot provide guarantees on when next query will be executed. In our case we need that isolation tester see that sleep is over and continue in other session...

> distinguish the reason they are terminated.  Right?
Yes.
> failed (as you point in 1), I don't strongly insist on it.

Indeed, if we check a reason of FATAL timeouts - we get flaky tests.

---

From: Japin Li <[email-masked]>
Date: Wed, 31 Jan 2024 17:27:06 +0800

If there is no other objections, I'll change it to ready for committer
next Monday.

---

From: Andrey Borodin <[email-masked]>
Date: Wed, 31 Jan 2024 14:57:32 +0500

> On 31 Jan 2024, at 14:27, Japin Li <[email-masked]> wrote:
> 
> LGTM.
> 

I think we have a quorum, so I decided to go ahead and flipped status to RfC. Thanks!

---

From: Alexander Korotkov <[email-masked]>
Date: Tue, 13 Feb 2024 23:42:35 +0200

Hi!

> I think we have a quorum, so I decided to go ahead and flipped status to RfC. Thanks!

I checked this patch.  Generally I look good.  I've slightly revised that.

I think there is one unaddressed concern by Andres Freund [1] about
the overhead of this patch by adding extra branches and function calls
in the case transaction_timeout is disabled.  I tried to measure the
overhead of this patch using a pgbench script containing 20 semicolons
(20 empty statements in 20 empty transactions).  I didn't manage to
find measurable overhead or change of performance profile (I used
XCode Instruments on my x86 MacBook).  One thing, which I still found
possible to do is to avoid unconditional calls to
get_timeout_active(TRANSACTION_TIMEOUT).  Instead I put responsibility
for disabling timeout after GUC disables the transaction_timeout
assign hook.

I removed the TODO comment from _doSetFixedOutputState().  I think
backup restore is the operation where slow commands and slow
transactions are expected, and it's natural to disable
transaction_timeout among other timeouts there.  And the existing
comment clarifies that.

Also I made some grammar fixes to docs and comments.

I'm going to push this if there are no objections.

Links.
1. https://www.postgresql.org/message-id/20221206011050.s6hapukjqha35hud%40alap3.anarazel.de

------
Regards,
Alexander Korotkov

---

From: Andres Freund <[email-masked]>
Date: Thu, 15 Feb 2024 15:08:56 -0800

Hi,

On 2024-02-13 23:42:35 +0200, Alexander Korotkov wrote:
> diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
> index 464858117e0..a124ba59330 100644
> --- a/src/backend/access/transam/xact.c
> +++ b/src/backend/access/transam/xact.c
> @@ -2139,6 +2139,10 @@ StartTransaction(void)
>  	 */
>  	s->state = TRANS_INPROGRESS;
>  
> +	/* Schedule transaction timeout */
>  	ShowTransactionState("StartTransaction");
>  }

Isn't it a problem that all uses of StartTransaction() trigger a timeout, but
transaction commit/abort don't?  What if e.g. logical replication apply starts
a transaction, commits it, and then goes idle? The timer would still be
active, afaict?

I don't think it works well to enable timeouts in xact.c and to disable them
in PostgresMain().

> @@ -4491,12 +4511,18 @@ PostgresMain(const char *dbname, const char *username)
> @@ -4504,12 +4530,18 @@ PostgresMain(const char *dbname, const char *username)

Why do we need to do anything in these cases if the timer is started in
StartTransaction()?

> new file mode 100644
> index 00000000000..ce2c9a43011
> --- /dev/null
> +++ b/src/test/isolation/specs/timeouts-long.spec
> @@ -0,0 +1,35 @@
> +# Tests for transaction timeout that require long wait times
> +
> +session s7
> +step s7_begin
> +{
> +    BEGIN ISOLATION LEVEL READ COMMITTED;
> +    SET transaction_timeout = '1s';
> +}
> +step s7_commit_and_chain { COMMIT AND CHAIN; }
> +step s7_sleep	{ SELECT pg_sleep(0.6); }
> +step s7_abort	{ ABORT; }
> +
> +session s8
> +step s8_begin
> +{
> +    BEGIN ISOLATION LEVEL READ COMMITTED;
> +    SET transaction_timeout = '900ms';
> +}
> +# to test that quick query does not restart transaction_timeout
> +step s8_select_1 { SELECT 1; }
> +step s8_sleep	{ SELECT pg_sleep(0.6); }
> +
> +session checker
> +step checker_sleep	{ SELECT pg_sleep(0.3); }

Isn't this test going to be very fragile on busy / slow machines? What if the
pg_sleep() takes one second, because there were other tasks to schedule?  I'd
be surprised if this didn't fail under valgrind, for example.

Greetings,

Andres Freund

---

From: "Andrey M. Borodin" <[email-masked]>
Date: Sun, 18 Feb 2024 22:16:02 +0300

Alexander, thanks for pushing this! This is small but very awaited feature.

> On 16 Feb 2024, at 02:08, Andres Freund <[email-masked]> wrote:
> 

Even more robust tests that were bullet-proof in CI previously exhibited some failures on buildfarm. Currently there are 5 failures through this weekend.
Failing tests are testing interaction of idle_in_transaction_session_timeout vs transaction_timeout(5), and rescheduling transaction_timeout(6).
Symptoms:

[0] transaction timeout occurs when it is being scheduled. Seems like SET was running to long.
 step s6_begin: BEGIN ISOLATION LEVEL READ COMMITTED;
 step s6_tt: SET statement_timeout = '1s'; SET transaction_timeout = '10ms';
+s6: FATAL:  terminating connection due to transaction timeout
 step checker_sleep: SELECT pg_sleep(0.1);

[1] transaction timeout 10ms is not detected after 1s
step s6_check: SELECT count(*) FROM pg_stat_activity WHERE application_name = 'isolation/timeouts/s6';
 count
 -----
-    0
+    1

[2] transaction timeout is not detected in both session 5 and session 6.

So far not signle animal reported failures twice, so it's hard to say anything about frequency. But it seems to be significant source of failures.

So far I have these ideas:

1. Remove test sessions 5 and 6. But it seems a little strange that session 3 did  not fail at all (it is testing interaction of statement_timeout and transaction_timeout). This test is very similar to test sessiont 5...
2. Increase wait times.
step checker_sleep	{ SELECT pg_sleep(0.1); }
Seems not enough to observe backend timed out from pg_stat_activity. But this won't help from [0].
3. Reuse waiting INJECTION_POINT from [3] to make timeout tests deterministic and safe from race conditions. With waiting injection points we can wait as much as needed in current environment.

Any advices are welcome.

[0] https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=tamandua&dt=2024-02-16%2020%3A06%3A51
[1] https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=kestrel&dt=2024-02-16%2001%3A45%3A10
[2] https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=fairywren&dt=2024-02-17%2001%3A55%3A45
[3] https://www.postgresql.org/message-id/[email-masked]

---

From: "Andrey M. Borodin" <[email-masked]>
Date: Mon, 19 Feb 2024 12:14:05 +0300

> On 18 Feb 2024, at 22:16, Andrey M. Borodin <[email-masked]> wrote:
> 
> But it seems a little strange that session 3 did  not fail at all
It was only coincidence. Any test that verifies FATALing out in 100ms can fail, see new failure here [0].

In a nearby thread Michael is proposing injections points that can wait and be awaken. So I propose following course of action:
1. Remove all tests that involve pg_stat_activity test of FATALed session (any permutation with checker_sleep step)
2. Add idle_in_transaction_session_timeout, statement_timeout and transaction_timeout tests when injection points features get committed.

Alexander, what do you think?

[0] https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=grassquit&dt=2024-02-18%2022%3A23%3A45

---

From: Japin Li <[email-masked]>
Date: Mon, 19 Feb 2024 18:17:16 +0800

> 2. Add idle_in_transaction_session_timeout, statement_timeout and transaction_timeout tests when injection points features get committed.

+1

---

From: "Andrey M. Borodin" <[email-masked]>
Date: Thu, 22 Feb 2024 22:23:24 +0500

> On 19 Feb 2024, at 15:17, Japin Li <[email-masked]> wrote:
> 
> 
> +1

PFA patch set of 4 patches:
1. remove all potential flaky tests. BTW recently we had a bingo when 3 of them failed together [0]
2-3. waiting injection points patchset by Michael Paquier, intact v2 from nearby thread.
4. prototype of simple TAP tests for timeouts.

I did not add a test for statement_timeout, because it still have good coverage in isolation tests. But added test for idle_sessoin_timeout.
Maybe these tests could be implemented with NOTICE injection points (not requiring steps 2-3), but I'm afraid that they might be flaky too: FATALed connection might not send information necesary for test success (we will see something like "PQconsumeInput failed: server closed the connection unexpectedly" as in [1]).

[0] https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=tamandua&dt=2024-02-20%2010%3A20%3A13
[1] https://www.postgresql.org/message-id/flat/CAAhFRxiQsRs2Eq5kCo9nXE3HTugsAAJdSQSmxncivebAxdmBjQ%40mail.gmail.com

---

From: Alexander Korotkov <[email-masked]>
Date: Sun, 25 Feb 2024 20:50:48 +0200

Hi, Andrey!

> Maybe these tests could be implemented with NOTICE injection points (not requiring steps 2-3), but I'm afraid that they might be flaky too: FATALed connection might not send information necesary fo…

Thank you for the patches.  I've pushed the 0001 patch to avoid
further failures on buildfarm.  Let 0004 wait till injections points
by Mechael are committed.

------
Regards,
Alexander Korotkov

---

From: "Andrey M. Borodin" <[email-masked]>
Date: Wed, 6 Mar 2024 11:22:00 +0300

> On 25 Feb 2024, at 21:50, Alexander Korotkov <[email-masked]> wrote:
> 

Thanks!

All prerequisites are committed. I propose something in a line with this patch.

---

From: Alexander Korotkov <[email-masked]>
Date: Wed, 6 Mar 2024 21:55:46 +0200

> All prerequisites are committed. I propose something in a line with this patch.

Thank you.  I took a look at the patch.  Should we also check the
relevant message after the timeout is fired?  We could check it in
psql stderr or log for that.

------
Regards,
Alexander Korotkov

---

From: "Andrey M. Borodin" <[email-masked]>
Date: Mon, 11 Mar 2024 15:52:57 +0500

> On 7 Mar 2024, at 00:55, Alexander Korotkov <[email-masked]> wrote:
> 

PFA version which checks log output.
But I could not come up with a proper use of BackgroundPsql->query_until() to check outputs. And there are multiple possible errors.

We can copy test from src/bin/psql/t/001_basic.pl:

# test behavior and output on server crash
my ($ret, $out, $err) = $node->psql('postgres',
"SELECT 'before' AS running;\n"
. "SELECT pg_terminate_backend(pg_backend_pid());\n"
. "SELECT 'AFTER' AS not_running;\n");

is($ret, 2, 'server crash: psql exit code');
like($out, qr/before/, 'server crash: output before crash');
ok($out !~ qr/AFTER/, 'server crash: no output after crash');
is( $err,
'psql:<stdin>:2: FATAL: terminating connection due to administrator command
psql:<stdin>:2: server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
psql:<stdin>:2: error: connection to server was lost',
'server crash: error message’);

But I do not see much value in this.
What do you think?

---

From: Alexander Korotkov <[email-masked]>
Date: Mon, 11 Mar 2024 13:18:01 +0200

> But I do not see much value in this.

I think if checking psql stderr is problematic, checking just logs is
fine.  Could we wait for the relevant log messages one by one with
$node->wait_for_log() just like 040_standby_failover_slots_sync.pl do?

------
Regards,
Alexander Korotkov

---

From: "Andrey M. Borodin" <[email-masked]>
Date: Tue, 12 Mar 2024 13:28:19 +0500

> On 11 Mar 2024, at 16:18, Alexander Korotkov <[email-masked]> wrote:
> 

PFA version with $node->wait_for_log()

---

From: Alexander Korotkov <[email-masked]>
Date: Wed, 13 Mar 2024 02:23:27 +0200

> PFA version with $node->wait_for_log()

I've slightly revised the patch.  I'm going to push it if no objections.

------
Regards,
Alexander Korotkov

---

From: "Andrey M. Borodin" <[email-masked]>
Date: Wed, 13 Mar 2024 10:56:06 +0500

> On 13 Mar 2024, at 05:23, Alexander Korotkov <[email-masked]> wrote:
> 

One small note: log_offset was updated, but was not used.

Thanks!

---

From: Alexander Korotkov <[email-masked]>
Date: Wed, 13 Mar 2024 10:17:37 +0200

> One small note: log_offset was updated, but was not used.

Thank you. This is the updated version.

------
Regards,
Alexander Korotkov