From: Andrey M. Borodin <x4mmm@yandex-team.ru>
To: Alexander Korotkov <aekorotkov@gmail.com>
Cc: Michael Paquier <michael@paquier.xyz>
Cc: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Cc: akorotkov@postgresql.org
Cc: pgsql-committers@lists.postgresql.org
Subject: Re: pgsql: Add TAP tests for timeouts
Date: Fri, 15 Mar 2024 17:25:24 +0500
Message-ID: <F719524D-ACE0-474D-8E45-229EFE5AE195@yandex-team.ru> (raw)
In-Reply-To: <CAPpHfdvAFor_8cftdNtXvnDAyVmv77AwtLGyEa+Gb5FVMbjZ6A@mail.gmail.com>
References: <E1rkjDR-003kjB-St@gemulon.postgresql.org>
<20240315.104235.1835366724413653745.horikyota.ntt@gmail.com>
<ZfPchPC6oNN71X2J@paquier.xyz>
<2D5B6D1F-11D2-4306-B006-4FE53D5F0E6A@yandex-team.ru>
<ZfP8St9syoccloLg@paquier.xyz>
<CAPpHfdsp6s6yN8XUV6Ke_1SO7iKq6CWa2g2qO7J1T3E1o+a=nA@mail.gmail.com>
<91C963B5-63CC-4FAE-9119-E226CF84112A@yandex-team.ru>
<CAPpHfdvAFor_8cftdNtXvnDAyVmv77AwtLGyEa+Gb5FVMbjZ6A@mail.gmail.com>
> On 15 Mar 2024, at 16:30, Alexander Korotkov <aekorotkov@gmail.com> wrote:
>
> Maybe, but do you see any negative side effects of the unconditionally
> unset of flags?
Nope, just expressed possible option.
> If not, I would prefer to keep the code simple.
IMO that's fine. Let's incorporate wording improvement from Kyotaro to the same patch? PFA draft for this.
Maybe it worth to wait a little to see if some other failures will pop up?
Best regards, Andrey Borodin.
Attachments:v2-0001-Fix-race-condition-in-transaction-timeout-TAP-tes.patch (application/octet-stream, 4.3K) download
From b79d828ea4958a3ef6a1e79736c12683cfab4bb2 Mon Sep 17 00:00:00 2001
From: "Andrey M. Borodin" <x4mmm@night.local>
Date: Fri, 15 Mar 2024 17:18:19 +0500
Subject: [PATCH v2] Fix race condition in transaction timeout TAP tests
Interruption handler within injection point can stuck in infinite
loop while handling transaction timeout. To avoid this situation
we reset timeout flag before invoking injection point.
Author: Alexander Korotkov
Reviewed-by: Andrey Borodin
Discussion: https://www.postgresql.org/message-id/flat/E1rkjDR-003kjB-St%40gemulon.postgresql.org
---
src/backend/tcop/postgres.c | 12 +++++-------
src/test/modules/test_misc/t/005_timeouts.pl | 12 ++++++------
2 files changed, 11 insertions(+), 13 deletions(-)
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.cindex 7ac623019b..fd4199a098 100644--- a/src/backend/tcop/postgres.c+++ b/src/backend/tcop/postgres.c@@ -3409,8 +3409,10 @@ ProcessInterrupts(void)
/*
* If the GUC has been reset to zero, ignore the signal. This is
* important because the GUC update itself won't disable any pending
- * interrupt.+ * interrupt. We need to unset the flag before the injection point,+ * otherwise we could loop in interrupts checking.
*/
+ IdleInTransactionSessionTimeoutPending = false;
if (IdleInTransactionSessionTimeout > 0)
{
INJECTION_POINT("idle-in-transaction-session-timeout");
@@ -3418,13 +3420,12 @@ ProcessInterrupts(void)
(errcode(ERRCODE_IDLE_IN_TRANSACTION_SESSION_TIMEOUT),
errmsg("terminating connection due to idle-in-transaction timeout")));
}
- else- IdleInTransactionSessionTimeoutPending = false;
}
if (TransactionTimeoutPending)
{
/* As above, ignore the signal if the GUC has been reset to zero. */
+ TransactionTimeoutPending = false;
if (TransactionTimeout > 0)
{
INJECTION_POINT("transaction-timeout");
@@ -3432,13 +3433,12 @@ ProcessInterrupts(void)
(errcode(ERRCODE_TRANSACTION_TIMEOUT),
errmsg("terminating connection due to transaction timeout")));
}
- else- TransactionTimeoutPending = false;
}
if (IdleSessionTimeoutPending)
{
/* As above, ignore the signal if the GUC has been reset to zero. */
+ IdleSessionTimeoutPending = false;
if (IdleSessionTimeout > 0)
{
INJECTION_POINT("idle-session-timeout");
@@ -3446,8 +3446,6 @@ ProcessInterrupts(void)
(errcode(ERRCODE_IDLE_SESSION_TIMEOUT),
errmsg("terminating connection due to idle-session timeout")));
}
- else- IdleSessionTimeoutPending = false;
}
/*
diff --git a/src/test/modules/test_misc/t/005_timeouts.pl b/src/test/modules/test_misc/t/005_timeouts.plindex e67b3e694b..dc45213685 100644--- a/src/test/modules/test_misc/t/005_timeouts.pl+++ b/src/test/modules/test_misc/t/005_timeouts.pl@@ -44,7 +44,7 @@ $psql_session->query_until(
\q
));
-# Wait until the backend is in the timeout injection point. Will get an error+# Wait until the backend enters the timeout injection point. Will get an error
# here if anything goes wrong.
$node->wait_for_event('client backend', 'transaction-timeout');
@@ -58,13 +58,13 @@ $node->safe_psql('postgres',
$node->wait_for_log('terminating connection due to transaction timeout',
$log_offset);
-# If we send \q with $psql_session->quit it can get to pump already closed.-# So \q is in initial script, here we only finish IPC::Run.+# If we send \q with $psql_session->quit the command can be sent to the session+# already closed. So \q is in initial script, here we only finish IPC::Run.
$psql_session->{run}->finish;
#
-# 2. Test of the sidle in transaction timeout+# 2. Test of the idle in transaction timeout
#
$node->safe_psql('postgres',
@@ -80,7 +80,7 @@ $psql_session->query_until(
BEGIN;
));
-# Wait until the backend is in the timeout injection point.+# Wait until the backend enters the timeout injection point.
$node->wait_for_event('client backend',
'idle-in-transaction-session-timeout');
@@ -111,7 +111,7 @@ $psql_session->query_until(
SET idle_session_timeout to '10ms';
));
-# Wait until the backend is in the timeout injection point.+# Wait until the backend enters the timeout injection point.
$node->wait_for_event('client backend', 'idle-session-timeout');
$log_offset = -s $node->logfile;
--
2.37.1 (Apple Git-137.1)
=
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-committers@postgresql.org
Cc: x4mmm@yandex-team.ru, aekorotkov@gmail.com, michael@paquier.xyz, horikyota.ntt@gmail.com, akorotkov@postgresql.org, pgsql-committers@lists.postgresql.org
Subject: Re: pgsql: Add TAP tests for timeouts
In-Reply-To: <F719524D-ACE0-474D-8E45-229EFE5AE195@yandex-team.ru>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox