pg.ddx.io pgsql-committers@postgresql.org mailing list archive
help / color / mirror / Atom feedpgsql: Fix xslt_process() to ensure that it inserts a NULL terminator
17+ messages / 6 participants
[nested] [flat]
Commits from this thread:
2024-03-15 4c2eda67f5 Fix race condition in transaction timeout TAP tests
2024-03-15 7a65cc079e Fix wordings in timeouts TAP test
* pgsql: Fix xslt_process() to ensure that it inserts a NULL terminator
@ 2009-07-10 00:32 Tom Lane <tgl@postgresql.org>
0 siblings, 0 replies; 17+ messages in thread
From: tgl@postgresql.org @ 2009-07-10 00:32 UTC (permalink / raw)
To: pgsql-committers
Log Message:
-----------
Fix xslt_process() to ensure that it inserts a NULL terminator after the
last pair of parameter name/value strings, even when there are MAXPARAMS
of them. Aboriginal bug in contrib/xml2, noted while studying bug #4912
(though I'm not sure whether there's something else involved in that
report).
This might be thought a security issue, since it's a potential backend
crash; but considering that untrustworthy users shouldn't be allowed
to get their hands on xslt_process() anyway, it's probably not worth
getting excited about.
Modified Files:
--------------
pgsql/contrib/xml2:
xslt_proc.c (r1.15 -> r1.16)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/contrib/xml2/xslt_proc.c?r1=1.15&r2=1.16)
^ permalink raw reply [nested|flat] 17+ messages in thread
* pgsql: Fix xslt_process() to ensure that it inserts a NULL terminator
@ 2009-07-10 00:32 Tom Lane <tgl@postgresql.org>
0 siblings, 0 replies; 17+ messages in thread
From: tgl@postgresql.org @ 2009-07-10 00:32 UTC (permalink / raw)
To: pgsql-committers
Log Message:
-----------
Fix xslt_process() to ensure that it inserts a NULL terminator after the
last pair of parameter name/value strings, even when there are MAXPARAMS
of them. Aboriginal bug in contrib/xml2, noted while studying bug #4912
(though I'm not sure whether there's something else involved in that
report).
This might be thought a security issue, since it's a potential backend
crash; but considering that untrustworthy users shouldn't be allowed
to get their hands on xslt_process() anyway, it's probably not worth
getting excited about.
Tags:
----
REL8_4_STABLE
Modified Files:
--------------
pgsql/contrib/xml2:
xslt_proc.c (r1.15 -> r1.15.2.1)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/contrib/xml2/xslt_proc.c?r1=1.15&r2=1.15.2.1)
^ permalink raw reply [nested|flat] 17+ messages in thread
* pgsql: Fix xslt_process() to ensure that it inserts a NULL terminator
@ 2009-07-10 00:32 Tom Lane <tgl@postgresql.org>
0 siblings, 0 replies; 17+ messages in thread
From: tgl@postgresql.org @ 2009-07-10 00:32 UTC (permalink / raw)
To: pgsql-committers
Log Message:
-----------
Fix xslt_process() to ensure that it inserts a NULL terminator after the
last pair of parameter name/value strings, even when there are MAXPARAMS
of them. Aboriginal bug in contrib/xml2, noted while studying bug #4912
(though I'm not sure whether there's something else involved in that
report).
This might be thought a security issue, since it's a potential backend
crash; but considering that untrustworthy users shouldn't be allowed
to get their hands on xslt_process() anyway, it's probably not worth
getting excited about.
Tags:
----
REL8_3_STABLE
Modified Files:
--------------
pgsql/contrib/xml2:
xslt_proc.c (r1.9 -> r1.9.2.1)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/contrib/xml2/xslt_proc.c?r1=1.9&r2=1.9.2.1)
^ permalink raw reply [nested|flat] 17+ messages in thread
* pgsql: Fix xslt_process() to ensure that it inserts a NULL terminator
@ 2009-07-10 00:32 Tom Lane <tgl@postgresql.org>
0 siblings, 0 replies; 17+ messages in thread
From: tgl@postgresql.org @ 2009-07-10 00:32 UTC (permalink / raw)
To: pgsql-committers
Log Message:
-----------
Fix xslt_process() to ensure that it inserts a NULL terminator after the
last pair of parameter name/value strings, even when there are MAXPARAMS
of them. Aboriginal bug in contrib/xml2, noted while studying bug #4912
(though I'm not sure whether there's something else involved in that
report).
This might be thought a security issue, since it's a potential backend
crash; but considering that untrustworthy users shouldn't be allowed
to get their hands on xslt_process() anyway, it's probably not worth
getting excited about.
Tags:
----
REL8_2_STABLE
Modified Files:
--------------
pgsql/contrib/xml2:
xslt_proc.c (r1.7 -> r1.7.2.1)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/contrib/xml2/xslt_proc.c?r1=1.7&r2=1.7.2.1)
^ permalink raw reply [nested|flat] 17+ messages in thread
* pgsql: Fix xslt_process() to ensure that it inserts a NULL terminator
@ 2009-07-10 00:32 Tom Lane <tgl@postgresql.org>
0 siblings, 0 replies; 17+ messages in thread
From: tgl@postgresql.org @ 2009-07-10 00:32 UTC (permalink / raw)
To: pgsql-committers
Log Message:
-----------
Fix xslt_process() to ensure that it inserts a NULL terminator after the
last pair of parameter name/value strings, even when there are MAXPARAMS
of them. Aboriginal bug in contrib/xml2, noted while studying bug #4912
(though I'm not sure whether there's something else involved in that
report).
This might be thought a security issue, since it's a potential backend
crash; but considering that untrustworthy users shouldn't be allowed
to get their hands on xslt_process() anyway, it's probably not worth
getting excited about.
Tags:
----
REL8_1_STABLE
Modified Files:
--------------
pgsql/contrib/xml2:
xslt_proc.c (r1.4 -> r1.4.2.1)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/contrib/xml2/xslt_proc.c?r1=1.4&r2=1.4.2.1)
^ permalink raw reply [nested|flat] 17+ messages in thread
* pgsql: Fix xslt_process() to ensure that it inserts a NULL terminator
@ 2009-07-10 00:32 Tom Lane <tgl@postgresql.org>
0 siblings, 0 replies; 17+ messages in thread
From: tgl@postgresql.org @ 2009-07-10 00:32 UTC (permalink / raw)
To: pgsql-committers
Log Message:
-----------
Fix xslt_process() to ensure that it inserts a NULL terminator after the
last pair of parameter name/value strings, even when there are MAXPARAMS
of them. Aboriginal bug in contrib/xml2, noted while studying bug #4912
(though I'm not sure whether there's something else involved in that
report).
This might be thought a security issue, since it's a potential backend
crash; but considering that untrustworthy users shouldn't be allowed
to get their hands on xslt_process() anyway, it's probably not worth
getting excited about.
Tags:
----
REL8_0_STABLE
Modified Files:
--------------
pgsql/contrib/xml2:
xslt_proc.c (r1.3 -> r1.3.4.1)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/contrib/xml2/xslt_proc.c?r1=1.3&r2=1.3.4.1)
^ permalink raw reply [nested|flat] 17+ messages in thread
* pgsql: Add TAP tests for timeouts
@ 2024-03-14 11:25 Alexander Korotkov <akorotkov@postgresql.org>
0 siblings, 1 reply; 17+ messages in thread
From: Alexander Korotkov @ 2024-03-14 11:25 UTC (permalink / raw)
To: pgsql-committers@lists.postgresql.org
Add TAP tests for timeouts
This commit adds new tests to verify that transaction_timeout,
idle_session_timeout, and idle_in_transaction_session_timeout work as expected.
We introduce new injection points in before throwing a timeout FATAL error
and check these injection points are reached.
Discussion: https://postgr.es/m/CAAhFRxiQsRs2Eq5kCo9nXE3HTugsAAJdSQSmxncivebAxdmBjQ%40mail.gmail.com
Author: Andrey Borodin
Reviewed-by: Alexander Korotkov
Branch
------
master
Details
-------
https://git.postgresql.org/pg/commitdiff/eeefd4280f6e5167d70efabb89586b7d38922d95
Modified Files
--------------
src/backend/tcop/postgres.c | 10 +++
src/test/modules/test_misc/Makefile | 4 +
src/test/modules/test_misc/meson.build | 4 +
src/test/modules/test_misc/t/005_timeouts.pl | 129 +++++++++++++++++++++++++++
4 files changed, 147 insertions(+)
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: pgsql: Add TAP tests for timeouts
@ 2024-03-15 01:42 Kyotaro Horiguchi <horikyota.ntt@gmail.com>
parent: Alexander Korotkov <akorotkov@postgresql.org>
0 siblings, 1 reply; 17+ messages in thread
From: Kyotaro Horiguchi @ 2024-03-15 01:42 UTC (permalink / raw)
To: akorotkov@postgresql.org; +Cc: pgsql-committers@lists.postgresql.org
Hello.
At Thu, 14 Mar 2024 11:25:42 +0000, Alexander Korotkov <akorotkov@postgresql.org> wrote in
> Add TAP tests for timeouts
>
> This commit adds new tests to verify that transaction_timeout,
> idle_session_timeout, and idle_in_transaction_session_timeout work as expected.
> We introduce new injection points in before throwing a timeout FATAL error
> and check these injection points are reached.
>
> Discussion: https://postgr.es/m/CAAhFRxiQsRs2Eq5kCo9nXE3HTugsAAJdSQSmxncivebAxdmBjQ%40mail.gmail.com
> Author: Andrey Borodin
> Reviewed-by: Alexander Korotkov
In 005_timeouts.pl, I found the following comment.
> # 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.
> $psql_session->{run}->finish;
I'm not sure if "it can get to pump already closed." makes sense. I
guess that it means "the command can get to be pumped (or "can be
sent") to the session already closed" or something similar?
> # 2. Test of the sidle in transaction timeout
s/sidle/idle/ ?
> # Wait until the backend is in the timeout injection point.
I'm not sure, but it seems that "is in" meant "passes" or something like?
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: pgsql: Add TAP tests for timeouts
@ 2024-03-15 05:28 Michael Paquier <michael@paquier.xyz>
parent: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
0 siblings, 1 reply; 17+ messages in thread
From: Michael Paquier @ 2024-03-15 05:28 UTC (permalink / raw)
To: Kyotaro Horiguchi <horikyota.ntt@gmail.com>; +Cc: akorotkov@postgresql.org; pgsql-committers@lists.postgresql.org
On Fri, Mar 15, 2024 at 10:42:35AM +0900, Kyotaro Horiguchi wrote:
> In 005_timeouts.pl, I found the following comment.
Note also that the test is not stable, one of my machines with
injection points enabled has complained twice in its last three runs:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=hachi&dt=2024-03-14%2015%3A05%3A04
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=hachi&dt=2024-03-15%2003%3A21%3A15
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: pgsql: Add TAP tests for timeouts
@ 2024-03-15 06:20 Andrey M. Borodin <x4mmm@yandex-team.ru>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 2 replies; 17+ messages in thread
From: Andrey M. Borodin @ 2024-03-15 06:20 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Kyotaro Horiguchi <horikyota.ntt@gmail.com>; akorotkov@postgresql.org; pgsql-committers@lists.postgresql.org
Kyotaro, thanks for your corrections. I agree that wordings should be improved. But let’s deal with failures first.
> On 15 Mar 2024, at 10:28, Michael Paquier <michael@paquier.xyz> wrote:
>
> On Fri, Mar 15, 2024 at 10:42:35AM +0900, Kyotaro Horiguchi wrote:
>> In 005_timeouts.pl, I found the following comment.
>
> Note also that the test is not stable, one of my machines with
> injection points enabled has complained twice in its last three runs:
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=hachi&dt=2024-03-14%2015%3A05%3A04
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=hachi&dt=2024-03-15%2003%3A21%3A15
The failure is
t/005_timeouts.pl (Wstat: 65280 Tests: 0 Failed: 0)
Non-zero exit status: 255
Parse errors: No plan found in TAP output
Files=5, Tests=68, 185 wallclock secs ( 0.03 usr 0.00 sys + 0.86 cusr 0.80 csys = 1.69 CPU)
Result: FAIL
The failure seems to be Perl-related.
As far as I can see I’ve done everything akin to 041_checkpoint_at_promote.pl. On batta this test pass, but hachi seems to be unhappy with this test.
And hachi sometimes pass this test too [0].
I’ll look more on this. Do I understand right that we have only 2 buildfarm members with injection points?
Best regards, Andrey Borodin.
[0] https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=hachi&dt=2024-03-14%2022%3A02%3A41...
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: pgsql: Add TAP tests for timeouts
@ 2024-03-15 06:54 Andrey M. Borodin <x4mmm@yandex-team.ru>
parent: Andrey M. Borodin <x4mmm@yandex-team.ru>
1 sibling, 0 replies; 17+ messages in thread
From: Andrey M. Borodin @ 2024-03-15 06:54 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Kyotaro Horiguchi <horikyota.ntt@gmail.com>; akorotkov@postgresql.org; pgsql-committers@lists.postgresql.org
> On 15 Mar 2024, at 11:20, Andrey M. Borodin <x4mmm@yandex-team.ru> wrote:
>
> The failure seems to be Perl-related.
It’s not Perl-related. This is how wait_for_log() times out.
timed out waiting for match: terminating connection due to transaction timeout at t/005_timeouts.pl line 58.
But it did not even wait for a 100ms…
2024-03-15 03:32:26.492 UTC [1405044:4] 005_timeouts.pl LOG: statement: SELECT injection_points_wakeup('transaction-timeout’);
///// here we start to wait.
2024-03-15 03:32:26.492 UTC [1405044:5] 005_timeouts.pl LOG: disconnection: session time: 0:00:00.002 user=admin database=postgres host=[local]
2024-03-15 03:35:26.623 UTC [1405009:4] LOG: received immediate shutdown request
Best regards, Andrey Borodin.
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: pgsql: Add TAP tests for timeouts
@ 2024-03-15 07:44 Michael Paquier <michael@paquier.xyz>
parent: Andrey M. Borodin <x4mmm@yandex-team.ru>
1 sibling, 1 reply; 17+ messages in thread
From: Michael Paquier @ 2024-03-15 07:44 UTC (permalink / raw)
To: Andrey M. Borodin <x4mmm@yandex-team.ru>; +Cc: Kyotaro Horiguchi <horikyota.ntt@gmail.com>; akorotkov@postgresql.org; pgsql-committers@lists.postgresql.org
On Fri, Mar 15, 2024 at 11:20:49AM +0500, Andrey M. Borodin wrote:
> And hachi sometimes pass this test too [0].
>
> I’ll look more on this. Do I understand right that we have only 2 buildfarm members with injection points?
I have added --enable-injection-points to three of them: hachi,
gokiburi and batta. The switch is also enabled in the CI by defaultm
where you may be able to capture a bit more information than the
buildfarm.
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: pgsql: Add TAP tests for timeouts
@ 2024-03-15 10:44 Alexander Korotkov <aekorotkov@gmail.com>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 17+ messages in thread
From: Alexander Korotkov @ 2024-03-15 10:44 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Andrey M. Borodin <x4mmm@yandex-team.ru>; Kyotaro Horiguchi <horikyota.ntt@gmail.com>; akorotkov@postgresql.org; pgsql-committers@lists.postgresql.org
On Fri, Mar 15, 2024 at 9:44 AM Michael Paquier <michael@paquier.xyz> wrote:
> On Fri, Mar 15, 2024 at 11:20:49AM +0500, Andrey M. Borodin wrote:
> > And hachi sometimes pass this test too [0].
> >
> > I’ll look more on this. Do I understand right that we have only 2
buildfarm members with injection points?
>
> I have added --enable-injection-points to three of them: hachi,
> gokiburi and batta. The switch is also enabled in the CI by defaultm
> where you may be able to capture a bit more information than the
> buildfarm.
I managed to reproduce the issue locally by setting the following extra
config via TEMP_CONFIG:
log_line_prefix = '%m [%p:%l] %q%a '
log_connections = 'true'
log_disconnections = 'true'
log_checkpoints = 'true'
log_statement = 'all'
wal_compression = 'on'
debug_parallel_query = regress
wal_compression = lz4
default_toast_compression = lz4
The backtrace is as follows. We loop in the interrupt checking, given that
the injection point handler checks for interrupts internally. I propose to
unset the timeout flag before the injection point (see the attached patch).
* thread #1, name = 'postgres', stop reason = signal SIGSTOP
* frame #0: 0x00007f3756f289e7 libc.so.6`epoll_wait(epfd=9,
events=0x0000556a302cabc8, maxevents=1, timeout=-1) at epoll_wait.c:30:10
frame #1: 0x0000556a2fc41aab postgres`WaitEventSetWait at latch.c:1570:7
frame #2: 0x0000556a2fc41e95 postgres`WaitLatch(latch=<unavailable>,
wakeEvents=<unavailable>, timeout=<unavailable>, wait_event_info=117440513)
at latch.c:538:6
frame #3: 0x0000556a2fc4f611
postgres`ConditionVariableTimedSleep(cv=0x00007f3757b0e224, timeout=-1,
wait_event_info=117440513) at condition_variable.c:163:10
frame #4: 0x00007f3757b10628 injection_points.so`injection_wait at
injection_points.c:153:3
frame #5: 0x0000556a2fdd4f54 postgres`InjectionPointRun at
injection_point.c:313:2
frame #6: 0x0000556a2fc6a8b3 postgres`ProcessInterrupts.part.0 at
postgres.c:3430:4
frame #7: 0x0000556a2fc4f70b
postgres`ConditionVariableTimedSleep(cv=0x00007f3757b0e224, timeout=-1,
wait_event_info=117440513) at condition_variable.c:196:3
frame #8: 0x00007f3757b10628 injection_points.so`injection_wait at
injection_points.c:153:3
frame #9: 0x0000556a2fdd4f54 postgres`InjectionPointRun at
injection_point.c:313:2
frame #10: 0x0000556a2fc6a8b3 postgres`ProcessInterrupts.part.0 at
postgres.c:3430:4
frame #11: 0x0000556a2fc4f70b
postgres`ConditionVariableTimedSleep(cv=0x00007f3757b0e224, timeout=-1,
wait_event_info=117440513) at condition_variable.c:196:3
frame #12: 0x00007f3757b10628 injection_points.so`injection_wait at
injection_points.c:153:3
frame #13: 0x0000556a2fdd4f54 postgres`InjectionPointRun at
injection_point.c:313:2
frame #14: 0x0000556a2fc6a8b3 postgres`ProcessInterrupts.part.0 at
postgres.c:3430:4
frame #15: 0x0000556a2fabe63d postgres`ExecGather at nodeGather.c:315:3
frame #16: 0x0000556a2fabe574 postgres`ExecGather at nodeGather.c:269:10
frame #17: 0x0000556a2fabe549
postgres`ExecGather(pstate=0x0000556a303d8138) at nodeGather.c:222:9
frame #18: 0x0000556a2faa21da postgres`standard_ExecutorRun at
executor.h:274:9
frame #19: 0x0000556a2fc6f3cf postgres`PortalRunSelect at pquery.c:924:4
frame #20: 0x0000556a2fc70b7e postgres`PortalRun at pquery.c:768:18
frame #21: 0x0000556a2fc6c996 postgres`exec_simple_query at
postgres.c:1274:10
frame #22: 0x0000556a2fc6e378 postgres`PostgresMain at postgres.c:4682:7
frame #23: 0x0000556a2fbcefa0 postgres`ServerLoop.isra.0 at
postmaster.c:4452:2
frame #24: 0x0000556a2fbcff8b postgres`PostmasterMain at
postmaster.c:1478:11
frame #25: 0x0000556a2f8cb5a7 postgres`main(argc=4,
argv=0x0000556a302ca340) at main.c:197:3
frame #26: 0x00007f3756e28150
libc.so.6`__libc_start_call_main(main=(postgres`main at main.c:59:1),
argc=4, argv=0x00007ffffc17e678) at libc_start_call_main.h:58:16
frame #27: 0x00007f3756e28209
libc.so.6`__libc_start_main_impl(main=(postgres`main at main.c:59:1),
argc=4, argv=0x00007ffffc17e678, init=<unavailable>, fini=<unavailable>,
rtld_fini=<unavailable>, stack_end=0x00007ffffc17e668) at libc-start.c:360:3
frame #28: 0x0000556a2f8cbb75 postgres`_start + 37
------
Regards,
Alexander Korotkov
Attachments:
transaction_timeout_patch_fix.patch (application/octet-stream, 1.8K) download
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 7ac623019bc..fd4199a0983 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;
}
/*
* Re: pgsql: Add TAP tests for timeouts
@ 2024-03-15 11:27 Andrey M. Borodin <x4mmm@yandex-team.ru>
parent: Alexander Korotkov <aekorotkov@gmail.com>
0 siblings, 1 reply; 17+ messages in thread
From: Andrey M. Borodin @ 2024-03-15 11:27 UTC (permalink / raw)
To: Alexander Korotkov <aekorotkov@gmail.com>; +Cc: Michael Paquier <michael@paquier.xyz>; Kyotaro Horiguchi <horikyota.ntt@gmail.com>; akorotkov@postgresql.org; pgsql-committers@lists.postgresql.org
> On 15 Mar 2024, at 15:44, Alexander Korotkov <aekorotkov@gmail.com> wrote:
>
> We loop in the interrupt checking, given that the injection point handler checks for interrupts internally. I propose to unset the timeout flag before the injection point (see the attached patch).
Oh, cool.
As far as I understand, this is only necessary for the test with injection point.
So, maybe unset it only when injection points are enabled? Something like this is already used in GIN.
#ifdef USE_INJECTION_POINTS
....
#endif
Best regards, Andrey Borodin.
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: pgsql: Add TAP tests for timeouts
@ 2024-03-15 11:30 Alexander Korotkov <aekorotkov@gmail.com>
parent: Andrey M. Borodin <x4mmm@yandex-team.ru>
0 siblings, 1 reply; 17+ messages in thread
From: Alexander Korotkov @ 2024-03-15 11:30 UTC (permalink / raw)
To: Andrey M. Borodin <x4mmm@yandex-team.ru>; +Cc: Michael Paquier <michael@paquier.xyz>; Kyotaro Horiguchi <horikyota.ntt@gmail.com>; akorotkov@postgresql.org; pgsql-committers@lists.postgresql.org
On Fri, Mar 15, 2024 at 1:27 PM Andrey M. Borodin <x4mmm@yandex-team.ru> wrote:
>
> > On 15 Mar 2024, at 15:44, Alexander Korotkov <aekorotkov@gmail.com> wrote:
> >
> > We loop in the interrupt checking, given that the injection point handler checks for interrupts internally. I propose to unset the timeout flag before the injection point (see the attached patch).
>
> Oh, cool.
> As far as I understand, this is only necessary for the test with injection point.
> So, maybe unset it only when injection points are enabled? Something like this is already used in GIN.
>
> #ifdef USE_INJECTION_POINTS
> ....
> #endif
Maybe, but do you see any negative side effects of the unconditionally
unset of flags? If not, I would prefer to keep the code simple.
------
Regards,
Alexander Korotkov
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: pgsql: Add TAP tests for timeouts
@ 2024-03-15 12:25 Andrey M. Borodin <x4mmm@yandex-team.ru>
parent: Alexander Korotkov <aekorotkov@gmail.com>
0 siblings, 1 reply; 17+ messages in thread
From: Andrey M. Borodin @ 2024-03-15 12:25 UTC (permalink / raw)
To: Alexander Korotkov <aekorotkov@gmail.com>; +Cc: Michael Paquier <michael@paquier.xyz>; Kyotaro Horiguchi <horikyota.ntt@gmail.com>; akorotkov@postgresql.org; pgsql-committers@lists.postgresql.org
> 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.c
index 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.pl
index 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)
=
* Re: pgsql: Add TAP tests for timeouts
@ 2024-03-15 12:54 Alexander Korotkov <aekorotkov@gmail.com>
parent: Andrey M. Borodin <x4mmm@yandex-team.ru>
0 siblings, 0 replies; 17+ messages in thread
From: Alexander Korotkov @ 2024-03-15 12:54 UTC (permalink / raw)
To: Andrey M. Borodin <x4mmm@yandex-team.ru>; +Cc: Michael Paquier <michael@paquier.xyz>; Kyotaro Horiguchi <horikyota.ntt@gmail.com>; akorotkov@postgresql.org; pgsql-committers@lists.postgresql.org
On Fri, Mar 15, 2024 at 2:25 PM Andrey M. Borodin <x4mmm@yandex-team.ru> wrote:
> > 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.
Thank you, I've pushed this, but split into two commits for bugfix and
for improved wordings.
> Maybe it worth to wait a little to see if some other failures will pop up?
I prefer to push now because we're quite confident this fix will
improve the situation. It should increase the chances of catching
other bugs if any, because buildfarm which was failing on this bug
will go ahead and have a chance to spot more.
------
Regards,
Alexander Korotkov
^ permalink raw reply [nested|flat] 17+ messages in thread
end of thread, other threads:[~2024-03-15 12:54 UTC | newest]
Thread overview: 17+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2009-07-10 00:32 pgsql: Fix xslt_process() to ensure that it inserts a NULL terminator Tom Lane <tgl@postgresql.org>
2009-07-10 00:32 pgsql: Fix xslt_process() to ensure that it inserts a NULL terminator Tom Lane <tgl@postgresql.org>
2009-07-10 00:32 pgsql: Fix xslt_process() to ensure that it inserts a NULL terminator Tom Lane <tgl@postgresql.org>
2009-07-10 00:32 pgsql: Fix xslt_process() to ensure that it inserts a NULL terminator Tom Lane <tgl@postgresql.org>
2009-07-10 00:32 pgsql: Fix xslt_process() to ensure that it inserts a NULL terminator Tom Lane <tgl@postgresql.org>
2009-07-10 00:32 pgsql: Fix xslt_process() to ensure that it inserts a NULL terminator Tom Lane <tgl@postgresql.org>
2024-03-14 11:25 pgsql: Add TAP tests for timeouts Alexander Korotkov <akorotkov@postgresql.org>
2024-03-15 01:42 ` Re: pgsql: Add TAP tests for timeouts Kyotaro Horiguchi <horikyota.ntt@gmail.com>
2024-03-15 05:28 ` Re: pgsql: Add TAP tests for timeouts Michael Paquier <michael@paquier.xyz>
2024-03-15 06:20 ` Re: pgsql: Add TAP tests for timeouts Andrey M. Borodin <x4mmm@yandex-team.ru>
2024-03-15 06:54 ` Re: pgsql: Add TAP tests for timeouts Andrey M. Borodin <x4mmm@yandex-team.ru>
2024-03-15 07:44 ` Re: pgsql: Add TAP tests for timeouts Michael Paquier <michael@paquier.xyz>
2024-03-15 10:44 ` Re: pgsql: Add TAP tests for timeouts Alexander Korotkov <aekorotkov@gmail.com>
2024-03-15 11:27 ` Re: pgsql: Add TAP tests for timeouts Andrey M. Borodin <x4mmm@yandex-team.ru>
2024-03-15 11:30 ` Re: pgsql: Add TAP tests for timeouts Alexander Korotkov <aekorotkov@gmail.com>
2024-03-15 12:25 ` Re: pgsql: Add TAP tests for timeouts Andrey M. Borodin <x4mmm@yandex-team.ru>
2024-03-15 12:54 ` Re: pgsql: Add TAP tests for timeouts Alexander Korotkov <aekorotkov@gmail.com>
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