Re: Transaction timeout - Mailing list 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