v4-0001-Intorduce-transaction_timeout.patch

From f75b18441db3a40f143ce094cb7e705ae34640db Mon Sep 17 00:00:00 2001
From: Andrey Borodin <xformmm@amazon.com>
Date: Fri, 2 Dec 2022 21:01:29 -0800
Subject: [PATCH v4] Intorduce transaction_timeout

Just like statement_timeout, but for transaction.

Author: Andrey Borodin <amborodin@acm.org>
Reviewed-by: Andres Freund <andres@anarazel.de>
Discussion: https://postgr.es/m/CAAhFRxiQsRs2Eq5kCo9nXE3HTugsAAJdSQSmxncivebAxdmBjQ%40mail.gmail.com
---
 doc/src/sgml/config.sgml                      | 24 +++++++++
 src/backend/postmaster/autovacuum.c           |  1 +
 src/backend/storage/lmgr/proc.c               |  1 +
 src/backend/tcop/postgres.c                   | 51 +++++++++++--------
 src/backend/utils/errcodes.txt                |  1 +
 src/backend/utils/init/postinit.c             |  9 ++--
 src/backend/utils/misc/guc_tables.c           | 11 ++++
 src/backend/utils/misc/postgresql.conf.sample |  7 +--
 src/bin/pg_dump/pg_backup_archiver.c          |  1 +
 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/expected/timeouts.out      | 18 +++++++
 src/test/isolation/specs/timeouts.spec        |  8 +++
 16 files changed, 110 insertions(+), 28 deletions(-)

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 39d1c89e33..fd74db7527 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -9009,6 +9009,30 @@ 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>
+        Cancel any transaction that spans longer than the specified amount of
+        time. The limit applies both to explicit transactions (started with
+        <command>BEGIN</command>) and to implicitly started transaction
+        corresponding to single statement.
+        If this value is specified without units, it is taken as milliseconds.
+        A value of zero (the default) disables the timeout.
+       </para>
+
+       <para>
+        Setting <varname>transaction_timeout</varname> in
+        <filename>postgresql.conf</filename> is not recommended because it would
+        affect all sessions.
+       </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 601834d4b4..828a28af0a 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -588,6 +588,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);
diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c
index b1c35653fc..0170e226d0 100644
--- a/src/backend/storage/lmgr/proc.c
+++ b/src/backend/storage/lmgr/proc.c
@@ -61,6 +61,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 3082093d1e..1eaf2e054a 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -2691,6 +2691,9 @@ start_xact_command(void)
 	{
 		StartTransactionCommand();
 
+		if (TransactionTimeout > 0 && !get_timeout_active(TRANSACTION_TIMEOUT))
+			enable_timeout_after(TRANSACTION_TIMEOUT, TransactionTimeout);
+
 		xact_started = true;
 	}
 
