Re: Transaction timeout

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