Re: Transaction timeout

Hi,

On 2024-02-13 23:42:35 +0200, Alexander Korotkov wrote:
> 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");
> }

Isn't it a problem that all uses of StartTransaction() trigger a timeout, but
transaction commit/abort don't? What if e.g. logical replication apply starts
a transaction, commits it, and then goes idle? The timer would still be
active, afaict?

I don't think it works well to enable timeouts in xact.c and to disable them
in PostgresMain().

> @@ -4491,12 +4511,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 +4530,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
> {

Why do we need to do anything in these cases if the timer is started in
StartTransaction()?

> 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); }

Isn't this test going to be very fragile on busy / slow machines? What if the
pg_sleep() takes one second, because there were other tasks to schedule? I'd
be surprised if this didn't fail under valgrind, for example.

Greetings,

Andres Freund