From bf82f43790a675dd1b9522a7799357e61e7aa635 Mon Sep 17 00:00:00 2001 From: Alexander Korotkov Date: Fri, 16 Feb 2024 03:36:38 +0200 Subject: [PATCH] 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 | 12 +++++++ src/backend/tcop/postgres.c | 31 +++++++----------- src/test/isolation/isolation_schedule | 1 - src/test/isolation/specs/timeouts-long.spec | 35 --------------------- 4 files changed, 23 insertions(+), 56 deletions(-) delete mode 100644 src/test/isolation/specs/timeouts-long.spec diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c index a124ba5933..70ab6e27a1 100644 --- 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 de9f5d1a6c..2c63b7875a 100644 --- 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 86ef62bbcf..b2be88ead1 100644 --- 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 index ce2c9a4301..0000000000 --- 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 -- 2.39.5