Followup fixes for transaction_timeout

Don't deal with transaction timeout in PostgresMain().  Instead, release
transaction timeout activated by StartTransaction() in
CommitTransaction()/AbortTransaction()/PrepareTransaction().  Deal with both
enabling and disabling transaction timeout in assign_transaction_timeout().

Also, remove potentially flaky timeouts-long isolation test, which has no
guarantees to pass on slow/busy machines.

Reported-by: Andres Freund
Discussion: https://postgr.es/m/20240215230856.pc6k57tqxt7fhldm%40awork3.anarazel.de

src/backend/access/transam/xact.c

@@ -2262,6 +2262,10 @@ CommitTransaction(void)
 	s->state = TRANS_COMMIT;
 	s->parallelModeLevel = 0;
 
+	/* Disable transaction timeout */
+	if (TransactionTimeout > 0)
+		disable_timeout(TRANSACTION_TIMEOUT, false);
+
 	if (!is_parallel_worker)
 	{
 		/*
@@ -2535,6 +2539,10 @@ PrepareTransaction(void)
 	 */
 	s->state = TRANS_PREPARE;
 
+	/* Disable transaction timeout */
+	if (TransactionTimeout > 0)
+		disable_timeout(TRANSACTION_TIMEOUT, false);
+
 	prepared_at = GetCurrentTimestamp();
 
 	/*
@@ -2707,6 +2715,10 @@ AbortTransaction(void)
 	/* Prevent cancel/die interrupt while cleaning up */
 	HOLD_INTERRUPTS();
 
+	/* Disable transaction timeout */
+	if (TransactionTimeout > 0)
+		disable_timeout(TRANSACTION_TIMEOUT, false);
+
 	/* Make sure we have a valid memory context and resource owner */
 	AtAbort_Memory();
 	AtAbort_ResourceOwner();

src/backend/tcop/postgres.c

@@ -3647,9 +3647,17 @@ check_log_stats(bool *newval, void **extra, GucSource source)
 void
 assign_transaction_timeout(int newval, void *extra)
 {
-	if (TransactionTimeout <= 0 &&
+	if (IsTransactionState())
-		get_timeout_active(TRANSACTION_TIMEOUT))
+	{
+		/*
+		 * If transaction_timeout GUC has changes within the transaction block
+		 * enable or disable the timer correspondingly.
+		 */
+		if (newval > 0 && !get_timeout_active(TRANSACTION_TIMEOUT))
+			enable_timeout_after(TRANSACTION_TIMEOUT, newval);
+		else if (newval <= 0 && get_timeout_active(TRANSACTION_TIMEOUT))
 			disable_timeout(TRANSACTION_TIMEOUT, false);
+	}
 }
 
 
@@ -4510,11 +4518,6 @@ 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())
 			{
@@ -4529,11 +4532,6 @@ 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
 			{
@@ -4586,13 +4584,6 @@ PostgresMain(const char *dbname, const char *username)
 					enable_timeout_after(IDLE_SESSION_TIMEOUT,
 										 IdleSessionTimeout);
 				}
-
-				/*
-				 * If GUC is changed then it's handled in
-				 * assign_transaction_timeout().
-				 */
-				if (TransactionTimeout > 0 && get_timeout_active(TRANSACTION_TIMEOUT))
-					disable_timeout(TRANSACTION_TIMEOUT, false);
 			}
 
 			/* Report any recently-changed GUC options */

src/test/isolation/isolation_schedule

@@ -89,7 +89,6 @@ test: sequence-ddl
 test: async-notify
 test: vacuum-no-cleanup-lock
 test: timeouts
-test: timeouts-long
 test: vacuum-concurrent-drop
 test: vacuum-conflict
 test: vacuum-skip-locked

src/test/isolation/specs/timeouts-long.spec

@@ -1,35 +0,0 @@
-# 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); }
-step s7_check	{ SELECT count(*) FROM pg_stat_activity WHERE application_name = 'isolation/timeouts/s7'; }
-step s8_check	{ SELECT count(*) FROM pg_stat_activity WHERE application_name = 'isolation/timeouts/s8'; }
-
-# COMMIT AND CHAIN must restart transaction timeout
-permutation s7_begin s7_sleep s7_commit_and_chain s7_sleep s7_check s7_abort
-# transaction timeout expires in presence of query flow, session s7 FATAL-out
-# this relatevely long sleeps are picked to ensure 300ms gap between check and timeouts firing
-# expected flow: timeouts is scheduled after s8_begin and fires approximately after checker_sleep (300ms before check)
-# possible buggy flow: timeout is schedules after s8_select_1 and fires 300ms after s8_check
-# to ensure this 300ms gap we need minimum transaction_timeout of 300ms
-permutation s8_begin s8_sleep s8_select_1 s8_check checker_sleep checker_sleep s8_check