Followup fixes for transaction_timeout

akorotkov · akorotkov · commit bf82f43790a6 · 2024-02-16T03:36:38.000+02:00
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
diff --git a/src/backend/access/transam/xact.c b/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();
diff --git a/src/backend/tcop/postgres.c b/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 &&
-		get_timeout_active(TRANSACTION_TIMEOUT))
-		disable_timeout(TRANSACTION_TIMEOUT, false);
+	if (IsTransactionState())
+	{
+		/*
+		 * 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 */
diff --git a/src/test/isolation/isolation_schedule b/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
diff --git a/src/test/isolation/specs/timeouts-long.spec b/src/test/isolation/specs/timeouts-long.spec

Original file line number	Diff line number	Diff line change
`@@ -3647,9 +3647,17 @@ check_log_stats(bool newval, void *extra, GucSource source)`
`3647`	`3647`	`void`
`3648`	`3648`	`assign_transaction_timeout(int newval, void *extra)`
`3649`	`3649`	`{`
`3650`		`- if (TransactionTimeout <= 0 &&`
`3651`		`- get_timeout_active(TRANSACTION_TIMEOUT))`
`3652`		`- disable_timeout(TRANSACTION_TIMEOUT, false);`
	`3650`	`+ if (IsTransactionState())`
	`3651`	`+ {`
	`3652`	`+ /*`
	`3653`	`+ * If transaction_timeout GUC has changes within the transaction block`
	`3654`	`+ * enable or disable the timer correspondingly.`
	`3655`	`+ */`
	`3656`	`+ if (newval > 0 && !get_timeout_active(TRANSACTION_TIMEOUT))`
	`3657`	`+ enable_timeout_after(TRANSACTION_TIMEOUT, newval);`
	`3658`	`+ else if (newval <= 0 && get_timeout_active(TRANSACTION_TIMEOUT))`
	`3659`	`+ disable_timeout(TRANSACTION_TIMEOUT, false);`
	`3660`	`+ }`
`3653`	`3661`	`}`
`3654`	`3662`
`3655`	`3663`
`@@ -4510,11 +4518,6 @@ PostgresMain(const char dbname, const char username)`
`4510`	`4518`	`enable_timeout_after(IDLE_IN_TRANSACTION_SESSION_TIMEOUT,`
`4511`	`4519`	`IdleInTransactionSessionTimeout);`
`4512`	`4520`	`}`
`4513`		`-`
`4514`		`- /* Schedule or reschedule transaction timeout */`
`4515`		`- if (TransactionTimeout > 0 && !get_timeout_active(TRANSACTION_TIMEOUT))`
`4516`		`- enable_timeout_after(TRANSACTION_TIMEOUT,`
`4517`		`- TransactionTimeout);`
`4518`	`4521`	`}`
`4519`	`4522`	`else if (IsTransactionOrTransactionBlock())`
`4520`	`4523`	`{`
`@@ -4529,11 +4532,6 @@ PostgresMain(const char dbname, const char username)`
`4529`	`4532`	`enable_timeout_after(IDLE_IN_TRANSACTION_SESSION_TIMEOUT,`
`4530`	`4533`	`IdleInTransactionSessionTimeout);`
`4531`	`4534`	`}`
`4532`		`-`
`4533`		`- /* Schedule or reschedule transaction timeout */`
`4534`		`- if (TransactionTimeout > 0 && !get_timeout_active(TRANSACTION_TIMEOUT))`
`4535`		`- enable_timeout_after(TRANSACTION_TIMEOUT,`
`4536`		`- TransactionTimeout);`
`4537`	`4535`	`}`
`4538`	`4536`	`else`
`4539`	`4537`	`{`
`@@ -4586,13 +4584,6 @@ PostgresMain(const char dbname, const char username)`
`4586`	`4584`	`enable_timeout_after(IDLE_SESSION_TIMEOUT,`
`4587`	`4585`	`IdleSessionTimeout);`
`4588`	`4586`	`}`
`4589`		`-`
`4590`		`- /*`
`4591`		`- * If GUC is changed then it's handled in`
`4592`		`- * assign_transaction_timeout().`
`4593`		`- */`
`4594`		`- if (TransactionTimeout > 0 && get_timeout_active(TRANSACTION_TIMEOUT))`
`4595`		`- disable_timeout(TRANSACTION_TIMEOUT, false);`
`4596`	`4587`	`}`
`4597`	`4588`
`4598`	`4589`	`/* Report any recently-changed GUC options */`