> On 26 Jan 2024, at 19:58, Japin Li <japi...@hotmail.com> wrote: > > Thanks for updating the patch. Here are some comments for v24. > > + <para> > + Terminate any session that spans longer than the specified amount of > + time in transaction. The limit applies both to explicit transactions > + (started with <command>BEGIN</command>) and to implicitly started > + transaction corresponding to single statement. But this limit is not > + applied to prepared transactions. > + If this value is specified without units, it is taken as > milliseconds. > + A value of zero (the default) disables the timeout. > + </para> > The sentence "But this limit is not applied to prepared transactions" is > redundant, > since we have a paragraph to describe this later. Fixed. > > + > + <para> > + If <varname>transaction_timeout</varname> is shorter than > + <varname>idle_in_transaction_session_timeout</varname> or > <varname>statement_timeout</varname> > + <varname>transaction_timeout</varname> will invalidate longer > timeout. > + </para> > + > > Since we are already try to disable the timeouts, should we try to disable > them even if they are equal.
Well, we disable timeouts on equality. Fixed docs. > > + > + <para> > + Prepared transactions are not subject for this timeout. > + </para> > > Maybe wrap this with <note> is a good idea. Done. > >> I’ve inspected CI fails and they were caused by two different problems: >> 1. It’s unsafe for isaoltion tester to await transaction_timeout within a >> query. Usually it gets >> FATAL: terminating connection due to transaction timeout >> But if VM is a bit slow it can get occasional >> PQconsumeInput failed: server closed the connection unexpectedly >> So, currently all tests use “passive waiting”, in a session that will not >> timeout. >> >> 2. In some cases pg_sleep(0.1) were sleeping up to 200 ms. That was making >> s7 and s8 fail, because they rely on this margin. > > I'm curious why this happened. I think pg_sleep() cannot provide guarantees on when next query will be executed. In our case we need that isolation tester see that sleep is over and continue in other session... >> I’ve separated these tests into different test timeouts-long and increased >> margin to 300ms. Now tests run horrible 2431 ms. Moreover I’m afraid that on >> buildfarm we can have much randomly-slower machines so this test might be >> excluded. >> This test checks COMMIT AND CHAIN and flow of small queries (Nik’s case). >> >> Also I’ve verified that every "enable_timeout_after(TRANSACTION_TIMEOUT)” >> and “disable_timeout(TRANSACTION_TIMEOUT)” is necessary and found that case >> of aborting "idle in transaction (aborted)” is not covered by tests. I’m not >> sure we need a test for this. > > I see there is a test about idle_in_transaction_timeout and > transaction_timeout. > > Both of them only check the session, but don't check the reason, so we cannot > distinguish the reason they are terminated. Right? Yes. > >> Japin, Junwang, what do you think? > > However, checking the reason on the timeout session may cause regression test > failed (as you point in 1), I don't strongly insist on it. Indeed, if we check a reason of FATAL timeouts - we get flaky tests. Best regards, Andrey Borodin.
v25-0001-Introduce-transaction_timeout.patch
Description: Binary data
