Hey Andrey,
On Sun, Dec 24, 2023 at 1:14 AM Andrey M. Borodin <[email protected]> wrote:
>
>
>
> > On 22 Dec 2023, at 10:39, Japin Li <[email protected]> wrote:
> >
> >
> > I try to split the test for transaction timeout, and all passed on my CI
> > [1].
>
>
> I like the refactoring you did in timeout.spec. I thought it is impossible,
> because permutations would try to reinitialize FATALed sessions. But,
> obviously, tests work the way you refactored it.
> However I don't think ignoring test failures on Windows without understanding
> root cause is a good idea.
> Let's get back to v13 version of tests, understand why it failed, apply your
> test refactorings afterwards. BTW are you sure that v14 refactorings are
> functional equivalent of v13 tests?
>
> To go with this plan I attach slightly modified version of v13 tests in v16
> patchset. The only change is timing in "sleep_there" step. I suspect that
> failure was induced by more coarse timer granularity on Windows. Tests were
> giving only 9 milliseconds for a timeout to entirely wipe away backend from
> pg_stat_activity. This saves testing time, but might induce false positive
> test flaps. So I've raised wait times to 100ms. This seems too much, but I do
> not have other ideas how to ensure tests stability. Maybe 50ms would be
> enough, I do not know. Isolation runs ~50 seconds now. I'm tempted to say
> that 200ms for timeouts worth it.
>
>
> As to 2nd step "Try to enable transaction_timeout during transaction", I
> think this makes sense. But if we are doing so, shouldn't we also allow to
> enable idle_in_transaction timeout in a same manner? Currently we only allow
> to disable other timeouts... Also, if we are already in transaction,
> shouldn't we also subtract current transaction span from timeout?
> I think making this functionality as another step of the patchset was a good
> idea.
>
> Thanks!
Seems V5~V17 doesn't work as expected for Nikolay's case:
postgres=# set transaction_timeout to '2s';
SET
postgres=# begin; select pg_sleep(1); select pg_sleep(1); select
pg_sleep(1); select pg_sleep(1); select pg_sleep(1); commit;
BEGIN
The reason for this seems the timer has been refreshed for each
command, xact_started along can not indicate it's a new
transaction or not, there is a TransactionState contains some
infos.
So I propose the following change, what do you think?
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index a2611cf8e6..cffd2c44d0 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -2746,7 +2746,7 @@ start_xact_command(void)
StartTransactionCommand();
/* Schedule or reschedule transaction timeout */
- if (TransactionTimeout > 0)
+ if (TransactionTimeout > 0 &&
!get_timeout_active(TRANSACTION_TIMEOUT))
enable_timeout_after(TRANSACTION_TIMEOUT,
TransactionTimeout);
xact_started = true;
>
>
> Best regards, Andrey Borodin.
--
Regards
Junwang Zhao