@@ -3268,40 +3271,43 @@ ProcessInterrupts(void)
 	{
 		bool		lock_timeout_occurred;
 		bool		stmt_timeout_occurred;
+		bool		tx_timeout_occurred;
+		int			err_code = ERRCODE_QUERY_CANCELED;
 
 		QueryCancelPending = false;
 
 		/*
-		 * If LOCK_TIMEOUT and STATEMENT_TIMEOUT indicators are both set, we
-		 * need to clear both, so always fetch both.
+		 * If LOCK_TIMEOUT, STATEMENT_TIMEOUT and TRANSACTION indicators are set, we
+		 * need to clear all of them, so always fetch each one.
 		 */
 		lock_timeout_occurred = get_timeout_indicator(LOCK_TIMEOUT, true);
 		stmt_timeout_occurred = get_timeout_indicator(STATEMENT_TIMEOUT, true);
-
-		/*
-		 * If both were set, we want to report whichever timeout completed
-		 * earlier; this ensures consistent behavior if the machine is slow
-		 * enough that the second timeout triggers before we get here.  A tie
-		 * is arbitrarily broken in favor of reporting a lock timeout.
-		 */
-		if (lock_timeout_occurred && stmt_timeout_occurred &&
-			get_timeout_finish_time(STATEMENT_TIMEOUT) < get_timeout_finish_time(LOCK_TIMEOUT))
-			lock_timeout_occurred = false;	/* report stmt timeout */
+		tx_timeout_occurred = get_timeout_indicator(TRANSACTION_TIMEOUT, true);
 
 		if (lock_timeout_occurred)
+			err_code = ERRCODE_LOCK_NOT_AVAILABLE;
+
+
+		if (lock_timeout_occurred || stmt_timeout_occurred || tx_timeout_occurred)
 		{
+			/* Report all reasons for timeout */
+			char* lock_reason = lock_timeout_occurred ?
+									_("lock timeout") : "";
+			char* stmt_reason = stmt_timeout_occurred ?
+									_("statement timeout") : "";
+			char* tx_reason = tx_timeout_occurred ?
+									_("transaction timeout") : "";
+			char* comma1 = lock_timeout_occurred && stmt_timeout_occurred ?
+									"," : "";
+			char* comma2 = (lock_timeout_occurred || stmt_timeout_occurred)
+									&& tx_timeout_occurred ? "," : "";
 			LockErrorCleanup();
 			ereport(ERROR,
-					(errcode(ERRCODE_LOCK_NOT_AVAILABLE),
-					 errmsg("canceling statement due to lock timeout")));
-		}
-		if (stmt_timeout_occurred)
-		{
-			LockErrorCleanup();
-			ereport(ERROR,
-					(errcode(ERRCODE_QUERY_CANCELED),
-					 errmsg("canceling statement due to statement timeout")));
+					(errcode(err_code),
+					 errmsg("canceling statement due to %s%s%s%s%s", lock_reason, comma1,
+					 			stmt_reason, comma2, tx_reason)));
 		}
