public inbox for [email protected]
help / color / mirror / Atom feedtransaction timeout
22+ messages / 10 participants
[nested] [flat]
* transaction timeout
@ 2001-03-17 01:10 Kevin T. Manley <[email protected]>
0 siblings, 0 replies; 22+ messages in thread
From: Kevin T. Manley @ 2001-03-17 01:10 UTC (permalink / raw)
To: pgsql-hackers
Is there a timeout setting I can use to abort transactions that aren't
deadlocked, but which have been blocked waiting for locks greater than some
amount of time? I didn't see anything in the docs on this and observed with
2 instances of psql that a transaction waiting on a lock seems to wait
forever.
If pgsql doesn't have such a setting, has there been any discussion about
adding it?
Regards,
Kevin Manley
^ permalink raw reply [nested|flat] 22+ messages in thread
* Transaction timeout
@ 2022-12-03 05:18 Andrey Borodin <[email protected]>
2022-12-03 06:59 ` Re: Transaction timeout Nikolay Samokhvalov <[email protected]>
2024-01-22 06:23 ` Re: Transaction timeout Peter Smith <[email protected]>
0 siblings, 2 replies; 22+ messages in thread
From: Andrey Borodin @ 2022-12-03 05:18 UTC (permalink / raw)
To: Nikolay Samokhvalov <[email protected]>; pgsql-hackers
Hello,
We have statement_timeout, idle_in_transaction_timeout,
idle_session_timeout and many more! But we have no
transaction_timeout. I've skimmed thread [0,1] about existing timeouts
and found no contraindications to implement transaction_timeout.
Nikolay asked me if I can prototype the feature for testing by him,
and it seems straightforward. Please find attached. If it's not known
to be a bad idea - we'll work on it.
Thanks!
Best regards, Andrey Borodin.
[0] https://www.postgresql.org/message-id/flat/763A0689-F189-459E-946F-F0EC4458980B%40hotmail.com
Attachments:
[application/octet-stream] v1-0001-Intorduce-transaction_timeout.patch (9.8K, ../../CAAhFRxiQsRs2Eq5kCo9nXE3HTugsAAJdSQSmxncivebAxdmBjQ@mail.gmail.com/2-v1-0001-Intorduce-transaction_timeout.patch)
download | inline diff:
From 91b73b077f6b5d96aeb8f386a0fa6b6a156e27d1 Mon Sep 17 00:00:00 2001
From: Andrey Borodin <[email protected]>
Date: Fri, 2 Dec 2022 21:01:29 -0800
Subject: [PATCH v1] Intorduce transaction_timeout
Just like statement_timeout, but for transaction.
---
src/backend/postmaster/autovacuum.c | 1 +
src/backend/storage/lmgr/proc.c | 1 +
src/backend/tcop/postgres.c | 18 ++++++++++++++++++
src/backend/utils/errcodes.txt | 1 +
src/backend/utils/init/postinit.c | 13 +++++++++++++
src/backend/utils/misc/guc_tables.c | 11 +++++++++++
src/bin/pg_dump/pg_backup_archiver.c | 1 +
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 +
12 files changed, 52 insertions(+)
diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index 601834d4b4..828a28af0a 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -588,6 +588,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);
diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c
index b1c35653fc..0170e226d0 100644
--- a/src/backend/storage/lmgr/proc.c
+++ b/src/backend/storage/lmgr/proc.c
@@ -61,6 +61,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 3082093d1e..45db2bdbb5 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -2691,6 +2691,10 @@ start_xact_command(void)
{
StartTransactionCommand();
+ Assert(!get_timeout_active(TRANSACTION_TIMEOUT));
+ if (TransactionTimeout > 0)
+ enable_timeout_after(TRANSACTION_TIMEOUT, TransactionTimeout);
+
xact_started = true;
}
@@ -2720,6 +2724,7 @@ finish_xact_command(void)
if (xact_started)
{
+
CommitTransactionCommand();
#ifdef MEMORY_CONTEXT_CHECKING
@@ -3268,6 +3273,7 @@ ProcessInterrupts(void)
{
bool lock_timeout_occurred;
bool stmt_timeout_occurred;
+ bool tx_timeout_occurred;
QueryCancelPending = false;
@@ -3277,6 +3283,7 @@ ProcessInterrupts(void)
*/
lock_timeout_occurred = get_timeout_indicator(LOCK_TIMEOUT, true);
stmt_timeout_occurred = get_timeout_indicator(STATEMENT_TIMEOUT, true);
+ tx_timeout_occurred = get_timeout_indicator(TRANSACTION_TIMEOUT, true);
/*
* If both were set, we want to report whichever timeout completed
@@ -3302,6 +3309,13 @@ ProcessInterrupts(void)
(errcode(ERRCODE_QUERY_CANCELED),
errmsg("canceling statement due to statement timeout")));
}
+ if (tx_timeout_occurred)
+ {
+ LockErrorCleanup();
+ ereport(ERROR,
+ (errcode(ERRCODE_TRANSACTION_TIMEOUT),
+ errmsg("canceling transaction due to transaction timeout")));
+ }
if (IsAutoVacuumWorkerProcess())
{
LockErrorCleanup();
@@ -4460,6 +4474,10 @@ 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 */
diff --git a/src/backend/utils/errcodes.txt b/src/backend/utils/errcodes.txt
index 62418a051a..3ae2bbda70 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/postinit.c b/src/backend/utils/init/postinit.c
index a990c833c5..56d11b5e62 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);
@@ -756,6 +757,8 @@ 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,
@@ -1360,6 +1363,16 @@ IdleInTransactionSessionTimeoutHandler(void)
SetLatch(MyLatch);
}
+static void
+TransactionTimeoutHandler(void)
+{
+#ifdef HAVE_SETSID
+ /* try to signal whole process group */
+ kill(-MyProcPid, SIGINT);
+#endif
+ kill(MyProcPid, SIGINT);
+}
+
static void
IdleSessionTimeoutHandler(void)
{
diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c
index 1bf14eec66..ca21d2544c 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -2452,6 +2452,17 @@ struct config_int ConfigureNamesInt[] =
NULL, NULL, NULL
},
+ {
+ {"transaction_timeout", PGC_USERSET, CLIENT_CONN_STATEMENT,
+ gettext_noop("Sets the maximum allowed in a 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/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c
index 0081873a72..5229fe3555 100644
--- a/src/bin/pg_dump/pg_backup_archiver.c
+++ b/src/bin/pg_dump/pg_backup_archiver.c
@@ -3089,6 +3089,7 @@ _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");
+ 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 44e8cd4704..87be74753d 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -1188,6 +1188,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 >= 160000)
+ 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 011c9cce6e..1b9674140a 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 795182fa51..484384b4ed 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 aa13e1d66e..a892c72765 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 c068986d09..c8fa0dc3d9 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,
--
2.37.0 (Apple Git-136)
^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: Transaction timeout
2022-12-03 05:18 Transaction timeout Andrey Borodin <[email protected]>
@ 2022-12-03 06:59 ` Nikolay Samokhvalov <[email protected]>
2022-12-03 17:41 ` Re: Transaction timeout Andrey Borodin <[email protected]>
1 sibling, 1 reply; 22+ messages in thread
From: Nikolay Samokhvalov @ 2022-12-03 06:59 UTC (permalink / raw)
To: Andrey Borodin <[email protected]>; +Cc: pgsql-hackers
On Fri, Dec 2, 2022 at 9:18 PM Andrey Borodin <[email protected]> wrote:
> Hello,
>
> We have statement_timeout, idle_in_transaction_timeout,
> idle_session_timeout and many more! But we have no
> transaction_timeout. I've skimmed thread [0,1] about existing timeouts
> and found no contraindications to implement transaction_timeout.
>
> Nikolay asked me if I can prototype the feature for testing by him,
> and it seems straightforward. Please find attached. If it's not known
> to be a bad idea - we'll work on it.
>
Thanks!! It was a super quick reaction to my proposal Honestly, I was
thinking about it for several years, wondering why it's still not
implemented.
The reasons to have it should be straightforward – here are a couple of
them I can see.
First one. In the OLTP context, we usually have:
- a hard timeout set in application server
- a hard timeout set in HTTP server
- users not willing to wait more than several seconds – and almost never
being ready to wait for more than 30-60s.
If Postgres allows longer transactions (it does since we cannot reliably
limit their duration now, it's always virtually unlimited), it might be
doing the work that nobody is waiting / is needing anymore, speeding
resources, affecting health, etc.
Why we cannot limit transaction duration reliably? The existing timeouts
(namely, statement_timeout + idle_session_timeout) don't protect from
having transactions consisting of a series of small statements and short
pauses between them. If such behavior happens (e.g., a long series of fast
UPDATEs in a loop). It can be dangerous, affecting general DB health (bloat
issues). This is reason number two – DBAs might want to decide to minimize
the cases of long transactions, setting transaction limits globally (and
allowing to set it locally for particular sessions or for some users in
rare cases).
Speaking of the patch – I just tested it (gitpod:
https://gitpod.io/#https://gitlab.com/NikolayS/postgres/tree/transaction_timeout);
it applies, works as expected for single-statement transactions:
postgres=# set transaction_timeout to '2s';
SET
postgres=# select pg_sleep(3);
ERROR: canceling transaction due to transaction timeout
But it fails in the "worst" case I've described above – a series of small
statements:
postgres=# set transaction_timeout to '2s';
SET
postgres=# begin; select pg_sleep(1); select pg_sleep(1); select
pg_sleep(1); select pg_sleep(1); select pg_sleep(1); commit;
BEGIN
pg_sleep
----------
(1 row)
pg_sleep
----------
(1 row)
pg_sleep
----------
(1 row)
pg_sleep
----------
(1 row)
pg_sleep
----------
(1 row)
COMMIT
postgres=#
^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: Transaction timeout
2022-12-03 05:18 Transaction timeout Andrey Borodin <[email protected]>
2022-12-03 06:59 ` Re: Transaction timeout Nikolay Samokhvalov <[email protected]>
@ 2022-12-03 17:41 ` Andrey Borodin <[email protected]>
2022-12-05 22:17 ` Re: Transaction timeout Nikolay Samokhvalov <[email protected]>
2022-12-05 23:07 ` Re: Transaction timeout Andres Freund <[email protected]>
2022-12-07 18:23 ` Re: Transaction timeout Andres Freund <[email protected]>
0 siblings, 3 replies; 22+ messages in thread
From: Andrey Borodin @ 2022-12-03 17:41 UTC (permalink / raw)
To: Nikolay Samokhvalov <[email protected]>; +Cc: pgsql-hackers
On Fri, Dec 2, 2022 at 10:59 PM Nikolay Samokhvalov
<[email protected]> wrote:
>
> But it fails in the "worst" case I've described above – a series of small statements:
Fixed. Added test for this.
Open questions:
1. Docs
2. Order of reporting if happened lock_timeout, statement_timeout, and
transaction_timeout simultaneously. Currently there's a lot of code
around this...
Thanks!
Best regards, Andrey Borodin.
Attachments:
[application/octet-stream] v2-0001-Intorduce-transaction_timeout.patch (11.8K, ../../CAAhFRxg7p+Z=xAFAdPeuLeOvc+3DyKKJtg8PKfdE453yybfHMQ@mail.gmail.com/2-v2-0001-Intorduce-transaction_timeout.patch)
download | inline diff:
From 462ef31450db71ee122706afda13587a31156107 Mon Sep 17 00:00:00 2001
From: Andrey Borodin <[email protected]>
Date: Fri, 2 Dec 2022 21:01:29 -0800
Subject: [PATCH v2] Intorduce transaction_timeout
Just like statement_timeout, but for transaction.
---
src/backend/postmaster/autovacuum.c | 1 +
src/backend/storage/lmgr/proc.c | 1 +
src/backend/tcop/postgres.c | 17 +++++++++++++++++
src/backend/utils/errcodes.txt | 1 +
src/backend/utils/init/postinit.c | 13 +++++++++++++
src/backend/utils/misc/guc_tables.c | 11 +++++++++++
src/bin/pg_dump/pg_backup_archiver.c | 1 +
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/expected/timeouts.out | 18 ++++++++++++++++++
src/test/isolation/specs/timeouts.spec | 5 +++++
14 files changed, 74 insertions(+)
diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index 601834d4b4..828a28af0a 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -588,6 +588,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);
diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c
index b1c35653fc..0170e226d0 100644
--- a/src/backend/storage/lmgr/proc.c
+++ b/src/backend/storage/lmgr/proc.c
@@ -61,6 +61,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 3082093d1e..41f3d19e95 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -2691,6 +2691,9 @@ start_xact_command(void)
{
StartTransactionCommand();
+ if (TransactionTimeout > 0 && !get_timeout_active(TRANSACTION_TIMEOUT))
+ enable_timeout_after(TRANSACTION_TIMEOUT, TransactionTimeout);
+
xact_started = true;
}
@@ -2720,6 +2723,7 @@ finish_xact_command(void)
if (xact_started)
{
+
CommitTransactionCommand();
#ifdef MEMORY_CONTEXT_CHECKING
@@ -3268,6 +3272,7 @@ ProcessInterrupts(void)
{
bool lock_timeout_occurred;
bool stmt_timeout_occurred;
+ bool tx_timeout_occurred;
QueryCancelPending = false;
@@ -3277,6 +3282,7 @@ ProcessInterrupts(void)
*/
lock_timeout_occurred = get_timeout_indicator(LOCK_TIMEOUT, true);
stmt_timeout_occurred = get_timeout_indicator(STATEMENT_TIMEOUT, true);
+ tx_timeout_occurred = get_timeout_indicator(TRANSACTION_TIMEOUT, true);
/*
* If both were set, we want to report whichever timeout completed
@@ -3302,6 +3308,13 @@ ProcessInterrupts(void)
(errcode(ERRCODE_QUERY_CANCELED),
errmsg("canceling statement due to statement timeout")));
}
+ if (tx_timeout_occurred)
+ {
+ LockErrorCleanup();
+ ereport(ERROR,
+ (errcode(ERRCODE_TRANSACTION_TIMEOUT),
+ errmsg("canceling transaction due to transaction timeout")));
+ }
if (IsAutoVacuumWorkerProcess())
{
LockErrorCleanup();
@@ -4460,6 +4473,10 @@ 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 */
diff --git a/src/backend/utils/errcodes.txt b/src/backend/utils/errcodes.txt
index 62418a051a..3ae2bbda70 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/postinit.c b/src/backend/utils/init/postinit.c
index a990c833c5..56d11b5e62 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);
@@ -756,6 +757,8 @@ 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,
@@ -1360,6 +1363,16 @@ IdleInTransactionSessionTimeoutHandler(void)
SetLatch(MyLatch);
}
+static void
+TransactionTimeoutHandler(void)
+{
+#ifdef HAVE_SETSID
+ /* try to signal whole process group */
+ kill(-MyProcPid, SIGINT);
+#endif
+ kill(MyProcPid, SIGINT);
+}
+
static void
IdleSessionTimeoutHandler(void)
{
diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c
index 1bf14eec66..ca21d2544c 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -2452,6 +2452,17 @@ struct config_int ConfigureNamesInt[] =
NULL, NULL, NULL
},
+ {
+ {"transaction_timeout", PGC_USERSET, CLIENT_CONN_STATEMENT,
+ gettext_noop("Sets the maximum allowed in a 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/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c
index 0081873a72..5229fe3555 100644
--- a/src/bin/pg_dump/pg_backup_archiver.c
+++ b/src/bin/pg_dump/pg_backup_archiver.c
@@ -3089,6 +3089,7 @@ _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");
+ 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 44e8cd4704..87be74753d 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -1188,6 +1188,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 >= 160000)
+ 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 011c9cce6e..1b9674140a 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 795182fa51..484384b4ed 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 aa13e1d66e..a892c72765 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 c068986d09..c8fa0dc3d9 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/expected/timeouts.out b/src/test/isolation/expected/timeouts.out
index 9328676f1c..d07a2fae02 100644
--- a/src/test/isolation/expected/timeouts.out
+++ b/src/test/isolation/expected/timeouts.out
@@ -79,3 +79,21 @@ step slto: SET lock_timeout = '10s'; SET statement_timeout = '10ms';
step update: DELETE FROM accounts WHERE accountid = 'checking'; <waiting ...>
step update: <... completed>
ERROR: canceling statement due to statement timeout
+
+starting permutation: tto sleep0 sleep0 sleep10
+step tto: SET transaction_timeout = '10ms';
+step sleep0: SELECT pg_sleep(0.0001)
+pg_sleep
+--------
+
+(1 row)
+
+step sleep0: SELECT pg_sleep(0.0001)
+pg_sleep
+--------
+
+(1 row)
+
+step sleep10: SELECT pg_sleep(0.01) <waiting ...>
+step sleep10: <... completed>
+ERROR: canceling transaction due to transaction timeout
diff --git a/src/test/isolation/specs/timeouts.spec b/src/test/isolation/specs/timeouts.spec
index c747b4ae28..a7f27811c7 100644
--- a/src/test/isolation/specs/timeouts.spec
+++ b/src/test/isolation/specs/timeouts.spec
@@ -23,6 +23,9 @@ step sto { SET statement_timeout = '10ms'; }
step lto { SET lock_timeout = '10ms'; }
step lsto { SET lock_timeout = '10ms'; SET statement_timeout = '10s'; }
step slto { SET lock_timeout = '10s'; SET statement_timeout = '10ms'; }
+step tto { SET transaction_timeout = '10ms'; }
+step sleep0 { SELECT pg_sleep(0.0001) }
+step sleep10 { SELECT pg_sleep(0.01) }
step locktbl { LOCK TABLE accounts; }
step update { DELETE FROM accounts WHERE accountid = 'checking'; }
teardown { ABORT; }
@@ -47,3 +50,5 @@ permutation wrtbl lto update(*)
permutation wrtbl lsto update(*)
# statement timeout expires first, row-level lock
permutation wrtbl slto update(*)
+# transaction timeout
+permutation tto sleep0 sleep0 sleep10(*)
\ No newline at end of file
--
2.37.0 (Apple Git-136)
^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: Transaction timeout
2022-12-03 05:18 Transaction timeout Andrey Borodin <[email protected]>
2022-12-03 06:59 ` Re: Transaction timeout Nikolay Samokhvalov <[email protected]>
2022-12-03 17:41 ` Re: Transaction timeout Andrey Borodin <[email protected]>
@ 2022-12-05 22:17 ` Nikolay Samokhvalov <[email protected]>
2022-12-05 22:28 ` Re: Transaction timeout Nikolay Samokhvalov <[email protected]>
2 siblings, 1 reply; 22+ messages in thread
From: Nikolay Samokhvalov @ 2022-12-05 22:17 UTC (permalink / raw)
To: Andrey Borodin <[email protected]>; +Cc: pgsql-hackers
On Sat, Dec 3, 2022 at 9:41 AM Andrey Borodin <[email protected]> wrote:
> Fixed. Added test for this.
>
Thanks! Tested (gitpod:
https://gitpod.io/#https://gitlab.com/NikolayS/postgres/tree/transaction_timeout-v2
),
works as expected.
^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: Transaction timeout
2022-12-03 05:18 Transaction timeout Andrey Borodin <[email protected]>
2022-12-03 06:59 ` Re: Transaction timeout Nikolay Samokhvalov <[email protected]>
2022-12-03 17:41 ` Re: Transaction timeout Andrey Borodin <[email protected]>
2022-12-05 22:17 ` Re: Transaction timeout Nikolay Samokhvalov <[email protected]>
@ 2022-12-05 22:28 ` Nikolay Samokhvalov <[email protected]>
0 siblings, 0 replies; 22+ messages in thread
From: Nikolay Samokhvalov @ 2022-12-05 22:28 UTC (permalink / raw)
To: [email protected]; +Cc: Andrey Borodin <[email protected]>
The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: not tested
Documentation: not tested
Tested, works as expected;
documentation is not yet added
^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: Transaction timeout
2022-12-03 05:18 Transaction timeout Andrey Borodin <[email protected]>
2022-12-03 06:59 ` Re: Transaction timeout Nikolay Samokhvalov <[email protected]>
2022-12-03 17:41 ` Re: Transaction timeout Andrey Borodin <[email protected]>
@ 2022-12-05 23:07 ` Andres Freund <[email protected]>
2022-12-05 23:41 ` Re: Transaction timeout Andrey Borodin <[email protected]>
2022-12-06 00:44 ` Re: Transaction timeout Kyotaro Horiguchi <[email protected]>
2 siblings, 2 replies; 22+ messages in thread
From: Andres Freund @ 2022-12-05 23:07 UTC (permalink / raw)
To: Andrey Borodin <[email protected]>; +Cc: Nikolay Samokhvalov <[email protected]>; pgsql-hackers
Hi,
On 2022-12-03 09:41:04 -0800, Andrey Borodin wrote:
> @@ -2720,6 +2723,7 @@ finish_xact_command(void)
>
> if (xact_started)
> {
> +
> CommitTransactionCommand();
>
> #ifdef MEMORY_CONTEXT_CHECKING
Spurious newline added.
> @@ -4460,6 +4473,10 @@ 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 */
Too many newlines added.
I'm a bit worried about adding evermore branches and function calls for
the processing of single statements. We already spend a noticable
percentage of the cycles for a single statement in PostgresMain(), this
adds additional overhead.
I'm somewhat inclined to think that we need some redesign here before we
add more overhead.
> @@ -1360,6 +1363,16 @@ IdleInTransactionSessionTimeoutHandler(void)
> SetLatch(MyLatch);
> }
>
> +static void
> +TransactionTimeoutHandler(void)
> +{
> +#ifdef HAVE_SETSID
> + /* try to signal whole process group */
> + kill(-MyProcPid, SIGINT);
> +#endif
> + kill(MyProcPid, SIGINT);
> +}
> +
Why does this use signals instead of just setting the latch like
IdleInTransactionSessionTimeoutHandler() etc?
> diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c
> index 0081873a72..5229fe3555 100644
> --- a/src/bin/pg_dump/pg_backup_archiver.c
> +++ b/src/bin/pg_dump/pg_backup_archiver.c
> @@ -3089,6 +3089,7 @@ _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");
> + ahprintf(AH, "SET transaction_timeout = 0;\n");
Hm - why is that the right thing to do?
> diff --git a/src/test/isolation/specs/timeouts.spec b/src/test/isolation/specs/timeouts.spec
> index c747b4ae28..a7f27811c7 100644
> --- a/src/test/isolation/specs/timeouts.spec
> +++ b/src/test/isolation/specs/timeouts.spec
> @@ -23,6 +23,9 @@ step sto { SET statement_timeout = '10ms'; }
> step lto { SET lock_timeout = '10ms'; }
> step lsto { SET lock_timeout = '10ms'; SET statement_timeout = '10s'; }
> step slto { SET lock_timeout = '10s'; SET statement_timeout = '10ms'; }
> +step tto { SET transaction_timeout = '10ms'; }
> +step sleep0 { SELECT pg_sleep(0.0001) }
> +step sleep10 { SELECT pg_sleep(0.01) }
> step locktbl { LOCK TABLE accounts; }
> step update { DELETE FROM accounts WHERE accountid = 'checking'; }
> teardown { ABORT; }
> @@ -47,3 +50,5 @@ permutation wrtbl lto update(*)
> permutation wrtbl lsto update(*)
> # statement timeout expires first, row-level lock
> permutation wrtbl slto update(*)
> +# transaction timeout
> +permutation tto sleep0 sleep0 sleep10(*)
> \ No newline at end of file
I don't think this is quite sufficient. I think the test should verify
that transaction timeout interacts correctly with statement timeout /
idle in tx timeout.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: Transaction timeout
2022-12-03 05:18 Transaction timeout Andrey Borodin <[email protected]>
2022-12-03 06:59 ` Re: Transaction timeout Nikolay Samokhvalov <[email protected]>
2022-12-03 17:41 ` Re: Transaction timeout Andrey Borodin <[email protected]>
2022-12-05 23:07 ` Re: Transaction timeout Andres Freund <[email protected]>
@ 2022-12-05 23:41 ` Andrey Borodin <[email protected]>
2022-12-06 00:22 ` Re: Transaction timeout Andres Freund <[email protected]>
1 sibling, 1 reply; 22+ messages in thread
From: Andrey Borodin @ 2022-12-05 23:41 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Nikolay Samokhvalov <[email protected]>; pgsql-hackers
Thanks for looking into this Andres!
On Mon, Dec 5, 2022 at 3:07 PM Andres Freund <[email protected]> wrote:
>
> I'm a bit worried about adding evermore branches and function calls for
> the processing of single statements. We already spend a noticable
> percentage of the cycles for a single statement in PostgresMain(), this
> adds additional overhead.
>
> I'm somewhat inclined to think that we need some redesign here before we
> add more overhead.
>
We can cap statement_timeout\idle_session_timeout by the budget of
transaction_timeout left.
Either way we can do batch function enable_timeouts() instead
enable_timeout_after().
Does anything of it make sense?
>
> > @@ -1360,6 +1363,16 @@ IdleInTransactionSessionTimeoutHandler(void)
> > SetLatch(MyLatch);
> > }
> >
> > +static void
> > +TransactionTimeoutHandler(void)
> > +{
> > +#ifdef HAVE_SETSID
> > + /* try to signal whole process group */
> > + kill(-MyProcPid, SIGINT);
> > +#endif
> > + kill(MyProcPid, SIGINT);
> > +}
> > +
>
> Why does this use signals instead of just setting the latch like
> IdleInTransactionSessionTimeoutHandler() etc?
I just copied statement_timeout behaviour. As I understand this
implementation is prefered if the timeout can catch the backend
running at full steam.
> > diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c
> > index 0081873a72..5229fe3555 100644
> > --- a/src/bin/pg_dump/pg_backup_archiver.c
> > +++ b/src/bin/pg_dump/pg_backup_archiver.c
> > @@ -3089,6 +3089,7 @@ _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");
> > + ahprintf(AH, "SET transaction_timeout = 0;\n");
>
> Hm - why is that the right thing to do?
Because transaction_timeout has effects of statement_timeout.
Thank you!
Best regards, Andrey Borodin.
^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: Transaction timeout
2022-12-03 05:18 Transaction timeout Andrey Borodin <[email protected]>
2022-12-03 06:59 ` Re: Transaction timeout Nikolay Samokhvalov <[email protected]>
2022-12-03 17:41 ` Re: Transaction timeout Andrey Borodin <[email protected]>
2022-12-05 23:07 ` Re: Transaction timeout Andres Freund <[email protected]>
2022-12-05 23:41 ` Re: Transaction timeout Andrey Borodin <[email protected]>
@ 2022-12-06 00:22 ` Andres Freund <[email protected]>
0 siblings, 0 replies; 22+ messages in thread
From: Andres Freund @ 2022-12-06 00:22 UTC (permalink / raw)
To: Andrey Borodin <[email protected]>; Thomas Munro <[email protected]>; +Cc: Nikolay Samokhvalov <[email protected]>; pgsql-hackers
Hi,
On 2022-12-05 15:41:29 -0800, Andrey Borodin wrote:
> Thanks for looking into this Andres!
>
> On Mon, Dec 5, 2022 at 3:07 PM Andres Freund <[email protected]> wrote:
> >
> > I'm a bit worried about adding evermore branches and function calls for
> > the processing of single statements. We already spend a noticable
> > percentage of the cycles for a single statement in PostgresMain(), this
> > adds additional overhead.
> >
> > I'm somewhat inclined to think that we need some redesign here before we
> > add more overhead.
> >
> We can cap statement_timeout\idle_session_timeout by the budget of
> transaction_timeout left.
I don't know what you mean by that.
> @@ -3277,6 +3282,7 @@ ProcessInterrupts(void)
> */
> lock_timeout_occurred = get_timeout_indicator(LOCK_TIMEOUT, true);
> stmt_timeout_occurred = get_timeout_indicator(STATEMENT_TIMEOUT, true);
> + tx_timeout_occurred = get_timeout_indicator(TRANSACTION_TIMEOUT, true);
>
> /*
> * If both were set, we want to report whichever timeout completed
This doesn't update the preceding comment, btw, which now reads oddly:
/*
* If LOCK_TIMEOUT and STATEMENT_TIMEOUT indicators are both set, we
* need to clear both, so always fetch both.
*/
> > > @@ -1360,6 +1363,16 @@ IdleInTransactionSessionTimeoutHandler(void)
> > > SetLatch(MyLatch);
> > > }
> > >
> > > +static void
> > > +TransactionTimeoutHandler(void)
> > > +{
> > > +#ifdef HAVE_SETSID
> > > + /* try to signal whole process group */
> > > + kill(-MyProcPid, SIGINT);
> > > +#endif
> > > + kill(MyProcPid, SIGINT);
> > > +}
> > > +
> >
> > Why does this use signals instead of just setting the latch like
> > IdleInTransactionSessionTimeoutHandler() etc?
>
> I just copied statement_timeout behaviour. As I understand this
> implementation is prefered if the timeout can catch the backend
> running at full steam.
Hm. I'm not particularly convinced by that code. Be that as it may, I
don't think it's a good idea to have one more copy of this code. At
least the patch should wrap the signalling code in a helper.
FWIW, the HAVE_SETSID code originates in:
commit 3ad0728c817bf8abd2c76bd11d856967509b307c
Author: Tom Lane <[email protected]>
Date: 2006-11-21 20:59:53 +0000
On systems that have setsid(2) (which should be just about everything except
Windows), arrange for each postmaster child process to be its own process
group leader, and deliver signals SIGINT, SIGTERM, SIGQUIT to the whole
process group not only the direct child process. This provides saner behavior
for archive and recovery scripts; in particular, it's possible to shut down a
warm-standby recovery server using "pg_ctl stop -m immediate", since delivery
of SIGQUIT to the startup subprocess will result in killing the waiting
recovery_command. Also, this makes Query Cancel and statement_timeout apply
to scripts being run from backends via system(). (There is no support in the
core backend for that, but it's widely done using untrusted PLs.) Per gripe
from Stephen Harris and subsequent discussion.
> > > diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c
> > > index 0081873a72..5229fe3555 100644
> > > --- a/src/bin/pg_dump/pg_backup_archiver.c
> > > +++ b/src/bin/pg_dump/pg_backup_archiver.c
> > > @@ -3089,6 +3089,7 @@ _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");
> > > + ahprintf(AH, "SET transaction_timeout = 0;\n");
> >
> > Hm - why is that the right thing to do?
> Because transaction_timeout has effects of statement_timeout.
I guess it's just following precedent - but it seems a bit presumptuous
to just disable safety settings a DBA might have set up. That makes some
sense for e.g. idle_in_transaction_session_timeout, because I think
e.g. parallel backup can lead to a connection being idle for a bit.
A few more review comments:
> Either way we can do batch function enable_timeouts() instead
> enable_timeout_after().
> Does anything of it make sense?
I'm at least as worried about the various calls *after* the execution of
a statement.
> + if (tx_timeout_occurred)
> + {
> + LockErrorCleanup();
> + ereport(ERROR,
> + (errcode(ERRCODE_TRANSACTION_TIMEOUT),
> + errmsg("canceling transaction due to transaction timeout")));
> + }
The number of calls to LockErrorCleanup() here feels wrong - there's
already 8 calls in ProcessInterrupts(). Besides the code duplication I
also think it's not a sane idea to rely on having LockErrorCleanup()
before all the relevant ereport(ERROR)s.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: Transaction timeout
2022-12-03 05:18 Transaction timeout Andrey Borodin <[email protected]>
2022-12-03 06:59 ` Re: Transaction timeout Nikolay Samokhvalov <[email protected]>
2022-12-03 17:41 ` Re: Transaction timeout Andrey Borodin <[email protected]>
2022-12-05 23:07 ` Re: Transaction timeout Andres Freund <[email protected]>
@ 2022-12-06 00:44 ` Kyotaro Horiguchi <[email protected]>
2022-12-06 01:10 ` Re: Transaction timeout Andres Freund <[email protected]>
1 sibling, 1 reply; 22+ messages in thread
From: Kyotaro Horiguchi @ 2022-12-06 00:44 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]; [email protected]; pgsql-hackers
At Mon, 5 Dec 2022 15:07:47 -0800, Andres Freund <[email protected]> wrote in
> I'm a bit worried about adding evermore branches and function calls for
> the processing of single statements. We already spend a noticable
> percentage of the cycles for a single statement in PostgresMain(), this
> adds additional overhead.
>
> I'm somewhat inclined to think that we need some redesign here before we
> add more overhead.
insert_timeout() and remove_timeout_index() move 40*(# of several
timeouts) bytes at every enabling/disabling a timeout. This is far
frequent than actually any timeout fires. schedule_alarm() is
interested only in the nearest timeout.
So, we can get rid of the insertion sort in
insert_timeout/remove_timeout_index then let them just search for the
nearest one and remember it. Finding the nearest should be faster than
the insertion sort. Instead we need to scan over the all timeouts
instead of the a few first ones, but that's overhead is not a matter
when a timeout fires.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: Transaction timeout
2022-12-03 05:18 Transaction timeout Andrey Borodin <[email protected]>
2022-12-03 06:59 ` Re: Transaction timeout Nikolay Samokhvalov <[email protected]>
2022-12-03 17:41 ` Re: Transaction timeout Andrey Borodin <[email protected]>
2022-12-05 23:07 ` Re: Transaction timeout Andres Freund <[email protected]>
2022-12-06 00:44 ` Re: Transaction timeout Kyotaro Horiguchi <[email protected]>
@ 2022-12-06 01:10 ` Andres Freund <[email protected]>
2022-12-07 02:52 ` Re: Transaction timeout Kyotaro Horiguchi <[email protected]>
0 siblings, 1 reply; 22+ messages in thread
From: Andres Freund @ 2022-12-06 01:10 UTC (permalink / raw)
To: Kyotaro Horiguchi <[email protected]>; +Cc: [email protected]; [email protected]; pgsql-hackers
Hi,
On 2022-12-06 09:44:01 +0900, Kyotaro Horiguchi wrote:
> At Mon, 5 Dec 2022 15:07:47 -0800, Andres Freund <[email protected]> wrote in
> > I'm a bit worried about adding evermore branches and function calls for
> > the processing of single statements. We already spend a noticable
> > percentage of the cycles for a single statement in PostgresMain(), this
> > adds additional overhead.
> >
> > I'm somewhat inclined to think that we need some redesign here before we
> > add more overhead.
>
> insert_timeout() and remove_timeout_index() move 40*(# of several
> timeouts) bytes at every enabling/disabling a timeout. This is far
> frequent than actually any timeout fires. schedule_alarm() is
> interested only in the nearest timeout.
> So, we can get rid of the insertion sort in
> insert_timeout/remove_timeout_index then let them just search for the
> nearest one and remember it. Finding the nearest should be faster than
> the insertion sort. Instead we need to scan over the all timeouts
> instead of the a few first ones, but that's overhead is not a matter
> when a timeout fires.
I'm most concerned about the overhead when the timeouts are *not*
enabled. And this adds a branch to start_xact_command() and a function
call for get_timeout_active(TRANSACTION_TIMEOUT) in that case. On its
own, that's not a whole lot, but it does add up. There's 10+ function
calls for timeout and ps_display purposes for every single statement.
But it's definitely also worth optimizing the timeout enabled paths. And
you're right, it looks like there's a fair bit of optimization
potential.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: Transaction timeout
2022-12-03 05:18 Transaction timeout Andrey Borodin <[email protected]>
2022-12-03 06:59 ` Re: Transaction timeout Nikolay Samokhvalov <[email protected]>
2022-12-03 17:41 ` Re: Transaction timeout Andrey Borodin <[email protected]>
2022-12-05 23:07 ` Re: Transaction timeout Andres Freund <[email protected]>
2022-12-06 00:44 ` Re: Transaction timeout Kyotaro Horiguchi <[email protected]>
2022-12-06 01:10 ` Re: Transaction timeout Andres Freund <[email protected]>
@ 2022-12-07 02:52 ` Kyotaro Horiguchi <[email protected]>
0 siblings, 0 replies; 22+ messages in thread
From: Kyotaro Horiguchi @ 2022-12-07 02:52 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]; [email protected]; pgsql-hackers
At Mon, 5 Dec 2022 17:10:50 -0800, Andres Freund <[email protected]> wrote in
> I'm most concerned about the overhead when the timeouts are *not*
> enabled. And this adds a branch to start_xact_command() and a function
> call for get_timeout_active(TRANSACTION_TIMEOUT) in that case. On its
> own, that's not a whole lot, but it does add up. There's 10+ function
> calls for timeout and ps_display purposes for every single statement.
That path seems like existing just for robustness. I inserted
"Assert(0)" just before the disable_timeout(), but make check-world
didn't fail [1]. Couldn't we get rid of that path, adding an assertion
instead? I'm not sure about other timeouts yet, though.
About disabling side, we cannot rely on StatementTimeout.
[1]
# 032_apply_delay.pl fails for me so I don't know any of the later
# tests fails.
> But it's definitely also worth optimizing the timeout enabled paths. And
> you're right, it looks like there's a fair bit of optimization
> potential.
Thanks. I'll work on that.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: Transaction timeout
2022-12-03 05:18 Transaction timeout Andrey Borodin <[email protected]>
2022-12-03 06:59 ` Re: Transaction timeout Nikolay Samokhvalov <[email protected]>
2022-12-03 17:41 ` Re: Transaction timeout Andrey Borodin <[email protected]>
@ 2022-12-07 18:23 ` Andres Freund <[email protected]>
2022-12-07 21:30 ` Re: Transaction timeout Andrey Borodin <[email protected]>
2 siblings, 1 reply; 22+ messages in thread
From: Andres Freund @ 2022-12-07 18:23 UTC (permalink / raw)
To: Andrey Borodin <[email protected]>; Michael Paquier <[email protected]>; +Cc: Nikolay Samokhvalov <[email protected]>; pgsql-hackers
Hi,
On 2022-12-03 09:41:04 -0800, Andrey Borodin wrote:
> Fixed. Added test for this.
The tests don't pass: https://cirrus-ci.com/build/4811553145356288
[00:54:35.337](1.251s) not ok 1 - no parameters missing from postgresql.conf.sample
[00:54:35.338](0.000s) # Failed test 'no parameters missing from postgresql.conf.sample'
# at /tmp/cirrus-ci-build/src/test/modules/test_misc/t/003_check_guc.pl line 81.
[00:54:35.338](0.000s) # got: '1'
# expected: '0'
I am just looking through a bunch of failing CF entries, so I'm perhaps
over-sensitized right now. But I'm a bit confused why there's so many
occasions of the tests clearly not having been run...
Michael, any reason 003_check_guc doesn't show the missing GUCs? It's not
particularly helpful to see "0 is different from 1". Seems that even just
something like
is_deeply(\@missing_from_file, [], "no parameters missing from postgresql.conf.sample");
would be a decent improvement?
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: Transaction timeout
2022-12-03 05:18 Transaction timeout Andrey Borodin <[email protected]>
2022-12-03 06:59 ` Re: Transaction timeout Nikolay Samokhvalov <[email protected]>
2022-12-03 17:41 ` Re: Transaction timeout Andrey Borodin <[email protected]>
2022-12-07 18:23 ` Re: Transaction timeout Andres Freund <[email protected]>
@ 2022-12-07 21:30 ` Andrey Borodin <[email protected]>
2022-12-18 20:53 ` Re: Transaction timeout Andrey Borodin <[email protected]>
0 siblings, 1 reply; 22+ messages in thread
From: Andrey Borodin @ 2022-12-07 21:30 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Michael Paquier <[email protected]>; Nikolay Samokhvalov <[email protected]>; pgsql-hackers
On Wed, Dec 7, 2022 at 10:23 AM Andres Freund <[email protected]> wrote:
>
> On 2022-12-03 09:41:04 -0800, Andrey Borodin wrote:
> > Fixed. Added test for this.
>
> The tests don't pass: https://cirrus-ci.com/build/4811553145356288
>
oops, sorry. Here's the fix. I hope to address other feedback on the
weekend. Thank you!
Best regards, Andrey Borodin.
Attachments:
[application/octet-stream] v3-0001-Intorduce-transaction_timeout.patch (12.5K, ../../CAAhFRxgP_w8ddYPJ3cJ4QZKQU7VWrH55MOYrxdceFBchDwuGrQ@mail.gmail.com/2-v3-0001-Intorduce-transaction_timeout.patch)
download | inline diff:
From 026cdc68e138918ddc927e7f779dca98a1eea30b Mon Sep 17 00:00:00 2001
From: Andrey Borodin <[email protected]>
Date: Fri, 2 Dec 2022 21:01:29 -0800
Subject: [PATCH v3] Intorduce transaction_timeout
Just like statement_timeout, but for transaction.
---
src/backend/postmaster/autovacuum.c | 1 +
src/backend/storage/lmgr/proc.c | 1 +
src/backend/tcop/postgres.c | 17 +++++++++++++++++
src/backend/utils/errcodes.txt | 1 +
src/backend/utils/init/postinit.c | 13 +++++++++++++
src/backend/utils/misc/guc_tables.c | 11 +++++++++++
src/backend/utils/misc/postgresql.conf.sample | 1 +
src/bin/pg_dump/pg_backup_archiver.c | 1 +
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/expected/timeouts.out | 18 ++++++++++++++++++
src/test/isolation/specs/timeouts.spec | 5 +++++
15 files changed, 75 insertions(+)
diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index 601834d4b4..828a28af0a 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -588,6 +588,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);
diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c
index b1c35653fc..0170e226d0 100644
--- a/src/backend/storage/lmgr/proc.c
+++ b/src/backend/storage/lmgr/proc.c
@@ -61,6 +61,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 3082093d1e..41f3d19e95 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -2691,6 +2691,9 @@ start_xact_command(void)
{
StartTransactionCommand();
+ if (TransactionTimeout > 0 && !get_timeout_active(TRANSACTION_TIMEOUT))
+ enable_timeout_after(TRANSACTION_TIMEOUT, TransactionTimeout);
+
xact_started = true;
}
@@ -2720,6 +2723,7 @@ finish_xact_command(void)
if (xact_started)
{
+
CommitTransactionCommand();
#ifdef MEMORY_CONTEXT_CHECKING
@@ -3268,6 +3272,7 @@ ProcessInterrupts(void)
{
bool lock_timeout_occurred;
bool stmt_timeout_occurred;
+ bool tx_timeout_occurred;
QueryCancelPending = false;
@@ -3277,6 +3282,7 @@ ProcessInterrupts(void)
*/
lock_timeout_occurred = get_timeout_indicator(LOCK_TIMEOUT, true);
stmt_timeout_occurred = get_timeout_indicator(STATEMENT_TIMEOUT, true);
+ tx_timeout_occurred = get_timeout_indicator(TRANSACTION_TIMEOUT, true);
/*
* If both were set, we want to report whichever timeout completed
@@ -3302,6 +3308,13 @@ ProcessInterrupts(void)
(errcode(ERRCODE_QUERY_CANCELED),
errmsg("canceling statement due to statement timeout")));
}
+ if (tx_timeout_occurred)
+ {
+ LockErrorCleanup();
+ ereport(ERROR,
+ (errcode(ERRCODE_TRANSACTION_TIMEOUT),
+ errmsg("canceling transaction due to transaction timeout")));
+ }
if (IsAutoVacuumWorkerProcess())
{
LockErrorCleanup();
@@ -4460,6 +4473,10 @@ 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 */
diff --git a/src/backend/utils/errcodes.txt b/src/backend/utils/errcodes.txt
index 62418a051a..3ae2bbda70 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/postinit.c b/src/backend/utils/init/postinit.c
index a990c833c5..56d11b5e62 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);
@@ -756,6 +757,8 @@ 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,
@@ -1360,6 +1363,16 @@ IdleInTransactionSessionTimeoutHandler(void)
SetLatch(MyLatch);
}
+static void
+TransactionTimeoutHandler(void)
+{
+#ifdef HAVE_SETSID
+ /* try to signal whole process group */
+ kill(-MyProcPid, SIGINT);
+#endif
+ kill(MyProcPid, SIGINT);
+}
+
static void
IdleSessionTimeoutHandler(void)
{
diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c
index 1bf14eec66..ca21d2544c 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -2452,6 +2452,17 @@ struct config_int ConfigureNamesInt[] =
NULL, NULL, NULL
},
+ {
+ {"transaction_timeout", PGC_USERSET, CLIENT_CONN_STATEMENT,
+ gettext_noop("Sets the maximum allowed in a 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 043864597f..c9c9bc6633 100644
--- a/src/backend/utils/misc/postgresql.conf.sample
+++ b/src/backend/utils/misc/postgresql.conf.sample
@@ -688,6 +688,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 0081873a72..5229fe3555 100644
--- a/src/bin/pg_dump/pg_backup_archiver.c
+++ b/src/bin/pg_dump/pg_backup_archiver.c
@@ -3089,6 +3089,7 @@ _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");
+ 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 44e8cd4704..87be74753d 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -1188,6 +1188,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 >= 160000)
+ 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 011c9cce6e..1b9674140a 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 795182fa51..484384b4ed 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 aa13e1d66e..a892c72765 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 c068986d09..c8fa0dc3d9 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/expected/timeouts.out b/src/test/isolation/expected/timeouts.out
index 9328676f1c..d07a2fae02 100644
--- a/src/test/isolation/expected/timeouts.out
+++ b/src/test/isolation/expected/timeouts.out
@@ -79,3 +79,21 @@ step slto: SET lock_timeout = '10s'; SET statement_timeout = '10ms';
step update: DELETE FROM accounts WHERE accountid = 'checking'; <waiting ...>
step update: <... completed>
ERROR: canceling statement due to statement timeout
+
+starting permutation: tto sleep0 sleep0 sleep10
+step tto: SET transaction_timeout = '10ms';
+step sleep0: SELECT pg_sleep(0.0001)
+pg_sleep
+--------
+
+(1 row)
+
+step sleep0: SELECT pg_sleep(0.0001)
+pg_sleep
+--------
+
+(1 row)
+
+step sleep10: SELECT pg_sleep(0.01) <waiting ...>
+step sleep10: <... completed>
+ERROR: canceling transaction due to transaction timeout
diff --git a/src/test/isolation/specs/timeouts.spec b/src/test/isolation/specs/timeouts.spec
index c747b4ae28..a7f27811c7 100644
--- a/src/test/isolation/specs/timeouts.spec
+++ b/src/test/isolation/specs/timeouts.spec
@@ -23,6 +23,9 @@ step sto { SET statement_timeout = '10ms'; }
step lto { SET lock_timeout = '10ms'; }
step lsto { SET lock_timeout = '10ms'; SET statement_timeout = '10s'; }
step slto { SET lock_timeout = '10s'; SET statement_timeout = '10ms'; }
+step tto { SET transaction_timeout = '10ms'; }
+step sleep0 { SELECT pg_sleep(0.0001) }
+step sleep10 { SELECT pg_sleep(0.01) }
step locktbl { LOCK TABLE accounts; }
step update { DELETE FROM accounts WHERE accountid = 'checking'; }
teardown { ABORT; }
@@ -47,3 +50,5 @@ permutation wrtbl lto update(*)
permutation wrtbl lsto update(*)
# statement timeout expires first, row-level lock
permutation wrtbl slto update(*)
+# transaction timeout
+permutation tto sleep0 sleep0 sleep10(*)
\ No newline at end of file
--
2.37.0 (Apple Git-136)
^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: Transaction timeout
2022-12-03 05:18 Transaction timeout Andrey Borodin <[email protected]>
2022-12-03 06:59 ` Re: Transaction timeout Nikolay Samokhvalov <[email protected]>
2022-12-03 17:41 ` Re: Transaction timeout Andrey Borodin <[email protected]>
2022-12-07 18:23 ` Re: Transaction timeout Andres Freund <[email protected]>
2022-12-07 21:30 ` Re: Transaction timeout Andrey Borodin <[email protected]>
@ 2022-12-18 20:53 ` Andrey Borodin <[email protected]>
0 siblings, 0 replies; 22+ messages in thread
From: Andrey Borodin @ 2022-12-18 20:53 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Michael Paquier <[email protected]>; Nikolay Samokhvalov <[email protected]>; pgsql-hackers
On Wed, Dec 7, 2022 at 1:30 PM Andrey Borodin <[email protected]> wrote:
> I hope to address other feedback on the weekend.
Andres, here's my progress on working with your review notes.
> > @@ -3277,6 +3282,7 @@ ProcessInterrupts(void)
> > */
> > lock_timeout_occurred = get_timeout_indicator(LOCK_TIMEOUT, true);
> > stmt_timeout_occurred = get_timeout_indicator(STATEMENT_TIMEOUT, true);
> > + tx_timeout_occurred = get_timeout_indicator(TRANSACTION_TIMEOUT, true);
> >
> > /*
> > * If both were set, we want to report whichever timeout completed
>
> This doesn't update the preceding comment, btw, which now reads oddly:
I've rewritten this part to correctly report all timeouts that did
happen. However there's now a tricky comma-formatting code which was
tested only manually.
> > > > @@ -1360,6 +1363,16 @@ IdleInTransactionSessionTimeoutHandler(void)
> > > > SetLatch(MyLatch);
> > > > }
> > > >
> > > > +static void
> > > > +TransactionTimeoutHandler(void)
> > > > +{
> > > > +#ifdef HAVE_SETSID
> > > > + /* try to signal whole process group */
> > > > + kill(-MyProcPid, SIGINT);
> > > > +#endif
> > > > + kill(MyProcPid, SIGINT);
> > > > +}
> > > > +
> > >
> > > Why does this use signals instead of just setting the latch like
> > > IdleInTransactionSessionTimeoutHandler() etc?
> >
> > I just copied statement_timeout behaviour. As I understand this
> > implementation is prefered if the timeout can catch the backend
> > running at full steam.
>
> Hm. I'm not particularly convinced by that code. Be that as it may, I
> don't think it's a good idea to have one more copy of this code. At
> least the patch should wrap the signalling code in a helper.
Done, now there is a single CancelOnTimeoutHandler() handler.
> > > > diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c
> > > > index 0081873a72..5229fe3555 100644
> > > > --- a/src/bin/pg_dump/pg_backup_archiver.c
> > > > +++ b/src/bin/pg_dump/pg_backup_archiver.c
> > > > @@ -3089,6 +3089,7 @@ _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");
> > > > + ahprintf(AH, "SET transaction_timeout = 0;\n");
> > >
> > > Hm - why is that the right thing to do?
> > Because transaction_timeout has effects of statement_timeout.
>
> I guess it's just following precedent - but it seems a bit presumptuous
> to just disable safety settings a DBA might have set up. That makes some
> sense for e.g. idle_in_transaction_session_timeout, because I think
> e.g. parallel backup can lead to a connection being idle for a bit.
I do not know. My reasoning - everywhere we turn off
statement_timeout, we should turn off transaction_timeout too.
But I have no strong opinion here. I left this code as is in the patch
so far. For the same reason I did not change anything in
pg_backup_archiver.c.
> > Either way we can do batch function enable_timeouts() instead
> > enable_timeout_after().
>
> > Does anything of it make sense?
>
> I'm at least as worried about the various calls *after* the execution of
> a statement.
I think this code is just a one bit check
if (get_timeout_active(TRANSACTION_TIMEOUT))
inside of get_timeout_active(). With all 14 timeouts we have, I don't
see a good way to optimize stuff so far.
> > + if (tx_timeout_occurred)
> > + {
> > + LockErrorCleanup();
> > + ereport(ERROR,
> > + (errcode(ERRCODE_TRANSACTION_TIMEOUT),
> > + errmsg("canceling transaction due to transaction timeout")));
> > + }
>
> The number of calls to LockErrorCleanup() here feels wrong - there's
> already 8 calls in ProcessInterrupts(). Besides the code duplication I
> also think it's not a sane idea to rely on having LockErrorCleanup()
> before all the relevant ereport(ERROR)s.
I've refactored that code down to 7 calls of LockErrorCleanup() :)
Logic behind various branches is not clear for me, e.g. why we do not
LockErrorCleanup() when reading commands from a client?
So I did not risk refactoring further.
> I think the test should verify
> that transaction timeout interacts correctly with statement timeout /
> idle in tx timeout.
I've added tests that check statement_timeout vs transaction_timeout.
However I could not produce stable tests with
idle_in_transaction_timeout vs transaction_timeout so far. But I'll
look into this more.
Actually, stabilizing statement_timeout vs transaction_timeout was
tricky on Windows too. I had to remove the second call to
pg_sleep(0.0001) because it was triggering 10ьs timeout from time to
time. Also, test timeout was increased to 30ms, because unlike others
in spec it's not supposed to happen at the very first SQL statement.
Thank you!
Best regards, Andrey Borodin.
Attachments:
[application/octet-stream] v4-0001-Intorduce-transaction_timeout.patch (16.6K, ../../CAAhFRxg9i6DrHT1cALBNztsnJ7OR3cawfCmyFA=J6=oDFX+h2Q@mail.gmail.com/2-v4-0001-Intorduce-transaction_timeout.patch)
download | inline diff:
From f75b18441db3a40f143ce094cb7e705ae34640db Mon Sep 17 00:00:00 2001
From: Andrey Borodin <[email protected]>
Date: Fri, 2 Dec 2022 21:01:29 -0800
Subject: [PATCH v4] Intorduce transaction_timeout
Just like statement_timeout, but for transaction.
Author: Andrey Borodin <[email protected]>
Reviewed-by: Andres Freund <[email protected]>
Discussion: https://postgr.es/m/CAAhFRxiQsRs2Eq5kCo9nXE3HTugsAAJdSQSmxncivebAxdmBjQ%40mail.gmail.com
---
doc/src/sgml/config.sgml | 24 +++++++++
src/backend/postmaster/autovacuum.c | 1 +
src/backend/storage/lmgr/proc.c | 1 +
src/backend/tcop/postgres.c | 51 +++++++++++--------
src/backend/utils/errcodes.txt | 1 +
src/backend/utils/init/postinit.c | 9 ++--
src/backend/utils/misc/guc_tables.c | 11 ++++
src/backend/utils/misc/postgresql.conf.sample | 7 +--
src/bin/pg_dump/pg_backup_archiver.c | 1 +
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/expected/timeouts.out | 18 +++++++
src/test/isolation/specs/timeouts.spec | 8 +++
16 files changed, 110 insertions(+), 28 deletions(-)
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 39d1c89e33..fd74db7527 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -9009,6 +9009,30 @@ COPY postgres_log FROM '/full/path/to/logfile.csv' WITH csv;
</listitem>
</varlistentry>
+ <varlistentry id="guc-transaction-timeout" xreflabel="transaction_timeout">
+ <term><varname>transaction_timeout</varname> (<type>integer</type>)
+ <indexterm>
+ <primary><varname>transaction_timeout</varname> configuration parameter</primary>
+ </indexterm>
+ </term>
+ <listitem>
+ <para>
+ Cancel any transaction that spans longer than the specified amount of
+ time. The limit applies both to explicit transactions (started with
+ <command>BEGIN</command>) and to implicitly started transaction
+ corresponding to single statement.
+ If this value is specified without units, it is taken as milliseconds.
+ A value of zero (the default) disables the timeout.
+ </para>
+
+ <para>
+ Setting <varname>transaction_timeout</varname> in
+ <filename>postgresql.conf</filename> is not recommended because it would
+ affect all sessions.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry id="guc-lock-timeout" xreflabel="lock_timeout">
<term><varname>lock_timeout</varname> (<type>integer</type>)
<indexterm>
diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index 601834d4b4..828a28af0a 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -588,6 +588,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);
diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c
index b1c35653fc..0170e226d0 100644
--- a/src/backend/storage/lmgr/proc.c
+++ b/src/backend/storage/lmgr/proc.c
@@ -61,6 +61,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 3082093d1e..1eaf2e054a 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -2691,6 +2691,9 @@ start_xact_command(void)
{
StartTransactionCommand();
+ if (TransactionTimeout > 0 && !get_timeout_active(TRANSACTION_TIMEOUT))
+ enable_timeout_after(TRANSACTION_TIMEOUT, TransactionTimeout);
+
xact_started = true;
}
@@ -3268,40 +3271,43 @@ ProcessInterrupts(void)
{
bool lock_timeout_occurred;
bool stmt_timeout_occurred;
+ bool tx_timeout_occurred;
+ int err_code = ERRCODE_QUERY_CANCELED;
QueryCancelPending = false;
/*
- * If LOCK_TIMEOUT and STATEMENT_TIMEOUT indicators are both set, we
- * need to clear both, so always fetch both.
+ * If LOCK_TIMEOUT, STATEMENT_TIMEOUT and TRANSACTION indicators are set, we
+ * need to clear all of them, so always fetch each one.
*/
lock_timeout_occurred = get_timeout_indicator(LOCK_TIMEOUT, true);
stmt_timeout_occurred = get_timeout_indicator(STATEMENT_TIMEOUT, true);
-
- /*
- * If both were set, we want to report whichever timeout completed
- * earlier; this ensures consistent behavior if the machine is slow
- * enough that the second timeout triggers before we get here. A tie
- * is arbitrarily broken in favor of reporting a lock timeout.
- */
- if (lock_timeout_occurred && stmt_timeout_occurred &&
- get_timeout_finish_time(STATEMENT_TIMEOUT) < get_timeout_finish_time(LOCK_TIMEOUT))
- lock_timeout_occurred = false; /* report stmt timeout */
+ tx_timeout_occurred = get_timeout_indicator(TRANSACTION_TIMEOUT, true);
if (lock_timeout_occurred)
+ err_code = ERRCODE_LOCK_NOT_AVAILABLE;
+
+
+ if (lock_timeout_occurred || stmt_timeout_occurred || tx_timeout_occurred)
{
+ /* Report all reasons for timeout */
+ char* lock_reason = lock_timeout_occurred ?
+ _("lock timeout") : "";
+ char* stmt_reason = stmt_timeout_occurred ?
+ _("statement timeout") : "";
+ char* tx_reason = tx_timeout_occurred ?
+ _("transaction timeout") : "";
+ char* comma1 = lock_timeout_occurred && stmt_timeout_occurred ?
+ "," : "";
+ char* comma2 = (lock_timeout_occurred || stmt_timeout_occurred)
+ && tx_timeout_occurred ? "," : "";
LockErrorCleanup();
ereport(ERROR,
- (errcode(ERRCODE_LOCK_NOT_AVAILABLE),
- errmsg("canceling statement due to lock timeout")));
- }
- if (stmt_timeout_occurred)
- {
- LockErrorCleanup();
- ereport(ERROR,
- (errcode(ERRCODE_QUERY_CANCELED),
- errmsg("canceling statement due to statement timeout")));
+ (errcode(err_code),
+ errmsg("canceling statement due to %s%s%s%s%s", lock_reason, comma1,
+ stmt_reason, comma2, tx_reason)));
}
+
if (IsAutoVacuumWorkerProcess())
{
LockErrorCleanup();
@@ -4460,6 +4466,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 */
diff --git a/src/backend/utils/errcodes.txt b/src/backend/utils/errcodes.txt
index 62418a051a..3ae2bbda70 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/postinit.c b/src/backend/utils/init/postinit.c
index a990c833c5..779cb22915 100644
--- a/src/backend/utils/init/postinit.c
+++ b/src/backend/utils/init/postinit.c
@@ -73,7 +73,7 @@ static void PerformAuthentication(Port *port);
static void CheckMyDatabase(const char *name, bool am_superuser, bool override_allow_connections);
static void ShutdownPostgres(int code, Datum arg);
static void StatementTimeoutHandler(void);
-static void LockTimeoutHandler(void);
+static void CancelOnTimeoutHandler(void);
static void IdleInTransactionSessionTimeoutHandler(void);
static void IdleSessionTimeoutHandler(void);
static void IdleStatsUpdateTimeoutHandler(void);
@@ -753,9 +753,10 @@ InitPostgres(const char *in_dbname, Oid dboid,
{
RegisterTimeout(DEADLOCK_TIMEOUT, CheckDeadLockAlert);
RegisterTimeout(STATEMENT_TIMEOUT, StatementTimeoutHandler);
- RegisterTimeout(LOCK_TIMEOUT, LockTimeoutHandler);
+ RegisterTimeout(LOCK_TIMEOUT, CancelOnTimeoutHandler);
RegisterTimeout(IDLE_IN_TRANSACTION_SESSION_TIMEOUT,
IdleInTransactionSessionTimeoutHandler);
+ RegisterTimeout(TRANSACTION_TIMEOUT, CancelOnTimeoutHandler);
RegisterTimeout(IDLE_SESSION_TIMEOUT, IdleSessionTimeoutHandler);
RegisterTimeout(CLIENT_CONNECTION_CHECK_TIMEOUT, ClientCheckTimeoutHandler);
RegisterTimeout(IDLE_STATS_UPDATE_TIMEOUT,
@@ -1340,10 +1341,10 @@ StatementTimeoutHandler(void)
}
/*
- * LOCK_TIMEOUT handler: trigger a query-cancel interrupt.
+ * LOCK_TIMEOUT and TRANSACTION_TIMEOUT handler: trigger a query-cancel interrupt.
*/
static void
-LockTimeoutHandler(void)
+CancelOnTimeoutHandler(void)
{
#ifdef HAVE_SETSID
/* try to signal whole process group */
diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c
index 1bf14eec66..ca21d2544c 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -2452,6 +2452,17 @@ struct config_int ConfigureNamesInt[] =
NULL, NULL, NULL
},
+ {
+ {"transaction_timeout", PGC_USERSET, CLIENT_CONN_STATEMENT,
+ gettext_noop("Sets the maximum allowed in a 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 043864597f..a8c935f60c 100644
--- a/src/backend/utils/misc/postgresql.conf.sample
+++ b/src/backend/utils/misc/postgresql.conf.sample
@@ -687,10 +687,11 @@
#default_transaction_read_only = off
#default_transaction_deferrable = off
#session_replication_role = 'origin'
-#statement_timeout = 0 # in milliseconds, 0 is disabled
-#lock_timeout = 0 # in milliseconds, 0 is disabled
+#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
+#idle_session_timeout = 0 # in milliseconds, 0 is disabled
#vacuum_freeze_table_age = 150000000
#vacuum_freeze_min_age = 50000000
#vacuum_failsafe_age = 1600000000
diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c
index 0081873a72..5229fe3555 100644
--- a/src/bin/pg_dump/pg_backup_archiver.c
+++ b/src/bin/pg_dump/pg_backup_archiver.c
@@ -3089,6 +3089,7 @@ _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");
+ 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 44e8cd4704..87be74753d 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -1188,6 +1188,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 >= 160000)
+ 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 011c9cce6e..1b9674140a 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 795182fa51..484384b4ed 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 aa13e1d66e..a892c72765 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 c068986d09..c8fa0dc3d9 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/expected/timeouts.out b/src/test/isolation/expected/timeouts.out
index 9328676f1c..6325ab4d00 100644
--- a/src/test/isolation/expected/timeouts.out
+++ b/src/test/isolation/expected/timeouts.out
@@ -79,3 +79,21 @@ step slto: SET lock_timeout = '10s'; SET statement_timeout = '10ms';
step update: DELETE FROM accounts WHERE accountid = 'checking'; <waiting ...>
step update: <... completed>
ERROR: canceling statement due to statement timeout
+
+starting permutation: tsto sleep0 sleep1000
+step tsto: SET transaction_timeout = '30ms'; SET statement_timeout = '10s';
+step sleep0: SELECT pg_sleep(0.0001)
+pg_sleep
+--------
+
+(1 row)
+
+step sleep1000: SELECT pg_sleep(1000) <waiting ...>
+step sleep1000: <... completed>
+ERROR: canceling statement due to transaction timeout
+
+starting permutation: stto sleep1000
+step stto: SET transaction_timeout = '10s'; SET statement_timeout = '10ms';
+step sleep1000: SELECT pg_sleep(1000) <waiting ...>
+step sleep1000: <... completed>
+ERROR: canceling statement due to statement timeout
diff --git a/src/test/isolation/specs/timeouts.spec b/src/test/isolation/specs/timeouts.spec
index c747b4ae28..82d7fc501a 100644
--- a/src/test/isolation/specs/timeouts.spec
+++ b/src/test/isolation/specs/timeouts.spec
@@ -23,6 +23,10 @@ step sto { SET statement_timeout = '10ms'; }
step lto { SET lock_timeout = '10ms'; }
step lsto { SET lock_timeout = '10ms'; SET statement_timeout = '10s'; }
step slto { SET lock_timeout = '10s'; SET statement_timeout = '10ms'; }
+step tsto { SET transaction_timeout = '30ms'; SET statement_timeout = '10s';}
+step stto { SET transaction_timeout = '10s'; SET statement_timeout = '10ms';}
+step sleep0 { SELECT pg_sleep(0.0001) } # expected to always finish in time
+step sleep1000 { SELECT pg_sleep(1000) } # expected to timeout always
step locktbl { LOCK TABLE accounts; }
step update { DELETE FROM accounts WHERE accountid = 'checking'; }
teardown { ABORT; }
@@ -47,3 +51,7 @@ permutation wrtbl lto update(*)
permutation wrtbl lsto update(*)
# statement timeout expires first, row-level lock
permutation wrtbl slto update(*)
+# transaction timeout before statement timeout
+permutation tsto sleep0 sleep1000(*)
+# statement timeout before transaction timeout
+permutation stto sleep1000(*)
\ No newline at end of file
--
2.37.0 (Apple Git-136)
^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: Transaction timeout
2022-12-03 05:18 Transaction timeout Andrey Borodin <[email protected]>
@ 2024-01-22 06:23 ` Peter Smith <[email protected]>
2024-01-26 06:44 ` Re: Transaction timeout Andrey M. Borodin <[email protected]>
1 sibling, 1 reply; 22+ messages in thread
From: Peter Smith @ 2024-01-22 06:23 UTC (permalink / raw)
To: Andrey M. Borodin <[email protected]>; +Cc: Japin Li <[email protected]>; Junwang Zhao <[email protected]>; 邱宇航 <[email protected]>; Fujii Masao <[email protected]>; Andrey Borodin <[email protected]>; Andres Freund <[email protected]>; Michael Paquier <[email protected]>; Nikolay Samokhvalov <[email protected]>; pgsql-hackers; [email protected] <[email protected]>
2024-01 Commitfest.
Hi, This patch has a CF status of "Needs Review" [1], but it seems
there was a CFbot test failure last time it was run [2]. Please have a
look and post an updated version if necessary.
======
[1] https://commitfest.postgresql.org/46/4040/
[2] https://cirrus-ci.com/task/4721191139672064
Kind Regards,
Peter Smith.
^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: Transaction timeout
2022-12-03 05:18 Transaction timeout Andrey Borodin <[email protected]>
2024-01-22 06:23 ` Re: Transaction timeout Peter Smith <[email protected]>
@ 2024-01-26 06:44 ` Andrey M. Borodin <[email protected]>
2024-01-26 06:46 ` Re: Transaction timeout Andrey M. Borodin <[email protected]>
2024-01-26 14:58 ` Re: Transaction timeout Japin Li <[email protected]>
0 siblings, 2 replies; 22+ messages in thread
From: Andrey M. Borodin @ 2024-01-26 06:44 UTC (permalink / raw)
To: Peter Smith <[email protected]>; +Cc: Japin Li <[email protected]>; Junwang Zhao <[email protected]>; 邱宇航 <[email protected]>; Fujii Masao <[email protected]>; Andrey Borodin <[email protected]>; Andres Freund <[email protected]>; Michael Paquier <[email protected]>; Nikolay Samokhvalov <[email protected]>; pgsql-hackers; [email protected] <[email protected]>
> On 22 Jan 2024, at 11:23, Peter Smith <[email protected]> wrote:
>
> Hi, This patch has a CF status of "Needs Review" [1], but it seems
> there was a CFbot test failure last time it was run [2]. Please have a
> look and post an updated version if necessary.
Thanks Peter!
I’ve inspected CI fails and they were caused by two different problems:
1. It’s unsafe for isaoltion tester to await transaction_timeout within a query. Usually it gets
FATAL: terminating connection due to transaction timeout
But if VM is a bit slow it can get occasional
PQconsumeInput failed: server closed the connection unexpectedly
So, currently all tests use “passive waiting”, in a session that will not timeout.
2. In some cases pg_sleep(0.1) were sleeping up to 200 ms. That was making s7 and s8 fail, because they rely on this margin.
I’ve separated these tests into different test timeouts-long and increased margin to 300ms. Now tests run horrible 2431 ms. Moreover I’m afraid that on buildfarm we can have much randomly-slower machines so this test might be excluded.
This test checks COMMIT AND CHAIN and flow of small queries (Nik’s case).
Also I’ve verified that every "enable_timeout_after(TRANSACTION_TIMEOUT)” and “disable_timeout(TRANSACTION_TIMEOUT)” is necessary and found that case of aborting "idle in transaction (aborted)” is not covered by tests. I’m not sure we need a test for this.
Japin, Junwang, what do you think?
Thanks!
Best regards, Andrey Borodin.
Attachments:
[application/octet-stream] v23-0001-Introduce-transaction_timeout.patch (26.3K, ../../[email protected]/2-v23-0001-Introduce-transaction_timeout.patch)
download | inline diff:
From 7b0a01b9fd47130c033e74188601ff7d78781084 Mon Sep 17 00:00:00 2001
From: "Andrey M. Borodin" <[email protected]>
Date: Sun, 3 Dec 2023 23:18:00 +0500
Subject: [PATCH v23] 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 <[email protected]>
Author: Japin Li <[email protected]>
Author: Junwang Zhao <[email protected]>
Reviewed-by: Nikolay Samokhvalov <[email protected]>
Reviewed-by: Andres Freund <[email protected]>
Reviewed-by: Fujii Masao <[email protected]>
Reviewed-by: bt23nguyent <[email protected]>
Reviewed-by: Yuhang Qiu <[email protected]>
Discussion: https://postgr.es/m/CAAhFRxiQsRs2Eq5kCo9nXE3HTugsAAJdSQSmxncivebAxdmBjQ%40mail.gmail.com
---
doc/src/sgml/config.sgml | 35 ++++++++
src/backend/access/transam/xact.c | 4 +
src/backend/postmaster/autovacuum.c | 2 +
src/backend/storage/lmgr/proc.c | 1 +
src/backend/tcop/postgres.c | 33 +++++++-
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 | 3 +
src/test/isolation/expected/timeouts-long.out | 69 ++++++++++++++++
src/test/isolation/expected/timeouts.out | 79 ++++++++++++++++++-
src/test/isolation/isolation_schedule | 1 +
src/test/isolation/specs/timeouts-long.spec | 35 ++++++++
src/test/isolation/specs/timeouts.spec | 40 +++++++++-
22 files changed, 329 insertions(+), 5 deletions(-)
create mode 100644 src/test/isolation/expected/timeouts-long.out
create mode 100644 src/test/isolation/specs/timeouts-long.spec
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 61038472c5a..bd099d06350 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -9135,6 +9135,41 @@ COPY postgres_log FROM '/full/path/to/logfile.csv' WITH csv;
</listitem>
</varlistentry>
+ <varlistentry id="guc-transaction-timeout" xreflabel="transaction_timeout">
+ <term><varname>transaction_timeout</varname> (<type>integer</type>)
+ <indexterm>
+ <primary><varname>transaction_timeout</varname> configuration parameter</primary>
+ </indexterm>
+ </term>
+ <listitem>
+ <para>
+ Terminate any session that spans longer than the specified amount of
+ time in transaction. The limit applies both to explicit transactions
+ (started with <command>BEGIN</command>) 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.
+ </para>
+
+ <para>
+ If <varname>transaction_timeout</varname> is shorter than
+ <varname>idle_in_transaction_session_timeout</varname> or <varname>statement_timeout</varname>
+ <varname>transaction_timeout</varname> will invalidate longer timeout.
+ </para>
+
+ <para>
+ Setting <varname>transaction_timeout</varname> in
+ <filename>postgresql.conf</filename> is not recommended because it would
+ affect all sessions.
+ </para>
+
+ <para>
+ Prepared transactions are not subject for this timeout.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry id="guc-lock-timeout" xreflabel="lock_timeout">
<term><varname>lock_timeout</varname> (<type>integer</type>)
<indexterm>
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index 464858117e0..a124ba59330 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -2139,6 +2139,10 @@ StartTransaction(void)
*/
s->state = TRANS_INPROGRESS;
+ /* Schedule transaction timeout */
+ if (TransactionTimeout > 0)
+ enable_timeout_after(TRANSACTION_TIMEOUT, TransactionTimeout);
+
ShowTransactionState("StartTransaction");
}
diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index 2c3099f76f1..c12fc6594ce 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 4ad96beb87a..b8234ef8e4b 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 1a34bd3715f..af28f425ce6 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -3426,6 +3426,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,12 +4502,18 @@ 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,
IdleInTransactionSessionTimeout);
}
+
+ /* Schedule or reschedule transaction timeout */
+ if (TransactionTimeout > 0 && !get_timeout_active(TRANSACTION_TIMEOUT))
+ enable_timeout_after(TRANSACTION_TIMEOUT,
+ TransactionTimeout);
}
else if (IsTransactionOrTransactionBlock())
{
@@ -4504,12 +4521,18 @@ 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,
IdleInTransactionSessionTimeout);
}
+
+ /* Schedule or reschedule transaction timeout */
+ if (TransactionTimeout > 0 && !get_timeout_active(TRANSACTION_TIMEOUT))
+ enable_timeout_after(TRANSACTION_TIMEOUT,
+ TransactionTimeout);
}
else
{
@@ -4562,6 +4585,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 +5146,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 29f367a5e1c..3250d539e1c 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 88b03e8fa3c..f024b1a8497 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 1ad33671598..7797876d008 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 7fe58518d7d..0fb5ec648e4 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -2577,6 +2577,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 da10b43dac3..3b8992f0fbf 100644
--- a/src/backend/utils/misc/postgresql.conf.sample
+++ b/src/backend/utils/misc/postgresql.conf.sample
@@ -701,6 +701,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 256d1e35a4e..3342971bd01 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 bc20a025ce4..c2076f979a3 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -1252,6 +1252,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 11347ab1824..7d898c3b501 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 0b01c1f0935..0445fbf61d7 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 4bc226e36cd..20d6fa652dc 100644
--- a/src/include/storage/proc.h
+++ b/src/include/storage/proc.h
@@ -429,6 +429,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 20e7cf72d0d..a5d8f078246 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 ade2256ed3a..91307e1a7e8 100644
--- a/src/test/isolation/Makefile
+++ b/src/test/isolation/Makefile
@@ -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 timeouts-long
diff --git a/src/test/isolation/expected/timeouts-long.out b/src/test/isolation/expected/timeouts-long.out
new file mode 100644
index 00000000000..26a6672c051
--- /dev/null
+++ b/src/test/isolation/expected/timeouts-long.out
@@ -0,0 +1,69 @@
+Parsed test spec with 3 sessions
+
+starting permutation: s7_begin s7_sleep s7_commit_and_chain s7_sleep s7_check s7_abort
+step s7_begin:
+ BEGIN ISOLATION LEVEL READ COMMITTED;
+ SET transaction_timeout = '1s';
+
+step s7_sleep: SELECT pg_sleep(0.6);
+pg_sleep
+--------
+
+(1 row)
+
+step s7_commit_and_chain: COMMIT AND CHAIN;
+step s7_sleep: SELECT pg_sleep(0.6);
+pg_sleep
+--------
+
+(1 row)
+
+step s7_check: SELECT count(*) FROM pg_stat_activity WHERE application_name = 'isolation/timeouts/s7';
+count
+-----
+ 0
+(1 row)
+
+step s7_abort: ABORT;
+
+starting permutation: s8_begin s8_sleep s8_select_1 s8_check checker_sleep checker_sleep s8_check
+step s8_begin:
+ BEGIN ISOLATION LEVEL READ COMMITTED;
+ SET transaction_timeout = '900ms';
+
+step s8_sleep: SELECT pg_sleep(0.6);
+pg_sleep
+--------
+
+(1 row)
+
+step s8_select_1: SELECT 1;
+?column?
+--------
+ 1
+(1 row)
+
+step s8_check: SELECT count(*) FROM pg_stat_activity WHERE application_name = 'isolation/timeouts/s8';
+count
+-----
+ 0
+(1 row)
+
+step checker_sleep: SELECT pg_sleep(0.3);
+pg_sleep
+--------
+
+(1 row)
+
+step checker_sleep: SELECT pg_sleep(0.3);
+pg_sleep
+--------
+
+(1 row)
+
+step s8_check: SELECT count(*) FROM pg_stat_activity WHERE application_name = 'isolation/timeouts/s8';
+count
+-----
+ 0
+(1 row)
+
diff --git a/src/test/isolation/expected/timeouts.out b/src/test/isolation/expected/timeouts.out
index 9328676f1cc..197c25a9a94 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 7 sessions
starting permutation: rdtbl sto locktbl
step rdtbl: SELECT * FROM accounts;
@@ -79,3 +79,80 @@ step slto: SET lock_timeout = '10s'; SET statement_timeout = '10ms';
step update: DELETE FROM accounts WHERE accountid = 'checking'; <waiting ...>
step update: <... completed>
ERROR: canceling statement due to statement timeout
+
+starting permutation: stto s3_begin s3_sleep s3_check s3_abort
+step stto: SET statement_timeout = '10ms'; SET transaction_timeout = '1s';
+step s3_begin: BEGIN ISOLATION LEVEL READ COMMITTED;
+step s3_sleep: SELECT pg_sleep(0.1);
+ERROR: canceling statement due to statement timeout
+step s3_check: SELECT count(*) FROM pg_stat_activity WHERE application_name = 'isolation/timeouts/s3';
+count
+-----
+ 1
+(1 row)
+
+step s3_abort: ABORT;
+
+starting permutation: tsto s3_begin s3_sleep s3_check
+step tsto: SET statement_timeout = '1s'; SET transaction_timeout = '10ms';
+step s3_begin: BEGIN ISOLATION LEVEL READ COMMITTED;
+step s3_sleep: SELECT pg_sleep(0.1);
+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 s3_check: SELECT count(*) FROM pg_stat_activity WHERE application_name = 'isolation/timeouts/s3';
+count
+-----
+ 0
+(1 row)
+
+
+starting permutation: itto s4_begin checker_sleep s4_check
+step itto: SET idle_in_transaction_session_timeout = '10ms'; SET transaction_timeout = '1s';
+step s4_begin: BEGIN ISOLATION LEVEL READ COMMITTED;
+step checker_sleep: SELECT pg_sleep(0.1);
+pg_sleep
+--------
+
+(1 row)
+
+step s4_check: SELECT count(*) FROM pg_stat_activity WHERE application_name = 'isolation/timeouts/s4';
+count
+-----
+ 0
+(1 row)
+
+
+starting permutation: tito s5_begin checker_sleep s5_check
+step tito: SET idle_in_transaction_session_timeout = '1s'; SET transaction_timeout = '10ms';
+step s5_begin: BEGIN ISOLATION LEVEL READ COMMITTED;
+step checker_sleep: SELECT pg_sleep(0.1);
+pg_sleep
+--------
+
+(1 row)
+
+step s5_check: SELECT count(*) FROM pg_stat_activity WHERE application_name = 'isolation/timeouts/s5';
+count
+-----
+ 0
+(1 row)
+
+
+starting permutation: s6_begin s6_tt checker_sleep s6_check
+step s6_begin: BEGIN ISOLATION LEVEL READ COMMITTED;
+step s6_tt: SET statement_timeout = '1s'; SET transaction_timeout = '10ms';
+step checker_sleep: SELECT pg_sleep(0.1);
+pg_sleep
+--------
+
+(1 row)
+
+step s6_check: SELECT count(*) FROM pg_stat_activity WHERE application_name = 'isolation/timeouts/s6';
+count
+-----
+ 0
+(1 row)
+
diff --git a/src/test/isolation/isolation_schedule b/src/test/isolation/isolation_schedule
index b2be88ead1d..86ef62bbcf6 100644
--- a/src/test/isolation/isolation_schedule
+++ b/src/test/isolation/isolation_schedule
@@ -89,6 +89,7 @@ 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
new file mode 100644
index 00000000000..ce2c9a43011
--- /dev/null
+++ b/src/test/isolation/specs/timeouts-long.spec
@@ -0,0 +1,35 @@
+# 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
diff --git a/src/test/isolation/specs/timeouts.spec b/src/test/isolation/specs/timeouts.spec
index c747b4ae28d..8560f01a6b3 100644
--- a/src/test/isolation/specs/timeouts.spec
+++ b/src/test/isolation/specs/timeouts.spec
@@ -1,4 +1,4 @@
-# Simple tests for statement_timeout and lock_timeout features
+# Simple tests for statement_timeout, lock_timeout and transaction_timeout features
setup
{
@@ -27,6 +27,33 @@ step locktbl { LOCK TABLE accounts; }
step update { DELETE FROM accounts WHERE accountid = 'checking'; }
teardown { ABORT; }
+session s3
+step s3_begin { BEGIN ISOLATION LEVEL READ COMMITTED; }
+step stto { SET statement_timeout = '10ms'; SET transaction_timeout = '1s'; }
+step tsto { SET statement_timeout = '1s'; SET transaction_timeout = '10ms'; }
+step s3_sleep { SELECT pg_sleep(0.1); }
+step s3_abort { ABORT; }
+
+session s4
+step s4_begin { BEGIN ISOLATION LEVEL READ COMMITTED; }
+step itto { SET idle_in_transaction_session_timeout = '10ms'; SET transaction_timeout = '1s'; }
+
+session s5
+step s5_begin { BEGIN ISOLATION LEVEL READ COMMITTED; }
+step tito { SET idle_in_transaction_session_timeout = '1s'; SET transaction_timeout = '10ms'; }
+
+session s6
+step s6_begin { BEGIN ISOLATION LEVEL READ COMMITTED; }
+step s6_tt { SET statement_timeout = '1s'; SET transaction_timeout = '10ms'; }
+
+session checker
+step checker_sleep { SELECT pg_sleep(0.1); }
+step s3_check { SELECT count(*) FROM pg_stat_activity WHERE application_name = 'isolation/timeouts/s3'; }
+step s4_check { SELECT count(*) FROM pg_stat_activity WHERE application_name = 'isolation/timeouts/s4'; }
+step s5_check { SELECT count(*) FROM pg_stat_activity WHERE application_name = 'isolation/timeouts/s5'; }
+step s6_check { SELECT count(*) FROM pg_stat_activity WHERE application_name = 'isolation/timeouts/s6'; }
+
+
# 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 +74,14 @@ permutation wrtbl lto update(*)
permutation wrtbl lsto update(*)
# statement timeout expires first, row-level lock
permutation wrtbl slto update(*)
+
+# statement timeout expires first
+permutation stto s3_begin s3_sleep s3_check s3_abort
+# transaction timeout expires first, session s3 FATAL-out
+permutation tsto s3_begin s3_sleep s3_check
+# idle in transaction timeout expires first, session s4 FATAL-out
+permutation itto s4_begin checker_sleep s4_check
+# transaction timeout expires first, session s5 FATAL-out
+permutation tito s5_begin checker_sleep s5_check
+# transaction timeout can be schedule amid transaction, session s6 FATAL-out
+permutation s6_begin s6_tt checker_sleep s6_check
\ No newline at end of file
--
2.42.0
^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: Transaction timeout
2022-12-03 05:18 Transaction timeout Andrey Borodin <[email protected]>
2024-01-22 06:23 ` Re: Transaction timeout Peter Smith <[email protected]>
2024-01-26 06:44 ` Re: Transaction timeout Andrey M. Borodin <[email protected]>
@ 2024-01-26 06:46 ` Andrey M. Borodin <[email protected]>
1 sibling, 0 replies; 22+ messages in thread
From: Andrey M. Borodin @ 2024-01-26 06:46 UTC (permalink / raw)
To: Peter Smith <[email protected]>; +Cc: Japin Li <[email protected]>; Junwang Zhao <[email protected]>; 邱宇航 <[email protected]>; Fujii Masao <[email protected]>; Andrey Borodin <[email protected]>; Andres Freund <[email protected]>; Michael Paquier <[email protected]>; Nikolay Samokhvalov <[email protected]>; pgsql-hackers; [email protected] <[email protected]>
> On 26 Jan 2024, at 11:44, Andrey M. Borodin <[email protected]> wrote:
>
>
> 1. It’s unsafe for isaoltion tester to await transaction_timeout within a query. Usually it gets
> FATAL: terminating connection due to transaction timeout
> But if VM is a bit slow it can get occasional
> PQconsumeInput failed: server closed the connection unexpectedly
> So, currently all tests use “passive waiting”, in a session that will not timeout.
Oops, sorry, I’ve accidentally sent version without this fix.
Here it is.
Best regards, Andrey Borodin.
Attachments:
[application/octet-stream] v24-0001-Introduce-transaction_timeout.patch (26.2K, ../../[email protected]/2-v24-0001-Introduce-transaction_timeout.patch)
download | inline diff:
From ca3b48b20c987b0a557c4b6efa0297a539c45cb5 Mon Sep 17 00:00:00 2001
From: "Andrey M. Borodin" <[email protected]>
Date: Sun, 3 Dec 2023 23:18:00 +0500
Subject: [PATCH v24] 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 <[email protected]>
Author: Japin Li <[email protected]>
Author: Junwang Zhao <[email protected]>
Reviewed-by: Nikolay Samokhvalov <[email protected]>
Reviewed-by: Andres Freund <[email protected]>
Reviewed-by: Fujii Masao <[email protected]>
Reviewed-by: bt23nguyent <[email protected]>
Reviewed-by: Yuhang Qiu <[email protected]>
Discussion: https://postgr.es/m/CAAhFRxiQsRs2Eq5kCo9nXE3HTugsAAJdSQSmxncivebAxdmBjQ%40mail.gmail.com
---
doc/src/sgml/config.sgml | 35 ++++++++
src/backend/access/transam/xact.c | 4 +
src/backend/postmaster/autovacuum.c | 2 +
src/backend/storage/lmgr/proc.c | 1 +
src/backend/tcop/postgres.c | 33 +++++++-
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 | 3 +
src/test/isolation/expected/timeouts-long.out | 69 ++++++++++++++++
src/test/isolation/expected/timeouts.out | 79 ++++++++++++++++++-
src/test/isolation/isolation_schedule | 1 +
src/test/isolation/specs/timeouts-long.spec | 35 ++++++++
src/test/isolation/specs/timeouts.spec | 40 +++++++++-
22 files changed, 329 insertions(+), 5 deletions(-)
create mode 100644 src/test/isolation/expected/timeouts-long.out
create mode 100644 src/test/isolation/specs/timeouts-long.spec
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 61038472c5a..bd099d06350 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -9135,6 +9135,41 @@ COPY postgres_log FROM '/full/path/to/logfile.csv' WITH csv;
</listitem>
</varlistentry>
+ <varlistentry id="guc-transaction-timeout" xreflabel="transaction_timeout">
+ <term><varname>transaction_timeout</varname> (<type>integer</type>)
+ <indexterm>
+ <primary><varname>transaction_timeout</varname> configuration parameter</primary>
+ </indexterm>
+ </term>
+ <listitem>
+ <para>
+ Terminate any session that spans longer than the specified amount of
+ time in transaction. The limit applies both to explicit transactions
+ (started with <command>BEGIN</command>) 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.
+ </para>
+
+ <para>
+ If <varname>transaction_timeout</varname> is shorter than
+ <varname>idle_in_transaction_session_timeout</varname> or <varname>statement_timeout</varname>
+ <varname>transaction_timeout</varname> will invalidate longer timeout.
+ </para>
+
+ <para>
+ Setting <varname>transaction_timeout</varname> in
+ <filename>postgresql.conf</filename> is not recommended because it would
+ affect all sessions.
+ </para>
+
+ <para>
+ Prepared transactions are not subject for this timeout.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry id="guc-lock-timeout" xreflabel="lock_timeout">
<term><varname>lock_timeout</varname> (<type>integer</type>)
<indexterm>
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index 464858117e0..a124ba59330 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -2139,6 +2139,10 @@ StartTransaction(void)
*/
s->state = TRANS_INPROGRESS;
+ /* Schedule transaction timeout */
+ if (TransactionTimeout > 0)
+ enable_timeout_after(TRANSACTION_TIMEOUT, TransactionTimeout);
+
ShowTransactionState("StartTransaction");
}
diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index 2c3099f76f1..c12fc6594ce 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 4ad96beb87a..b8234ef8e4b 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 1a34bd3715f..af28f425ce6 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -3426,6 +3426,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,12 +4502,18 @@ 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,
IdleInTransactionSessionTimeout);
}
+
+ /* Schedule or reschedule transaction timeout */
+ if (TransactionTimeout > 0 && !get_timeout_active(TRANSACTION_TIMEOUT))
+ enable_timeout_after(TRANSACTION_TIMEOUT,
+ TransactionTimeout);
}
else if (IsTransactionOrTransactionBlock())
{
@@ -4504,12 +4521,18 @@ 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,
IdleInTransactionSessionTimeout);
}
+
+ /* Schedule or reschedule transaction timeout */
+ if (TransactionTimeout > 0 && !get_timeout_active(TRANSACTION_TIMEOUT))
+ enable_timeout_after(TRANSACTION_TIMEOUT,
+ TransactionTimeout);
}
else
{
@@ -4562,6 +4585,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 +5146,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 29f367a5e1c..3250d539e1c 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 88b03e8fa3c..f024b1a8497 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 1ad33671598..7797876d008 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 7fe58518d7d..0fb5ec648e4 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -2577,6 +2577,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 da10b43dac3..3b8992f0fbf 100644
--- a/src/backend/utils/misc/postgresql.conf.sample
+++ b/src/backend/utils/misc/postgresql.conf.sample
@@ -701,6 +701,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 256d1e35a4e..3342971bd01 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 bc20a025ce4..c2076f979a3 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -1252,6 +1252,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 11347ab1824..7d898c3b501 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 0b01c1f0935..0445fbf61d7 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 4bc226e36cd..20d6fa652dc 100644
--- a/src/include/storage/proc.h
+++ b/src/include/storage/proc.h
@@ -429,6 +429,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 20e7cf72d0d..a5d8f078246 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 ade2256ed3a..91307e1a7e8 100644
--- a/src/test/isolation/Makefile
+++ b/src/test/isolation/Makefile
@@ -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 timeouts-long
diff --git a/src/test/isolation/expected/timeouts-long.out b/src/test/isolation/expected/timeouts-long.out
new file mode 100644
index 00000000000..26a6672c051
--- /dev/null
+++ b/src/test/isolation/expected/timeouts-long.out
@@ -0,0 +1,69 @@
+Parsed test spec with 3 sessions
+
+starting permutation: s7_begin s7_sleep s7_commit_and_chain s7_sleep s7_check s7_abort
+step s7_begin:
+ BEGIN ISOLATION LEVEL READ COMMITTED;
+ SET transaction_timeout = '1s';
+
+step s7_sleep: SELECT pg_sleep(0.6);
+pg_sleep
+--------
+
+(1 row)
+
+step s7_commit_and_chain: COMMIT AND CHAIN;
+step s7_sleep: SELECT pg_sleep(0.6);
+pg_sleep
+--------
+
+(1 row)
+
+step s7_check: SELECT count(*) FROM pg_stat_activity WHERE application_name = 'isolation/timeouts/s7';
+count
+-----
+ 0
+(1 row)
+
+step s7_abort: ABORT;
+
+starting permutation: s8_begin s8_sleep s8_select_1 s8_check checker_sleep checker_sleep s8_check
+step s8_begin:
+ BEGIN ISOLATION LEVEL READ COMMITTED;
+ SET transaction_timeout = '900ms';
+
+step s8_sleep: SELECT pg_sleep(0.6);
+pg_sleep
+--------
+
+(1 row)
+
+step s8_select_1: SELECT 1;
+?column?
+--------
+ 1
+(1 row)
+
+step s8_check: SELECT count(*) FROM pg_stat_activity WHERE application_name = 'isolation/timeouts/s8';
+count
+-----
+ 0
+(1 row)
+
+step checker_sleep: SELECT pg_sleep(0.3);
+pg_sleep
+--------
+
+(1 row)
+
+step checker_sleep: SELECT pg_sleep(0.3);
+pg_sleep
+--------
+
+(1 row)
+
+step s8_check: SELECT count(*) FROM pg_stat_activity WHERE application_name = 'isolation/timeouts/s8';
+count
+-----
+ 0
+(1 row)
+
diff --git a/src/test/isolation/expected/timeouts.out b/src/test/isolation/expected/timeouts.out
index 9328676f1cc..81a0016375b 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 7 sessions
starting permutation: rdtbl sto locktbl
step rdtbl: SELECT * FROM accounts;
@@ -79,3 +79,80 @@ step slto: SET lock_timeout = '10s'; SET statement_timeout = '10ms';
step update: DELETE FROM accounts WHERE accountid = 'checking'; <waiting ...>
step update: <... completed>
ERROR: canceling statement due to statement timeout
+
+starting permutation: stto s3_begin s3_sleep s3_check s3_abort
+step stto: SET statement_timeout = '10ms'; SET transaction_timeout = '1s';
+step s3_begin: BEGIN ISOLATION LEVEL READ COMMITTED;
+step s3_sleep: SELECT pg_sleep(0.1);
+ERROR: canceling statement due to statement timeout
+step s3_check: SELECT count(*) FROM pg_stat_activity WHERE application_name = 'isolation/timeouts/s3';
+count
+-----
+ 1
+(1 row)
+
+step s3_abort: ABORT;
+
+starting permutation: tsto s3_begin checker_sleep s3_check
+step tsto: SET statement_timeout = '1s'; SET transaction_timeout = '10ms';
+step s3_begin: BEGIN ISOLATION LEVEL READ COMMITTED;
+step checker_sleep: SELECT pg_sleep(0.1);
+pg_sleep
+--------
+
+(1 row)
+
+step s3_check: SELECT count(*) FROM pg_stat_activity WHERE application_name = 'isolation/timeouts/s3';
+count
+-----
+ 0
+(1 row)
+
+
+starting permutation: itto s4_begin checker_sleep s4_check
+step itto: SET idle_in_transaction_session_timeout = '10ms'; SET transaction_timeout = '1s';
+step s4_begin: BEGIN ISOLATION LEVEL READ COMMITTED;
+step checker_sleep: SELECT pg_sleep(0.1);
+pg_sleep
+--------
+
+(1 row)
+
+step s4_check: SELECT count(*) FROM pg_stat_activity WHERE application_name = 'isolation/timeouts/s4';
+count
+-----
+ 0
+(1 row)
+
+
+starting permutation: tito s5_begin checker_sleep s5_check
+step tito: SET idle_in_transaction_session_timeout = '1s'; SET transaction_timeout = '10ms';
+step s5_begin: BEGIN ISOLATION LEVEL READ COMMITTED;
+step checker_sleep: SELECT pg_sleep(0.1);
+pg_sleep
+--------
+
+(1 row)
+
+step s5_check: SELECT count(*) FROM pg_stat_activity WHERE application_name = 'isolation/timeouts/s5';
+count
+-----
+ 0
+(1 row)
+
+
+starting permutation: s6_begin s6_tt checker_sleep s6_check
+step s6_begin: BEGIN ISOLATION LEVEL READ COMMITTED;
+step s6_tt: SET statement_timeout = '1s'; SET transaction_timeout = '10ms';
+step checker_sleep: SELECT pg_sleep(0.1);
+pg_sleep
+--------
+
+(1 row)
+
+step s6_check: SELECT count(*) FROM pg_stat_activity WHERE application_name = 'isolation/timeouts/s6';
+count
+-----
+ 0
+(1 row)
+
diff --git a/src/test/isolation/isolation_schedule b/src/test/isolation/isolation_schedule
index b2be88ead1d..86ef62bbcf6 100644
--- a/src/test/isolation/isolation_schedule
+++ b/src/test/isolation/isolation_schedule
@@ -89,6 +89,7 @@ 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
new file mode 100644
index 00000000000..ce2c9a43011
--- /dev/null
+++ b/src/test/isolation/specs/timeouts-long.spec
@@ -0,0 +1,35 @@
+# 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
diff --git a/src/test/isolation/specs/timeouts.spec b/src/test/isolation/specs/timeouts.spec
index c747b4ae28d..c2cc5d8d37b 100644
--- a/src/test/isolation/specs/timeouts.spec
+++ b/src/test/isolation/specs/timeouts.spec
@@ -1,4 +1,4 @@
-# Simple tests for statement_timeout and lock_timeout features
+# Simple tests for statement_timeout, lock_timeout and transaction_timeout features
setup
{
@@ -27,6 +27,33 @@ step locktbl { LOCK TABLE accounts; }
step update { DELETE FROM accounts WHERE accountid = 'checking'; }
teardown { ABORT; }
+session s3
+step s3_begin { BEGIN ISOLATION LEVEL READ COMMITTED; }
+step stto { SET statement_timeout = '10ms'; SET transaction_timeout = '1s'; }
+step tsto { SET statement_timeout = '1s'; SET transaction_timeout = '10ms'; }
+step s3_sleep { SELECT pg_sleep(0.1); }
+step s3_abort { ABORT; }
+
+session s4
+step s4_begin { BEGIN ISOLATION LEVEL READ COMMITTED; }
+step itto { SET idle_in_transaction_session_timeout = '10ms'; SET transaction_timeout = '1s'; }
+
+session s5
+step s5_begin { BEGIN ISOLATION LEVEL READ COMMITTED; }
+step tito { SET idle_in_transaction_session_timeout = '1s'; SET transaction_timeout = '10ms'; }
+
+session s6
+step s6_begin { BEGIN ISOLATION LEVEL READ COMMITTED; }
+step s6_tt { SET statement_timeout = '1s'; SET transaction_timeout = '10ms'; }
+
+session checker
+step checker_sleep { SELECT pg_sleep(0.1); }
+step s3_check { SELECT count(*) FROM pg_stat_activity WHERE application_name = 'isolation/timeouts/s3'; }
+step s4_check { SELECT count(*) FROM pg_stat_activity WHERE application_name = 'isolation/timeouts/s4'; }
+step s5_check { SELECT count(*) FROM pg_stat_activity WHERE application_name = 'isolation/timeouts/s5'; }
+step s6_check { SELECT count(*) FROM pg_stat_activity WHERE application_name = 'isolation/timeouts/s6'; }
+
+
# 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 +74,14 @@ permutation wrtbl lto update(*)
permutation wrtbl lsto update(*)
# statement timeout expires first, row-level lock
permutation wrtbl slto update(*)
+
+# statement timeout expires first
+permutation stto s3_begin s3_sleep s3_check s3_abort
+# transaction timeout expires first, session s3 FATAL-out
+permutation tsto s3_begin checker_sleep s3_check
+# idle in transaction timeout expires first, session s4 FATAL-out
+permutation itto s4_begin checker_sleep s4_check
+# transaction timeout expires first, session s5 FATAL-out
+permutation tito s5_begin checker_sleep s5_check
+# transaction timeout can be schedule amid transaction, session s6 FATAL-out
+permutation s6_begin s6_tt checker_sleep s6_check
\ No newline at end of file
--
2.42.0
^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: Transaction timeout
2022-12-03 05:18 Transaction timeout Andrey Borodin <[email protected]>
2024-01-22 06:23 ` Re: Transaction timeout Peter Smith <[email protected]>
2024-01-26 06:44 ` Re: Transaction timeout Andrey M. Borodin <[email protected]>
@ 2024-01-26 14:58 ` Japin Li <[email protected]>
2024-01-30 06:22 ` Re: Transaction timeout Andrey M. Borodin <[email protected]>
1 sibling, 1 reply; 22+ messages in thread
From: Japin Li @ 2024-01-26 14:58 UTC (permalink / raw)
To: Andrey M. Borodin <[email protected]>; +Cc: Peter Smith <[email protected]>; Junwang Zhao <[email protected]>; 邱宇航 <[email protected]>; Fujii Masao <[email protected]>; Andrey Borodin <[email protected]>; Andres Freund <[email protected]>; Michael Paquier <[email protected]>; Nikolay Samokhvalov <[email protected]>; pgsql-hackers; [email protected] <[email protected]>
On Fri, 26 Jan 2024 at 14:44, Andrey M. Borodin <[email protected]> wrote:
>> On 22 Jan 2024, at 11:23, Peter Smith <[email protected]> wrote:
>>
>> Hi, This patch has a CF status of "Needs Review" [1], but it seems
>> there was a CFbot test failure last time it was run [2]. Please have a
>> look and post an updated version if necessary.
> Thanks Peter!
>
Thanks for updating the patch. Here are some comments for v24.
+ <para>
+ Terminate any session that spans longer than the specified amount of
+ time in transaction. The limit applies both to explicit transactions
+ (started with <command>BEGIN</command>) 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.
+ </para>
The sentence "But this limit is not applied to prepared transactions" is redundant,
since we have a paragraph to describe this later.
+
+ <para>
+ If <varname>transaction_timeout</varname> is shorter than
+ <varname>idle_in_transaction_session_timeout</varname> or <varname>statement_timeout</varname>
+ <varname>transaction_timeout</varname> will invalidate longer timeout.
+ </para>
+
Since we are already try to disable the timeouts, should we try to disable
them even if they are equal.
+
+ <para>
+ Prepared transactions are not subject for this timeout.
+ </para>
Maybe wrap this with <note> is a good idea.
> I’ve inspected CI fails and they were caused by two different problems:
> 1. It’s unsafe for isaoltion tester to await transaction_timeout within a query. Usually it gets
> FATAL: terminating connection due to transaction timeout
> But if VM is a bit slow it can get occasional
> PQconsumeInput failed: server closed the connection unexpectedly
> So, currently all tests use “passive waiting”, in a session that will not timeout.
>
> 2. In some cases pg_sleep(0.1) were sleeping up to 200 ms. That was making s7 and s8 fail, because they rely on this margin.
I'm curious why this happened.
> I’ve separated these tests into different test timeouts-long and increased margin to 300ms. Now tests run horrible 2431 ms. Moreover I’m afraid that on buildfarm we can have much randomly-slower machines so this test might be excluded.
> This test checks COMMIT AND CHAIN and flow of small queries (Nik’s case).
>
> Also I’ve verified that every "enable_timeout_after(TRANSACTION_TIMEOUT)” and “disable_timeout(TRANSACTION_TIMEOUT)” is necessary and found that case of aborting "idle in transaction (aborted)” is not covered by tests. I’m not sure we need a test for this.
I see there is a test about idle_in_transaction_timeout and transaction_timeout.
Both of them only check the session, but don't check the reason, so we cannot
distinguish the reason they are terminated. Right?
> Japin, Junwang, what do you think?
However, checking the reason on the timeout session may cause regression test
failed (as you point in 1), I don't strongly insist on it.
--
Best regards,
Japin Li.
^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: Transaction timeout
2022-12-03 05:18 Transaction timeout Andrey Borodin <[email protected]>
2024-01-22 06:23 ` Re: Transaction timeout Peter Smith <[email protected]>
2024-01-26 06:44 ` Re: Transaction timeout Andrey M. Borodin <[email protected]>
2024-01-26 14:58 ` Re: Transaction timeout Japin Li <[email protected]>
@ 2024-01-30 06:22 ` Andrey M. Borodin <[email protected]>
2024-01-31 09:27 ` Re: Transaction timeout Japin Li <[email protected]>
0 siblings, 1 reply; 22+ messages in thread
From: Andrey M. Borodin @ 2024-01-30 06:22 UTC (permalink / raw)
To: Japin Li <[email protected]>; +Cc: Peter Smith <[email protected]>; Junwang Zhao <[email protected]>; 邱宇航 <[email protected]>; Fujii Masao <[email protected]>; Andrey Borodin <[email protected]>; Andres Freund <[email protected]>; Michael Paquier <[email protected]>; Nikolay Samokhvalov <[email protected]>; pgsql-hackers; [email protected] <[email protected]>
> On 26 Jan 2024, at 19:58, Japin Li <[email protected]> wrote:
>
> Thanks for updating the patch. Here are some comments for v24.
>
> + <para>
> + Terminate any session that spans longer than the specified amount of
> + time in transaction. The limit applies both to explicit transactions
> + (started with <command>BEGIN</command>) 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.
> + </para>
> The sentence "But this limit is not applied to prepared transactions" is redundant,
> since we have a paragraph to describe this later.
Fixed.
>
> +
> + <para>
> + If <varname>transaction_timeout</varname> is shorter than
> + <varname>idle_in_transaction_session_timeout</varname> or <varname>statement_timeout</varname>
> + <varname>transaction_timeout</varname> will invalidate longer timeout.
> + </para>
> +
>
> Since we are already try to disable the timeouts, should we try to disable
> them even if they are equal.
Well, we disable timeouts on equality. Fixed docs.
>
> +
> + <para>
> + Prepared transactions are not subject for this timeout.
> + </para>
>
> Maybe wrap this with <note> is a good idea.
Done.
>
>> I’ve inspected CI fails and they were caused by two different problems:
>> 1. It’s unsafe for isaoltion tester to await transaction_timeout within a query. Usually it gets
>> FATAL: terminating connection due to transaction timeout
>> But if VM is a bit slow it can get occasional
>> PQconsumeInput failed: server closed the connection unexpectedly
>> So, currently all tests use “passive waiting”, in a session that will not timeout.
>>
>> 2. In some cases pg_sleep(0.1) were sleeping up to 200 ms. That was making s7 and s8 fail, because they rely on this margin.
>
> I'm curious why this happened.
I think pg_sleep() cannot provide guarantees on when next query will be executed. In our case we need that isolation tester see that sleep is over and continue in other session...
>> I’ve separated these tests into different test timeouts-long and increased margin to 300ms. Now tests run horrible 2431 ms. Moreover I’m afraid that on buildfarm we can have much randomly-slower machines so this test might be excluded.
>> This test checks COMMIT AND CHAIN and flow of small queries (Nik’s case).
>>
>> Also I’ve verified that every "enable_timeout_after(TRANSACTION_TIMEOUT)” and “disable_timeout(TRANSACTION_TIMEOUT)” is necessary and found that case of aborting "idle in transaction (aborted)” is not covered by tests. I’m not sure we need a test for this.
>
> I see there is a test about idle_in_transaction_timeout and transaction_timeout.
>
> Both of them only check the session, but don't check the reason, so we cannot
> distinguish the reason they are terminated. Right?
Yes.
>
>> Japin, Junwang, what do you think?
>
> However, checking the reason on the timeout session may cause regression test
> failed (as you point in 1), I don't strongly insist on it.
Indeed, if we check a reason of FATAL timeouts - we get flaky tests.
Best regards, Andrey Borodin.
Attachments:
[application/octet-stream] v25-0001-Introduce-transaction_timeout.patch (26.1K, ../../[email protected]/2-v25-0001-Introduce-transaction_timeout.patch)
download | inline diff:
From 5154fbc3377aa9a4025f04c90da61861ac558761 Mon Sep 17 00:00:00 2001
From: "Andrey M. Borodin" <[email protected]>
Date: Sun, 3 Dec 2023 23:18:00 +0500
Subject: [PATCH v25] 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 <[email protected]>
Author: Japin Li <[email protected]>
Author: Junwang Zhao <[email protected]>
Reviewed-by: Nikolay Samokhvalov <[email protected]>
Reviewed-by: Andres Freund <[email protected]>
Reviewed-by: Fujii Masao <[email protected]>
Reviewed-by: bt23nguyent <[email protected]>
Reviewed-by: Yuhang Qiu <[email protected]>
Discussion: https://postgr.es/m/CAAhFRxiQsRs2Eq5kCo9nXE3HTugsAAJdSQSmxncivebAxdmBjQ%40mail.gmail.com
---
doc/src/sgml/config.sgml | 36 +++++++++
src/backend/access/transam/xact.c | 4 +
src/backend/postmaster/autovacuum.c | 2 +
src/backend/storage/lmgr/proc.c | 1 +
src/backend/tcop/postgres.c | 33 +++++++-
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 | 3 +
src/test/isolation/expected/timeouts-long.out | 69 ++++++++++++++++
src/test/isolation/expected/timeouts.out | 79 ++++++++++++++++++-
src/test/isolation/isolation_schedule | 1 +
src/test/isolation/specs/timeouts-long.spec | 35 ++++++++
src/test/isolation/specs/timeouts.spec | 40 +++++++++-
22 files changed, 330 insertions(+), 5 deletions(-)
create mode 100644 src/test/isolation/expected/timeouts-long.out
create mode 100644 src/test/isolation/specs/timeouts-long.spec
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 61038472c5a..ddbfa9a631b 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -9135,6 +9135,42 @@ COPY postgres_log FROM '/full/path/to/logfile.csv' WITH csv;
</listitem>
</varlistentry>
+ <varlistentry id="guc-transaction-timeout" xreflabel="transaction_timeout">
+ <term><varname>transaction_timeout</varname> (<type>integer</type>)
+ <indexterm>
+ <primary><varname>transaction_timeout</varname> configuration parameter</primary>
+ </indexterm>
+ </term>
+ <listitem>
+ <para>
+ Terminate any session that spans longer than the specified amount of
+ time in transaction. The limit applies both to explicit transactions
+ (started with <command>BEGIN</command>) and to implicitly started
+ transaction corresponding to single statement.
+ If this value is specified without units, it is taken as milliseconds.
+ A value of zero (the default) disables the timeout.
+ </para>
+
+ <para>
+ If <varname>transaction_timeout</varname> is shorter or eqaul to
+ <varname>idle_in_transaction_session_timeout</varname> or <varname>statement_timeout</varname>
+ <varname>transaction_timeout</varname> will invalidate longer timeout.
+ </para>
+
+ <para>
+ Setting <varname>transaction_timeout</varname> in
+ <filename>postgresql.conf</filename> is not recommended because it would
+ affect all sessions.
+ </para>
+
+ <note>
+ <para>
+ Prepared transactions are not subject for this timeout.
+ </para>
+ </note>
+ </listitem>
+ </varlistentry>
+
<varlistentry id="guc-lock-timeout" xreflabel="lock_timeout">
<term><varname>lock_timeout</varname> (<type>integer</type>)
<indexterm>
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index 464858117e0..a124ba59330 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -2139,6 +2139,10 @@ StartTransaction(void)
*/
s->state = TRANS_INPROGRESS;
+ /* Schedule transaction timeout */
+ if (TransactionTimeout > 0)
+ enable_timeout_after(TRANSACTION_TIMEOUT, TransactionTimeout);
+
ShowTransactionState("StartTransaction");
}
diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index 2c3099f76f1..c12fc6594ce 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 e5977548fe2..1afcbfc052c 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 1a34bd3715f..af28f425ce6 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -3426,6 +3426,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,12 +4502,18 @@ 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,
IdleInTransactionSessionTimeout);
}
+
+ /* Schedule or reschedule transaction timeout */
+ if (TransactionTimeout > 0 && !get_timeout_active(TRANSACTION_TIMEOUT))
+ enable_timeout_after(TRANSACTION_TIMEOUT,
+ TransactionTimeout);
}
else if (IsTransactionOrTransactionBlock())
{
@@ -4504,12 +4521,18 @@ 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,
IdleInTransactionSessionTimeout);
}
+
+ /* Schedule or reschedule transaction timeout */
+ if (TransactionTimeout > 0 && !get_timeout_active(TRANSACTION_TIMEOUT))
+ enable_timeout_after(TRANSACTION_TIMEOUT,
+ TransactionTimeout);
}
else
{
@@ -4562,6 +4585,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 +5146,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 29f367a5e1c..3250d539e1c 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 88b03e8fa3c..f024b1a8497 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 1ad33671598..7797876d008 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 7fe58518d7d..0fb5ec648e4 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -2577,6 +2577,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 da10b43dac3..3b8992f0fbf 100644
--- a/src/backend/utils/misc/postgresql.conf.sample
+++ b/src/backend/utils/misc/postgresql.conf.sample
@@ -701,6 +701,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 256d1e35a4e..3342971bd01 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 a19443becd6..119cfbcf0f5 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -1252,6 +1252,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 11347ab1824..7d898c3b501 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 0b01c1f0935..0445fbf61d7 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 4bc226e36cd..20d6fa652dc 100644
--- a/src/include/storage/proc.h
+++ b/src/include/storage/proc.h
@@ -429,6 +429,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 20e7cf72d0d..a5d8f078246 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 ade2256ed3a..91307e1a7e8 100644
--- a/src/test/isolation/Makefile
+++ b/src/test/isolation/Makefile
@@ -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 timeouts-long
diff --git a/src/test/isolation/expected/timeouts-long.out b/src/test/isolation/expected/timeouts-long.out
new file mode 100644
index 00000000000..26a6672c051
--- /dev/null
+++ b/src/test/isolation/expected/timeouts-long.out
@@ -0,0 +1,69 @@
+Parsed test spec with 3 sessions
+
+starting permutation: s7_begin s7_sleep s7_commit_and_chain s7_sleep s7_check s7_abort
+step s7_begin:
+ BEGIN ISOLATION LEVEL READ COMMITTED;
+ SET transaction_timeout = '1s';
+
+step s7_sleep: SELECT pg_sleep(0.6);
+pg_sleep
+--------
+
+(1 row)
+
+step s7_commit_and_chain: COMMIT AND CHAIN;
+step s7_sleep: SELECT pg_sleep(0.6);
+pg_sleep
+--------
+
+(1 row)
+
+step s7_check: SELECT count(*) FROM pg_stat_activity WHERE application_name = 'isolation/timeouts/s7';
+count
+-----
+ 0
+(1 row)
+
+step s7_abort: ABORT;
+
+starting permutation: s8_begin s8_sleep s8_select_1 s8_check checker_sleep checker_sleep s8_check
+step s8_begin:
+ BEGIN ISOLATION LEVEL READ COMMITTED;
+ SET transaction_timeout = '900ms';
+
+step s8_sleep: SELECT pg_sleep(0.6);
+pg_sleep
+--------
+
+(1 row)
+
+step s8_select_1: SELECT 1;
+?column?
+--------
+ 1
+(1 row)
+
+step s8_check: SELECT count(*) FROM pg_stat_activity WHERE application_name = 'isolation/timeouts/s8';
+count
+-----
+ 0
+(1 row)
+
+step checker_sleep: SELECT pg_sleep(0.3);
+pg_sleep
+--------
+
+(1 row)
+
+step checker_sleep: SELECT pg_sleep(0.3);
+pg_sleep
+--------
+
+(1 row)
+
+step s8_check: SELECT count(*) FROM pg_stat_activity WHERE application_name = 'isolation/timeouts/s8';
+count
+-----
+ 0
+(1 row)
+
diff --git a/src/test/isolation/expected/timeouts.out b/src/test/isolation/expected/timeouts.out
index 9328676f1cc..81a0016375b 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 7 sessions
starting permutation: rdtbl sto locktbl
step rdtbl: SELECT * FROM accounts;
@@ -79,3 +79,80 @@ step slto: SET lock_timeout = '10s'; SET statement_timeout = '10ms';
step update: DELETE FROM accounts WHERE accountid = 'checking'; <waiting ...>
step update: <... completed>
ERROR: canceling statement due to statement timeout
+
+starting permutation: stto s3_begin s3_sleep s3_check s3_abort
+step stto: SET statement_timeout = '10ms'; SET transaction_timeout = '1s';
+step s3_begin: BEGIN ISOLATION LEVEL READ COMMITTED;
+step s3_sleep: SELECT pg_sleep(0.1);
+ERROR: canceling statement due to statement timeout
+step s3_check: SELECT count(*) FROM pg_stat_activity WHERE application_name = 'isolation/timeouts/s3';
+count
+-----
+ 1
+(1 row)
+
+step s3_abort: ABORT;
+
+starting permutation: tsto s3_begin checker_sleep s3_check
+step tsto: SET statement_timeout = '1s'; SET transaction_timeout = '10ms';
+step s3_begin: BEGIN ISOLATION LEVEL READ COMMITTED;
+step checker_sleep: SELECT pg_sleep(0.1);
+pg_sleep
+--------
+
+(1 row)
+
+step s3_check: SELECT count(*) FROM pg_stat_activity WHERE application_name = 'isolation/timeouts/s3';
+count
+-----
+ 0
+(1 row)
+
+
+starting permutation: itto s4_begin checker_sleep s4_check
+step itto: SET idle_in_transaction_session_timeout = '10ms'; SET transaction_timeout = '1s';
+step s4_begin: BEGIN ISOLATION LEVEL READ COMMITTED;
+step checker_sleep: SELECT pg_sleep(0.1);
+pg_sleep
+--------
+
+(1 row)
+
+step s4_check: SELECT count(*) FROM pg_stat_activity WHERE application_name = 'isolation/timeouts/s4';
+count
+-----
+ 0
+(1 row)
+
+
+starting permutation: tito s5_begin checker_sleep s5_check
+step tito: SET idle_in_transaction_session_timeout = '1s'; SET transaction_timeout = '10ms';
+step s5_begin: BEGIN ISOLATION LEVEL READ COMMITTED;
+step checker_sleep: SELECT pg_sleep(0.1);
+pg_sleep
+--------
+
+(1 row)
+
+step s5_check: SELECT count(*) FROM pg_stat_activity WHERE application_name = 'isolation/timeouts/s5';
+count
+-----
+ 0
+(1 row)
+
+
+starting permutation: s6_begin s6_tt checker_sleep s6_check
+step s6_begin: BEGIN ISOLATION LEVEL READ COMMITTED;
+step s6_tt: SET statement_timeout = '1s'; SET transaction_timeout = '10ms';
+step checker_sleep: SELECT pg_sleep(0.1);
+pg_sleep
+--------
+
+(1 row)
+
+step s6_check: SELECT count(*) FROM pg_stat_activity WHERE application_name = 'isolation/timeouts/s6';
+count
+-----
+ 0
+(1 row)
+
diff --git a/src/test/isolation/isolation_schedule b/src/test/isolation/isolation_schedule
index b2be88ead1d..86ef62bbcf6 100644
--- a/src/test/isolation/isolation_schedule
+++ b/src/test/isolation/isolation_schedule
@@ -89,6 +89,7 @@ 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
new file mode 100644
index 00000000000..ce2c9a43011
--- /dev/null
+++ b/src/test/isolation/specs/timeouts-long.spec
@@ -0,0 +1,35 @@
+# 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
diff --git a/src/test/isolation/specs/timeouts.spec b/src/test/isolation/specs/timeouts.spec
index c747b4ae28d..c2cc5d8d37b 100644
--- a/src/test/isolation/specs/timeouts.spec
+++ b/src/test/isolation/specs/timeouts.spec
@@ -1,4 +1,4 @@
-# Simple tests for statement_timeout and lock_timeout features
+# Simple tests for statement_timeout, lock_timeout and transaction_timeout features
setup
{
@@ -27,6 +27,33 @@ step locktbl { LOCK TABLE accounts; }
step update { DELETE FROM accounts WHERE accountid = 'checking'; }
teardown { ABORT; }
+session s3
+step s3_begin { BEGIN ISOLATION LEVEL READ COMMITTED; }
+step stto { SET statement_timeout = '10ms'; SET transaction_timeout = '1s'; }
+step tsto { SET statement_timeout = '1s'; SET transaction_timeout = '10ms'; }
+step s3_sleep { SELECT pg_sleep(0.1); }
+step s3_abort { ABORT; }
+
+session s4
+step s4_begin { BEGIN ISOLATION LEVEL READ COMMITTED; }
+step itto { SET idle_in_transaction_session_timeout = '10ms'; SET transaction_timeout = '1s'; }
+
+session s5
+step s5_begin { BEGIN ISOLATION LEVEL READ COMMITTED; }
+step tito { SET idle_in_transaction_session_timeout = '1s'; SET transaction_timeout = '10ms'; }
+
+session s6
+step s6_begin { BEGIN ISOLATION LEVEL READ COMMITTED; }
+step s6_tt { SET statement_timeout = '1s'; SET transaction_timeout = '10ms'; }
+
+session checker
+step checker_sleep { SELECT pg_sleep(0.1); }
+step s3_check { SELECT count(*) FROM pg_stat_activity WHERE application_name = 'isolation/timeouts/s3'; }
+step s4_check { SELECT count(*) FROM pg_stat_activity WHERE application_name = 'isolation/timeouts/s4'; }
+step s5_check { SELECT count(*) FROM pg_stat_activity WHERE application_name = 'isolation/timeouts/s5'; }
+step s6_check { SELECT count(*) FROM pg_stat_activity WHERE application_name = 'isolation/timeouts/s6'; }
+
+
# 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 +74,14 @@ permutation wrtbl lto update(*)
permutation wrtbl lsto update(*)
# statement timeout expires first, row-level lock
permutation wrtbl slto update(*)
+
+# statement timeout expires first
+permutation stto s3_begin s3_sleep s3_check s3_abort
+# transaction timeout expires first, session s3 FATAL-out
+permutation tsto s3_begin checker_sleep s3_check
+# idle in transaction timeout expires first, session s4 FATAL-out
+permutation itto s4_begin checker_sleep s4_check
+# transaction timeout expires first, session s5 FATAL-out
+permutation tito s5_begin checker_sleep s5_check
+# transaction timeout can be schedule amid transaction, session s6 FATAL-out
+permutation s6_begin s6_tt checker_sleep s6_check
\ No newline at end of file
--
2.42.0
^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: Transaction timeout
2022-12-03 05:18 Transaction timeout Andrey Borodin <[email protected]>
2024-01-22 06:23 ` Re: Transaction timeout Peter Smith <[email protected]>
2024-01-26 06:44 ` Re: Transaction timeout Andrey M. Borodin <[email protected]>
2024-01-26 14:58 ` Re: Transaction timeout Japin Li <[email protected]>
2024-01-30 06:22 ` Re: Transaction timeout Andrey M. Borodin <[email protected]>
@ 2024-01-31 09:27 ` Japin Li <[email protected]>
2024-01-31 09:57 ` Re: Transaction timeout Andrey Borodin <[email protected]>
0 siblings, 1 reply; 22+ messages in thread
From: Japin Li @ 2024-01-31 09:27 UTC (permalink / raw)
To: Andrey M. Borodin <[email protected]>; +Cc: Peter Smith <[email protected]>; Junwang Zhao <[email protected]>; 邱宇航 <[email protected]>; Fujii Masao <[email protected]>; Andrey Borodin <[email protected]>; Andres Freund <[email protected]>; Michael Paquier <[email protected]>; Nikolay Samokhvalov <[email protected]>; pgsql-hackers; [email protected] <[email protected]>
On Tue, 30 Jan 2024 at 14:22, Andrey M. Borodin <[email protected]> wrote:
>> On 26 Jan 2024, at 19:58, Japin Li <[email protected]> wrote:
>>
>> Thanks for updating the patch. Here are some comments for v24.
>>
>> + <para>
>> + Terminate any session that spans longer than the specified amount of
>> + time in transaction. The limit applies both to explicit transactions
>> + (started with <command>BEGIN</command>) 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.
>> + </para>
>> The sentence "But this limit is not applied to prepared transactions" is redundant,
>> since we have a paragraph to describe this later.
> Fixed.
>>
>> +
>> + <para>
>> + If <varname>transaction_timeout</varname> is shorter than
>> + <varname>idle_in_transaction_session_timeout</varname> or <varname>statement_timeout</varname>
>> + <varname>transaction_timeout</varname> will invalidate longer timeout.
>> + </para>
>> +
>>
>> Since we are already try to disable the timeouts, should we try to disable
>> them even if they are equal.
>
> Well, we disable timeouts on equality. Fixed docs.
>
>>
>> +
>> + <para>
>> + Prepared transactions are not subject for this timeout.
>> + </para>
>>
>> Maybe wrap this with <note> is a good idea.
> Done.
>
Thanks for updating the patch. LGTM.
If there is no other objections, I'll change it to ready for committer
next Monday.
^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: Transaction timeout
2022-12-03 05:18 Transaction timeout Andrey Borodin <[email protected]>
2024-01-22 06:23 ` Re: Transaction timeout Peter Smith <[email protected]>
2024-01-26 06:44 ` Re: Transaction timeout Andrey M. Borodin <[email protected]>
2024-01-26 14:58 ` Re: Transaction timeout Japin Li <[email protected]>
2024-01-30 06:22 ` Re: Transaction timeout Andrey M. Borodin <[email protected]>
2024-01-31 09:27 ` Re: Transaction timeout Japin Li <[email protected]>
@ 2024-01-31 09:57 ` Andrey Borodin <[email protected]>
0 siblings, 0 replies; 22+ messages in thread
From: Andrey Borodin @ 2024-01-31 09:57 UTC (permalink / raw)
To: Japin Li <[email protected]>; +Cc: Peter Smith <[email protected]>; Junwang Zhao <[email protected]>; 邱宇航 <[email protected]>; Fujii Masao <[email protected]>; Andrew Borodin <[email protected]>; Andres Freund <[email protected]>; Michael Paquier <[email protected]>; Nikolay Samokhvalov <[email protected]>; pgsql-hackers; pgsql-hackers mailing list <[email protected]>
> On 31 Jan 2024, at 14:27, Japin Li <[email protected]> wrote:
>
> LGTM.
>
> If there is no other objections, I'll change it to ready for committer
> next Monday.
I think we have a quorum, so I decided to go ahead and flipped status to RfC. Thanks!
Best regards, Andrey Borodin.
^ permalink raw reply [nested|flat] 22+ messages in thread
end of thread, other threads:[~2024-01-31 09:57 UTC | newest]
Thread overview: 22+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2001-03-17 01:10 transaction timeout Kevin T. Manley <[email protected]>
2022-12-03 05:18 Transaction timeout Andrey Borodin <[email protected]>
2022-12-03 06:59 ` Re: Transaction timeout Nikolay Samokhvalov <[email protected]>
2022-12-03 17:41 ` Re: Transaction timeout Andrey Borodin <[email protected]>
2022-12-05 22:17 ` Re: Transaction timeout Nikolay Samokhvalov <[email protected]>
2022-12-05 22:28 ` Re: Transaction timeout Nikolay Samokhvalov <[email protected]>
2022-12-05 23:07 ` Re: Transaction timeout Andres Freund <[email protected]>
2022-12-05 23:41 ` Re: Transaction timeout Andrey Borodin <[email protected]>
2022-12-06 00:22 ` Re: Transaction timeout Andres Freund <[email protected]>
2022-12-06 00:44 ` Re: Transaction timeout Kyotaro Horiguchi <[email protected]>
2022-12-06 01:10 ` Re: Transaction timeout Andres Freund <[email protected]>
2022-12-07 02:52 ` Re: Transaction timeout Kyotaro Horiguchi <[email protected]>
2022-12-07 18:23 ` Re: Transaction timeout Andres Freund <[email protected]>
2022-12-07 21:30 ` Re: Transaction timeout Andrey Borodin <[email protected]>
2022-12-18 20:53 ` Re: Transaction timeout Andrey Borodin <[email protected]>
2024-01-22 06:23 ` Re: Transaction timeout Peter Smith <[email protected]>
2024-01-26 06:44 ` Re: Transaction timeout Andrey M. Borodin <[email protected]>
2024-01-26 06:46 ` Re: Transaction timeout Andrey M. Borodin <[email protected]>
2024-01-26 14:58 ` Re: Transaction timeout Japin Li <[email protected]>
2024-01-30 06:22 ` Re: Transaction timeout Andrey M. Borodin <[email protected]>
2024-01-31 09:27 ` Re: Transaction timeout Japin Li <[email protected]>
2024-01-31 09:57 ` Re: Transaction timeout Andrey Borodin <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox