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