+
 		if (IsAutoVacuumWorkerProcess())
 		{
 			LockErrorCleanup();
@@ -4460,6 +4466,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 */
diff --git a/src/backend/utils/errcodes.txt b/src/backend/utils/errcodes.txt
index 62418a051a..3ae2bbda70 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/postinit.c b/src/backend/utils/init/postinit.c
index a990c833c5..779cb22915 100644
--- a/src/backend/utils/init/postinit.c
+++ b/src/backend/utils/init/postinit.c
@@ -73,7 +73,7 @@ static void PerformAuthentication(Port *port);
 static void CheckMyDatabase(const char *name, bool am_superuser, bool override_allow_connections);
 static void ShutdownPostgres(int code, Datum arg);
 static void StatementTimeoutHandler(void);
-static void LockTimeoutHandler(void);
+static void CancelOnTimeoutHandler(void);
 static void IdleInTransactionSessionTimeoutHandler(void);
 static void IdleSessionTimeoutHandler(void);
 static void IdleStatsUpdateTimeoutHandler(void);
@@ -753,9 +753,10 @@ InitPostgres(const char *in_dbname, Oid dboid,
 	{
 		RegisterTimeout(DEADLOCK_TIMEOUT, CheckDeadLockAlert);
 		RegisterTimeout(STATEMENT_TIMEOUT, StatementTimeoutHandler);
-		RegisterTimeout(LOCK_TIMEOUT, LockTimeoutHandler);
+		RegisterTimeout(LOCK_TIMEOUT, CancelOnTimeoutHandler);
 		RegisterTimeout(IDLE_IN_TRANSACTION_SESSION_TIMEOUT,
 						IdleInTransactionSessionTimeoutHandler);
+		RegisterTimeout(TRANSACTION_TIMEOUT, CancelOnTimeoutHandler);
 		RegisterTimeout(IDLE_SESSION_TIMEOUT, IdleSessionTimeoutHandler);
 		RegisterTimeout(CLIENT_CONNECTION_CHECK_TIMEOUT, ClientCheckTimeoutHandler);
 		RegisterTimeout(IDLE_STATS_UPDATE_TIMEOUT,
@@ -1340,10 +1341,10 @@ StatementTimeoutHandler(void)
 }
 
 /*
- * LOCK_TIMEOUT handler: trigger a query-cancel interrupt.
+ * LOCK_TIMEOUT and TRANSACTION_TIMEOUT handler: trigger a query-cancel interrupt.
  */
 static void
-LockTimeoutHandler(void)
+CancelOnTimeoutHandler(void)
 {
 #ifdef HAVE_SETSID
 	/* try to signal whole process group */
diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c
index 1bf14eec66..ca21d2544c 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -2452,6 +2452,17 @@ struct config_int ConfigureNamesInt[] =
 		NULL, NULL, NULL
 	},
 
+	{
+		{"transaction_timeout", PGC_USERSET, CLIENT_CONN_STATEMENT,
+			gettext_noop("Sets the maximum allowed in a 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 043864597f..a8c935f60c 100644
--- a/src/backend/utils/misc/postgresql.conf.sample
+++ b/src/backend/utils/misc/postgresql.conf.sample
@@ -687,10 +687,11 @@
 #default_transaction_read_only = off
 #default_transaction_deferrable = off
 #session_replication_role = 'origin'
-#statement_timeout = 0			# in milliseconds, 0 is disabled
-#lock_timeout = 0			# in milliseconds, 0 is disabled
+#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
+#idle_session_timeout = 0			# in milliseconds, 0 is disabled
 #vacuum_freeze_table_age = 150000000
 #vacuum_freeze_min_age = 50000000
 #vacuum_failsafe_age = 1600000000
diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c
index 0081873a72..5229fe3555 100644
--- a/src/bin/pg_dump/pg_backup_archiver.c
+++ b/src/bin/pg_dump/pg_backup_archiver.c
@@ -3089,6 +3089,7 @@ _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");
+	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 44e8cd4704..87be74753d 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -1188,6 +1188,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 >= 160000)
+		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 011c9cce6e..1b9674140a 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 795182fa51..484384b4ed 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 aa13e1d66e..a892c72765 100644
--- a/src/include/storage/proc.h
+++ b/src/include/storage/proc.h
@@ -428,6 +428,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 c068986d09..c8fa0dc3d9 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/expected/timeouts.out b/src/test/isolation/expected/timeouts.out
index 9328676f1c..6325ab4d00 100644
--- a/src/test/isolation/expected/timeouts.out
+++ b/src/test/isolation/expected/timeouts.out
@@ -79,3 +79,21 @@ 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: tsto sleep0 sleep1000
+step tsto: SET transaction_timeout = '30ms'; SET statement_timeout = '10s';
+step sleep0: SELECT pg_sleep(0.0001)
+pg_sleep
+--------
+        
+(1 row)
+
+step sleep1000: SELECT pg_sleep(1000) <waiting ...>
+step sleep1000: <... completed>
+ERROR:  canceling statement due to transaction timeout
+
+starting permutation: stto sleep1000
+step stto: SET transaction_timeout = '10s'; SET statement_timeout = '10ms';
+step sleep1000: SELECT pg_sleep(1000) <waiting ...>
+step sleep1000: <... completed>
+ERROR:  canceling statement due to statement timeout
diff --git a/src/test/isolation/specs/timeouts.spec b/src/test/isolation/specs/timeouts.spec
index c747b4ae28..82d7fc501a 100644
--- a/src/test/isolation/specs/timeouts.spec
+++ b/src/test/isolation/specs/timeouts.spec
@@ -23,6 +23,10 @@ step sto	{ SET statement_timeout = '10ms'; }
 step lto	{ SET lock_timeout = '10ms'; }
 step lsto	{ SET lock_timeout = '10ms'; SET statement_timeout = '10s'; }
 step slto	{ SET lock_timeout = '10s'; SET statement_timeout = '10ms'; }
+step tsto	{ SET transaction_timeout = '30ms'; SET statement_timeout = '10s';}
+step stto	{ SET transaction_timeout = '10s'; SET statement_timeout = '10ms';}
+step sleep0	{ SELECT pg_sleep(0.0001) } # expected to always finish in time
+step sleep1000	{ SELECT pg_sleep(1000) } # expected to timeout always
 step locktbl	{ LOCK TABLE accounts; }
 step update	{ DELETE FROM accounts WHERE accountid = 'checking'; }
 teardown	{ ABORT; }
@@ -47,3 +51,7 @@ permutation wrtbl lto update(*)
 permutation wrtbl lsto update(*)
 # statement timeout expires first, row-level lock
 permutation wrtbl slto update(*)
+# transaction timeout before statement timeout
+permutation tsto sleep0 sleep1000(*)
+# statement timeout before transaction timeout
+permutation stto sleep1000(*)
\ No newline at end of file
-- 
2.37.0 (Apple Git-136)