From b7bde97801877d6e8c1fd16e58b4cf4e9d07ef1b Mon Sep 17 00:00:00 2001 From: "Andrey M. Borodin" Date: Sun, 3 Dec 2023 23:18:00 +0500 Subject: [PATCH v12] Introduce transaction_timeout This commit adds timeout that is expected to be used as a prevention of long-running queries. Any session within transaction will be terminated after spanning longer than this timeout. However, this timeout is not applied to prepared transactions. Only transactions with user connections are affected. Author: Andrey Borodin Reviewed-by: Nikolay Samokhvalov Reviewed-by: Andres Freund Reviewed-by: Fujii Masao Reviewed-by: bt23nguyent Reviewed-by: Yuhang Qiu Reviewed-by: Japin Li Discussion: https://postgr.es/m/CAAhFRxiQsRs2Eq5kCo9nXE3HTugsAAJdSQSmxncivebAxdmBjQ%40mail.gmail.com --- doc/src/sgml/config.sgml | 35 ++++++++++++++++ src/backend/postmaster/autovacuum.c | 2 + src/backend/storage/lmgr/proc.c | 1 + src/backend/tcop/postgres.c | 27 ++++++++++-- src/backend/utils/errcodes.txt | 1 + src/backend/utils/init/globals.c | 1 + src/backend/utils/init/postinit.c | 10 +++++ src/backend/utils/misc/guc_tables.c | 11 +++++ src/backend/utils/misc/postgresql.conf.sample | 1 + src/bin/pg_dump/pg_backup_archiver.c | 2 + 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/Makefile | 5 ++- src/test/isolation/expected/timeouts.out | 41 ++++++++++++++++++- src/test/isolation/specs/timeouts.spec | 29 ++++++++++++- 18 files changed, 166 insertions(+), 6 deletions(-) diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index 94d1eb2b81..d62673051b 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -9075,6 +9075,41 @@ COPY postgres_log FROM '/full/path/to/logfile.csv' WITH csv; + + transaction_timeout (integer) + + transaction_timeout configuration parameter + + + + + Terminate any session that spans longer than the specified amount of + time in transaction. The limit applies both to explicit transactions + (started with BEGIN) and to implicitly started + transaction corresponding to single statement. But this limit is not + applied to prepared transactions. + If this value is specified without units, it is taken as milliseconds. + A value of zero (the default) disables the timeout. + + + + If transaction_timeout is shorter than + idle_in_transaction_session_timeout or statement_timeout + transaction_timeout will invalidate longer timeout. + + + + Setting transaction_timeout in + postgresql.conf is not recommended because it would + affect all sessions. + + + + Prepared transactions are not subject for this timeout. + + + + lock_timeout (integer) diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c index b04fcfc8c8..e6fa1cfdc2 100644 --- a/src/backend/postmaster/autovacuum.c +++ b/src/backend/postmaster/autovacuum.c @@ -586,6 +586,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); @@ -1591,6 +1592,7 @@ AutoVacWorkerMain(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 b6451d9d08..4be06c1e5d 100644 --- a/src/backend/storage/lmgr/proc.c +++ b/src/backend/storage/lmgr/proc.c @@ -59,6 +59,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 7298a187d1..a2611cf8e6 100644 --- a/src/backend/tcop/postgres.c +++ b/src/backend/tcop/postgres.c @@ -2745,6 +2745,10 @@ start_xact_command(void) { StartTransactionCommand(); + /* Schedule or reschedule transaction timeout */ + if (TransactionTimeout > 0) + enable_timeout_after(TRANSACTION_TIMEOUT, TransactionTimeout); + xact_started = true; } @@ -3426,6 +3430,17 @@ ProcessInterrupts(void) IdleInTransactionSessionTimeoutPending = false; } + if (TransactionTimeoutPending) + { + /* As above, ignore the signal if the GUC has been reset to zero. */ + if (TransactionTimeout > 0) + ereport(FATAL, + (errcode(ERRCODE_TRANSACTION_TIMEOUT), + errmsg("terminating connection due to transaction timeout"))); + else + TransactionTimeoutPending = false; + } + if (IdleSessionTimeoutPending) { /* As above, ignore the signal if the GUC has been reset to zero. */ @@ -4491,7 +4506,8 @@ PostgresMain(const char *dbname, const char *username) pgstat_report_activity(STATE_IDLEINTRANSACTION_ABORTED, NULL); /* Start the idle-in-transaction timer */ - if (IdleInTransactionSessionTimeout > 0) + if (IdleInTransactionSessionTimeout > 0 + && (IdleInTransactionSessionTimeout < TransactionTimeout || TransactionTimeout == 0)) { idle_in_transaction_timeout_enabled = true; enable_timeout_after(IDLE_IN_TRANSACTION_SESSION_TIMEOUT, @@ -4504,7 +4520,8 @@ PostgresMain(const char *dbname, const char *username) pgstat_report_activity(STATE_IDLEINTRANSACTION, NULL); /* Start the idle-in-transaction timer */ - if (IdleInTransactionSessionTimeout > 0) + if (IdleInTransactionSessionTimeout > 0 + && (IdleInTransactionSessionTimeout < TransactionTimeout || TransactionTimeout == 0)) { idle_in_transaction_timeout_enabled = true; enable_timeout_after(IDLE_IN_TRANSACTION_SESSION_TIMEOUT, @@ -4562,6 +4579,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 */ @@ -5120,7 +5140,8 @@ enable_statement_timeout(void) /* must be within an xact */ Assert(xact_started); - if (StatementTimeout > 0) + if (StatementTimeout > 0 + && (StatementTimeout < TransactionTimeout || TransactionTimeout == 0)) { if (!get_timeout_active(STATEMENT_TIMEOUT)) enable_timeout_after(STATEMENT_TIMEOUT, StatementTimeout); diff --git a/src/backend/utils/errcodes.txt b/src/backend/utils/errcodes.txt index 8e97a0150f..8f1157afee 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/globals.c b/src/backend/utils/init/globals.c index 60bc1217fb..fd586c193c 100644 --- a/src/backend/utils/init/globals.c +++ b/src/backend/utils/init/globals.c @@ -33,6 +33,7 @@ volatile sig_atomic_t ProcDiePending = false; volatile sig_atomic_t CheckClientConnectionPending = false; volatile sig_atomic_t ClientConnectionLost = false; volatile sig_atomic_t IdleInTransactionSessionTimeoutPending = false; +volatile sig_atomic_t TransactionTimeoutPending = false; volatile sig_atomic_t IdleSessionTimeoutPending = false; volatile sig_atomic_t ProcSignalBarrierPending = false; volatile sig_atomic_t LogMemoryContextPending = false; diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c index 552cf9d950..64be4de0c7 100644 --- a/src/backend/utils/init/postinit.c +++ b/src/backend/utils/init/postinit.c @@ -75,6 +75,7 @@ static void ShutdownPostgres(int code, Datum arg); static void StatementTimeoutHandler(void); static void LockTimeoutHandler(void); static void IdleInTransactionSessionTimeoutHandler(void); +static void TransactionTimeoutHandler(void); static void IdleSessionTimeoutHandler(void); static void IdleStatsUpdateTimeoutHandler(void); static void ClientCheckTimeoutHandler(void); @@ -764,6 +765,7 @@ InitPostgres(const char *in_dbname, Oid dboid, RegisterTimeout(LOCK_TIMEOUT, LockTimeoutHandler); RegisterTimeout(IDLE_IN_TRANSACTION_SESSION_TIMEOUT, IdleInTransactionSessionTimeoutHandler); + RegisterTimeout(TRANSACTION_TIMEOUT, TransactionTimeoutHandler); RegisterTimeout(IDLE_SESSION_TIMEOUT, IdleSessionTimeoutHandler); RegisterTimeout(CLIENT_CONNECTION_CHECK_TIMEOUT, ClientCheckTimeoutHandler); RegisterTimeout(IDLE_STATS_UPDATE_TIMEOUT, @@ -1395,6 +1397,14 @@ LockTimeoutHandler(void) kill(MyProcPid, SIGINT); } +static void +TransactionTimeoutHandler(void) +{ + TransactionTimeoutPending = true; + InterruptPending = true; + SetLatch(MyLatch); +} + static void IdleInTransactionSessionTimeoutHandler(void) { diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c index 6474e35ec0..4291a8cb01 100644 --- a/src/backend/utils/misc/guc_tables.c +++ b/src/backend/utils/misc/guc_tables.c @@ -2544,6 +2544,17 @@ struct config_int ConfigureNamesInt[] = NULL, NULL, NULL }, + { + {"transaction_timeout", PGC_USERSET, CLIENT_CONN_STATEMENT, + gettext_noop("Sets the maximum allowed time in a transaction with session (not a prepared 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 cf9f283cfe..07ebec7709 100644 --- a/src/backend/utils/misc/postgresql.conf.sample +++ b/src/backend/utils/misc/postgresql.conf.sample @@ -695,6 +695,7 @@ #default_transaction_deferrable = off #session_replication_role = 'origin' #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 diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c index 256d1e35a4..3342971bd0 100644 --- a/src/bin/pg_dump/pg_backup_archiver.c +++ b/src/bin/pg_dump/pg_backup_archiver.c @@ -3115,6 +3115,8 @@ _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"); + // TODO: AB: do we need spacial handling for this? + 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 8c0b5486b9..21bd16ef00 100644 --- a/src/bin/pg_dump/pg_dump.c +++ b/src/bin/pg_dump/pg_dump.c @@ -1251,6 +1251,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 >= 170000) + 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 417c74cfef..9cda3f3667 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 f0cc651435..732ca7d0f6 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 4b25961249..a49a83607f 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 8a61853371..608a83d5a8 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/Makefile b/src/test/isolation/Makefile index ade2256ed3..2bd06f8f15 100644 --- a/src/test/isolation/Makefile +++ b/src/test/isolation/Makefile @@ -62,7 +62,7 @@ installcheck: all $(pg_isolation_regress_installcheck) --schedule=$(srcdir)/isolation_schedule check: all - $(pg_isolation_regress_check) --schedule=$(srcdir)/isolation_schedule + $(pg_isolation_regress_check) timeouts # Non-default tests. It only makes sense to run these if set up to use # prepared transactions, via TEMP_CONFIG for the check case, or via the @@ -72,3 +72,6 @@ installcheck-prepared-txns: all temp-install check-prepared-txns: all temp-install $(pg_isolation_regress_check) --schedule=$(srcdir)/isolation_schedule prepared-transactions prepared-transactions-cic + +check-timeouts: all temp-install + $(pg_isolation_regress_check) timeouts diff --git a/src/test/isolation/expected/timeouts.out b/src/test/isolation/expected/timeouts.out index 9328676f1c..34f884bbba 100644 --- a/src/test/isolation/expected/timeouts.out +++ b/src/test/isolation/expected/timeouts.out @@ -1,4 +1,4 @@ -Parsed test spec with 2 sessions +Parsed test spec with 6 sessions starting permutation: rdtbl sto locktbl step rdtbl: SELECT * FROM accounts; @@ -79,3 +79,42 @@ step slto: SET lock_timeout = '10s'; SET statement_timeout = '10ms'; step update: DELETE FROM accounts WHERE accountid = 'checking'; step update: <... completed> ERROR: canceling statement due to statement timeout + +starting permutation: stt1_set stt1_begin sleep_here stt2_set stt2_begin sleep_there stt3_check_stt2 itt4_set itt4_begin sleep_there stt3_check_itt4 +step stt1_set: SET transaction_timeout = '1ms'; +step stt1_begin: BEGIN ISOLATION LEVEL READ COMMITTED; +step sleep_here: SELECT pg_sleep(0.01); +FATAL: terminating connection due to transaction timeout +server closed the connection unexpectedly + This probably means the server terminated abnormally + before or while processing the request. + +step stt2_set: SET transaction_timeout = '1ms'; +step stt2_begin: BEGIN ISOLATION LEVEL READ COMMITTED; +step sleep_there: SELECT pg_sleep(0.01); +pg_sleep +-------- + +(1 row) + +step stt3_check_stt2: SELECT count(*) FROM pg_stat_activity WHERE application_name = 'isolation/timeouts/stt2' +count +----- + 0 +(1 row) + +step itt4_set: SET idle_in_transaction_session_timeout = '1ms'; SET statement_timeout = '10s'; SET lock_timeout = '10s'; SET transaction_timeout = '10s'; +step itt4_begin: BEGIN ISOLATION LEVEL READ COMMITTED; +step sleep_there: SELECT pg_sleep(0.01); +pg_sleep +-------- + +(1 row) + +step stt3_check_itt4: SELECT count(*) FROM pg_stat_activity WHERE application_name = 'isolation/timeouts/itt4' +step stt3_check_itt4: <... completed> +count +----- + 0 +(1 row) + diff --git a/src/test/isolation/specs/timeouts.spec b/src/test/isolation/specs/timeouts.spec index c747b4ae28..7f62285923 100644 --- a/src/test/isolation/specs/timeouts.spec +++ b/src/test/isolation/specs/timeouts.spec @@ -18,7 +18,7 @@ step wrtbl { UPDATE accounts SET balance = balance + 100; } teardown { ABORT; } session s2 -setup { BEGIN ISOLATION LEVEL READ COMMITTED; } +setup { SET transaction_timeout = '10s'; SET idle_in_transaction_session_timeout = '10s'; BEGIN ISOLATION LEVEL READ COMMITTED; } step sto { SET statement_timeout = '10ms'; } step lto { SET lock_timeout = '10ms'; } step lsto { SET lock_timeout = '10ms'; SET statement_timeout = '10s'; } @@ -27,6 +27,29 @@ step locktbl { LOCK TABLE accounts; } step update { DELETE FROM accounts WHERE accountid = 'checking'; } teardown { ABORT; } +session stt1 +# enable statement_timeout to check interaction +setup { SET statement_timeout = '10s'; SET lock_timeout = '10s'; } +step stt1_set { SET transaction_timeout = '1ms'; } +step stt1_begin { BEGIN ISOLATION LEVEL READ COMMITTED; } +step sleep_here { SELECT pg_sleep(0.01); } + +session stt2 +setup { SET statement_timeout = '10s'; SET lock_timeout = '10s'; } +step stt2_set { SET transaction_timeout = '1ms'; } +step stt2_begin { BEGIN ISOLATION LEVEL READ COMMITTED; } +# Session stt2 is terminated in the background. However, isolation tester needs a step to observe it. + +session stt3 +step sleep_there{ SELECT pg_sleep(0.01); } +# Observe that stt2\itt4 died +step stt3_check_stt2 { SELECT count(*) FROM pg_stat_activity WHERE application_name = 'isolation/timeouts/stt2' } +step stt3_check_itt4 { SELECT count(*) FROM pg_stat_activity WHERE application_name = 'isolation/timeouts/itt4' } + +session itt4 +step itt4_set { SET idle_in_transaction_session_timeout = '1ms'; SET statement_timeout = '10s'; SET lock_timeout = '10s'; SET transaction_timeout = '10s'; } +step itt4_begin { BEGIN ISOLATION LEVEL READ COMMITTED; } + # It's possible that the isolation tester will not observe the final # steps as "waiting", thanks to the relatively short timeouts we use. # We can ensure consistent test output by marking those steps with (*). @@ -47,3 +70,7 @@ permutation wrtbl lto update(*) permutation wrtbl lsto update(*) # statement timeout expires first, row-level lock permutation wrtbl slto update(*) + +# timeout of active query, idle transaction timeout +permutation stt1_set stt1_begin sleep_here stt2_set stt2_begin sleep_there stt3_check_stt2 itt4_set itt4_begin sleep_there stt3_check_itt4(*) +# can't run tests after this, sessions stt1, stt2, and itt4 are expected to FATAL-out -- 2.42.0