On Fri, 26 Jan 2024 at 14:44, Andrey M. Borodin <x4...@yandex-team.ru> wrote: >> On 22 Jan 2024, at 11:23, Peter Smith <smithpb2...@gmail.com> wrote: >> >> Hi, This patch has a CF status of "Needs Review" [1], but it seems >> there was a CFbot test failure last time it was run [2]. Please have a >> look and post an updated version if necessary. > Thanks Peter! >
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. + + <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. + + <para> + Prepared transactions are not subject for this timeout. + </para> Maybe wrap this with <note> is a good idea. > 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’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? > 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. -- Best regards, Japin Li.
