Re: Transaction timeout

On Fri, 26 Jan 2024 at 14:44, Andrey M. Borodin <x4mmm@yandex-team.ru> wrote:
>> On 22 Jan 2024, at 11:23, Peter Smith <smithpb2250@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
2431ms. 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)”
isnecessary and found that case of aborting "idle in transaction (aborted)” is not covered by tests. I’m not sure we
needa 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.