pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feed004_timeline_switch TAP test may fail
5+ messages / 3 participants
[nested] [flat]
* 004_timeline_switch TAP test may fail
@ 2026-06-16 08:01 Sergey Tatarintsev <s.tatarintsev@postgrespro.ru>
2026-06-16 10:51 ` Re: 004_timeline_switch TAP test may fail Ewan Young <kdbase.hack@gmail.com>
2026-06-16 23:23 ` Re: 004_timeline_switch TAP test may fail Michael Paquier <michael@paquier.xyz>
0 siblings, 2 replies; 5+ messages in thread
From: Sergey Tatarintsev @ 2026-06-16 08:01 UTC (permalink / raw)
To: pgsql-hackers <pgsql-hackers@lists.postgresql.org>
Hi hackers!
I found that after commit 7185eddf0522b3146ed1ff6e063e8e129e77c706 we
got little omission
in TAP test 004_timeline_switch:
...
my $node_standby_1 = PostgreSQL::Test::Cluster->new('standby_1');
...
$node_primary->stop;
There is no guarantee that standby_1 and standby_2 was successfully
connected to primary and start
streaming before primary stopped.
I think we must ensure that primary knows about standby_1 and standby_2
--
With best regards,
Sergey Tatarintsev,
PostgresPro
Attachments:
[text/x-patch] 0001-Fix-004_timeline_switch-TAP-test-wait-for-standbys-s.patch (1015B, ../../fea4190e-f8b5-4432-a52d-bcbee5f34366@postgrespro.ru/2-0001-Fix-004_timeline_switch-TAP-test-wait-for-standbys-s.patch)
download | inline diff:
From cede62e8da1441f0ed2abf9c23f7073deff6d838 Mon Sep 17 00:00:00 2001
From: Sergey Tatarintsev <s.tatarintsev@postgrespro.ru>
Date: Tue, 16 Jun 2026 11:57:39 +0700
Subject: [PATCH] Fix 004_timeline_switch TAP test: wait for standbys starts
before primary stops
---
src/test/recovery/t/004_timeline_switch.pl | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/src/test/recovery/t/004_timeline_switch.pl b/src/test/recovery/t/004_timeline_switch.pl
index e0b3851927c..2c432b50e54 100644
--- a/src/test/recovery/t/004_timeline_switch.pl
+++ b/src/test/recovery/t/004_timeline_switch.pl
@@ -30,6 +30,10 @@ $node_standby_2->init_from_backup($node_primary, $backup_name,
has_streaming => 1);
$node_standby_2->start;
+# Wait for standby_1 and standby_2 starts
+$node_primary->poll_query_until('postgres',
+ "SELECT count(1) = 2 FROM pg_stat_replication");
+
# Create some content on primary
$node_primary->safe_psql('postgres',
"CREATE TABLE tab_int AS SELECT generate_series(1,1000) AS a");
--
2.43.0
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: 004_timeline_switch TAP test may fail
2026-06-16 08:01 004_timeline_switch TAP test may fail Sergey Tatarintsev <s.tatarintsev@postgrespro.ru>
@ 2026-06-16 10:51 ` Ewan Young <kdbase.hack@gmail.com>
2026-06-16 23:31 ` Re: 004_timeline_switch TAP test may fail Michael Paquier <michael@paquier.xyz>
2026-06-17 00:53 ` Re: 004_timeline_switch TAP test may fail Sergey Tatarintsev <s.tatarintsev@postgrespro.ru>
1 sibling, 2 replies; 5+ messages in thread
From: Ewan Young @ 2026-06-16 10:51 UTC (permalink / raw)
To: Sergey Tatarintsev <s.tatarintsev@postgrespro.ru>; +Cc: pgsql-hackers <pgsql-hackers@lists.postgresql.org>
Hi Sergey,
Thanks for the report and patch. I think the analysis is right, and the
fix is in the right place.
The gap traces back to commit 7185eddf, which deliberately dropped the
wait_for_catchup() and switched the primary from teardown_node() to a
clean stop(), on the grounds that a clean stop flushes all WAL to both
standbys before exiting. That's true, but only for standbys whose
walsender is *connected* at shutdown time -- and ->start() only waits
for the postmaster to accept connections, not for the standby's
walreceiver to have connected back to the primary. So if a standby
hasn't connected yet when the primary stops, the clean-shutdown flush
skips it, and we're back to the exact "standbys received different
amounts of WAL -> timeline fork on reconnect" failure that 7185eddf was
meant to fix.
Polling pg_stat_replication until both walsenders are present closes
that hole: it re-establishes the precondition the clean-stop design
silently assumed. And connection is enough here -- the walsender
shutdown path sends all WAL up to the shutdown checkpoint regardless of
catchup state -- so there's no need to additionally check
state = 'streaming'.
One small thing: the rest of this file uses count(*), so I'd write count(*) = 2
rather than count(1) = 2 just for local consistency. And the comment reads a
little better as something like "Wait until both standbys have
connected to the primary",
since by this point they've already started -- what we're waiting for is the
connection.
Regards,
Ewan
On Tue, Jun 16, 2026 at 4:01 PM Sergey Tatarintsev
<s.tatarintsev@postgrespro.ru> wrote:
>
> Hi hackers!
>
> I found that after commit 7185eddf0522b3146ed1ff6e063e8e129e77c706 we
> got little omission
> in TAP test 004_timeline_switch:
> ...
> my $node_standby_1 = PostgreSQL::Test::Cluster->new('standby_1');
> ...
> $node_primary->stop;
>
> There is no guarantee that standby_1 and standby_2 was successfully
> connected to primary and start
> streaming before primary stopped.
>
> I think we must ensure that primary knows about standby_1 and standby_2
>
> --
> With best regards,
> Sergey Tatarintsev,
> PostgresPro
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: 004_timeline_switch TAP test may fail
2026-06-16 08:01 004_timeline_switch TAP test may fail Sergey Tatarintsev <s.tatarintsev@postgrespro.ru>
2026-06-16 10:51 ` Re: 004_timeline_switch TAP test may fail Ewan Young <kdbase.hack@gmail.com>
@ 2026-06-16 23:31 ` Michael Paquier <michael@paquier.xyz>
1 sibling, 0 replies; 5+ messages in thread
From: Michael Paquier @ 2026-06-16 23:31 UTC (permalink / raw)
To: Ewan Young <kdbase.hack@gmail.com>; +Cc: Sergey Tatarintsev <s.tatarintsev@postgrespro.ru>; pgsql-hackers <pgsql-hackers@lists.postgresql.org>
On Tue, Jun 16, 2026 at 06:51:48PM +0800, Ewan Young wrote:
> One small thing: the rest of this file uses count(*), so I'd write count(*) = 2
> rather than count(1) = 2 just for local consistency. And the comment reads a
> little better as something like "Wait until both standbys have
> connected to the primary",
> since by this point they've already started -- what we're waiting for is the
> connection.
Both queries work the same for this purpose, so any of them is fine.
The comment needs to be refined a bit, though.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (832B, ../../ajHcy2e-xuba6fVU@paquier.xyz/2-signature.asc)
download
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: 004_timeline_switch TAP test may fail
2026-06-16 08:01 004_timeline_switch TAP test may fail Sergey Tatarintsev <s.tatarintsev@postgrespro.ru>
2026-06-16 10:51 ` Re: 004_timeline_switch TAP test may fail Ewan Young <kdbase.hack@gmail.com>
@ 2026-06-17 00:53 ` Sergey Tatarintsev <s.tatarintsev@postgrespro.ru>
1 sibling, 0 replies; 5+ messages in thread
From: Sergey Tatarintsev @ 2026-06-17 00:53 UTC (permalink / raw)
To: Ewan Young <kdbase.hack@gmail.com>; +Cc: pgsql-hackers <pgsql-hackers@lists.postgresql.org>
Hi!
Thanks for review!
v2 patch attached
comment changed, count(1) replaced with count(*).
> Hi Sergey,
>
> Thanks for the report and patch. I think the analysis is right, and the
> fix is in the right place.
>
> The gap traces back to commit 7185eddf, which deliberately dropped the
> wait_for_catchup() and switched the primary from teardown_node() to a
> clean stop(), on the grounds that a clean stop flushes all WAL to both
> standbys before exiting. That's true, but only for standbys whose
> walsender is *connected* at shutdown time -- and ->start() only waits
> for the postmaster to accept connections, not for the standby's
> walreceiver to have connected back to the primary. So if a standby
> hasn't connected yet when the primary stops, the clean-shutdown flush
> skips it, and we're back to the exact "standbys received different
> amounts of WAL -> timeline fork on reconnect" failure that 7185eddf was
> meant to fix.
>
> Polling pg_stat_replication until both walsenders are present closes
> that hole: it re-establishes the precondition the clean-stop design
> silently assumed. And connection is enough here -- the walsender
> shutdown path sends all WAL up to the shutdown checkpoint regardless of
> catchup state -- so there's no need to additionally check
> state = 'streaming'.
>
> One small thing: the rest of this file uses count(*), so I'd write count(*) = 2
> rather than count(1) = 2 just for local consistency. And the comment reads a
> little better as something like "Wait until both standbys have
> connected to the primary",
> since by this point they've already started -- what we're waiting for is the
> connection.
>
> Regards,
> Ewan
>
> On Tue, Jun 16, 2026 at 4:01 PM Sergey Tatarintsev
> <s.tatarintsev@postgrespro.ru> wrote:
>> Hi hackers!
>>
>> I found that after commit 7185eddf0522b3146ed1ff6e063e8e129e77c706 we
>> got little omission
>> in TAP test 004_timeline_switch:
>> ...
>> my $node_standby_1 = PostgreSQL::Test::Cluster->new('standby_1');
>> ...
>> $node_primary->stop;
>>
>> There is no guarantee that standby_1 and standby_2 was successfully
>> connected to primary and start
>> streaming before primary stopped.
>>
>> I think we must ensure that primary knows about standby_1 and standby_2
>>
>> --
>> With best regards,
>> Sergey Tatarintsev,
>> PostgresPro
--
With best regards,
Sergey Tatarintsev,
PostgresPro
Attachments:
[text/x-patch] v2-0001-Fix-004_timeline_switch-TAP-test-wait-for-standbys-s.patch (1.0K, ../../f3ce2134-d72b-489b-aba5-f8c012c7708e@postgrespro.ru/2-v2-0001-Fix-004_timeline_switch-TAP-test-wait-for-standbys-s.patch)
download | inline diff:
From dd449396060c71c34bcb03e5c1c5de0cb0868da0 Mon Sep 17 00:00:00 2001
From: Sergey Tatarintsev <s.tatarintsev@postgrespro.ru>
Date: Tue, 16 Jun 2026 11:57:39 +0700
Subject: [PATCH] Fix 004_timeline_switch TAP test: wait for standbys starts
before primary stops
---
src/test/recovery/t/004_timeline_switch.pl | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/src/test/recovery/t/004_timeline_switch.pl b/src/test/recovery/t/004_timeline_switch.pl
index e0b3851927c..c87f079cef8 100644
--- a/src/test/recovery/t/004_timeline_switch.pl
+++ b/src/test/recovery/t/004_timeline_switch.pl
@@ -30,6 +30,10 @@ $node_standby_2->init_from_backup($node_primary, $backup_name,
has_streaming => 1);
$node_standby_2->start;
+# Wait until both standbys have connected to the primary
+$node_primary->poll_query_until('postgres',
+ "SELECT count(*) = 2 FROM pg_stat_replication");
+
# Create some content on primary
$node_primary->safe_psql('postgres',
"CREATE TABLE tab_int AS SELECT generate_series(1,1000) AS a");
--
2.43.0
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: 004_timeline_switch TAP test may fail
2026-06-16 08:01 004_timeline_switch TAP test may fail Sergey Tatarintsev <s.tatarintsev@postgrespro.ru>
@ 2026-06-16 23:23 ` Michael Paquier <michael@paquier.xyz>
1 sibling, 0 replies; 5+ messages in thread
From: Michael Paquier @ 2026-06-16 23:23 UTC (permalink / raw)
To: Sergey Tatarintsev <s.tatarintsev@postgrespro.ru>; +Cc: pgsql-hackers <pgsql-hackers@lists.postgresql.org>
On Tue, Jun 16, 2026 at 03:01:15PM +0700, Sergey Tatarintsev wrote:
> I found that after commit 7185eddf0522b3146ed1ff6e063e8e129e77c706 we got
> little omission
> in TAP test 004_timeline_switch:
> ...
> my $node_standby_1 = PostgreSQL::Test::Cluster->new('standby_1');
> ...
> $node_primary->stop;
7185eddf0522 rings a bell.
> There is no guarantee that standby_1 and standby_2 was successfully
> connected to primary and start
> streaming before primary stopped.
Indeed. I assume that adding a conditional sleep that prevents the
startup process of standby1 or standby2 to connect to their primary
once they have reached a consistent state, before they are able to
replay the inserts of tab_int and before the primary is stopped would
be enough to make the test go rogue, with one or more standbys not
getting the records we want.
If standby2 gets ahead of standby1, we would fail the initial
poll_query_until() done after standby2 attempts to reconnect standby1,
failing the test on timeout. If standby1 gets ahead of standby2,
things would work; there is a wait step for standby2 to catch up with
standby1. So only the first pattern is problematic, not the second.
It does not seem like the buildfarm has complained on this one
(failures in latest 30 days for recoveryCheck report 026 and 035),
neither does the CI:
https://cfbot.cputube.org/highlights/all.html
--
Michael
Attachments:
[application/pgp-signature] signature.asc (832B, ../../ajHa3G4k1cCH2kQ7@paquier.xyz/2-signature.asc)
download
^ permalink raw reply [nested|flat] 5+ messages in thread
end of thread, other threads:[~2026-06-17 00:53 UTC | newest]
Thread overview: 5+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2026-06-16 08:01 004_timeline_switch TAP test may fail Sergey Tatarintsev <s.tatarintsev@postgrespro.ru>
2026-06-16 10:51 ` Ewan Young <kdbase.hack@gmail.com>
2026-06-16 23:31 ` Michael Paquier <michael@paquier.xyz>
2026-06-17 00:53 ` Sergey Tatarintsev <s.tatarintsev@postgrespro.ru>
2026-06-16 23:23 ` Michael Paquier <michael@paquier.xyz>
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