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