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

diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index a124ba5933030293213b779a2e3ed5a3eb45fb59..70ab6e27a13f6aa17b24bbb04ef907165fa73cba 100644 (file)
--- 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
index de9f5d1a6c455ad9635712b15db4f9caaeb3604d..2c63b7875a3567062fb79b21b82788ce452026cd 100644 (file)
--- 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
index 86ef62bbcf6feaa5af6025f7b23551c547498a99..b2be88ead1d2597c4b54805930ba8e9b310927b7 100644 (file)
--- 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
deleted file mode 100644 (file)
index ce2c9a4..0000000
--- a/src/test/isolation/specs/timeouts-long.spec
+++ /dev/null
@@ -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