Timing-sensitive case in src/test/recovery TAP tests
I've been experimenting with a change to pg_ctl, which I'll post
separately, to reduce its reaction time so that it reports success
more quickly after a wait for postmaster start/stop. I found one
case in "make check-world" that got a failure when I reduced the
reaction time to ~1ms. That's the very last test in 001_stream_rep.pl,
"cascaded slot xmin reset after startup with hs feedback reset", and
the cause appears to be that it's not allowing any delay time for a
replication slot's state to update after a postmaster restart.
This seems worth fixing independently of any possible code changes,
because it shows that this test could fail on a slow or overloaded
machine. I couldn't find any instances of such a failure in the
buildfarm archives, but that may have a lot to do with the fact that
owners of slow buildfarm animals are (mostly?) not running this test.
Some experimentation says that the minimum delay needed to make it
work reliably on my workstation is about 100ms, so a simple patch
along the lines of the attached might be good enough. I find this
approach conceptually dissatisfying, though, since it's still
potentially vulnerable to the failure under sufficient load.
I wonder if there is an easy way to improve that ... maybe convert
to something involving poll_query_until?
regards, tom lane
Attachments:
add-delay-in-time-sensitive-test.patchtext/x-diff; charset=us-ascii; name=add-delay-in-time-sensitive-test.patchDownload
diff --git a/src/test/recovery/t/001_stream_rep.pl b/src/test/recovery/t/001_stream_rep.pl
index 266d27c..8d6edd2 100644
*** a/src/test/recovery/t/001_stream_rep.pl
--- b/src/test/recovery/t/001_stream_rep.pl
*************** isnt($xmin, '', 'cascaded slot xmin non-
*** 265,270 ****
--- 265,272 ----
# Xmin from a previous run should be cleared on startup.
$node_standby_2->start;
+ sleep(1); # need some delay before interrogating slot xmin
+
($xmin, $catalog_xmin) = get_slot_xmins($node_standby_1, $slotname_2);
is($xmin, '',
'cascaded slot xmin reset after startup with hs feedback reset');
On 26 June 2017 at 05:10, Tom Lane <tgl@sss.pgh.pa.us> wrote:
I've been experimenting with a change to pg_ctl, which I'll post
separately, to reduce its reaction time so that it reports success
more quickly after a wait for postmaster start/stop. I found one
case in "make check-world" that got a failure when I reduced the
reaction time to ~1ms. That's the very last test in 001_stream_rep.pl,
"cascaded slot xmin reset after startup with hs feedback reset", and
the cause appears to be that it's not allowing any delay time for a
replication slot's state to update after a postmaster restart.This seems worth fixing independently of any possible code changes,
because it shows that this test could fail on a slow or overloaded
machine. I couldn't find any instances of such a failure in the
buildfarm archives, but that may have a lot to do with the fact that
owners of slow buildfarm animals are (mostly?) not running this test.Some experimentation says that the minimum delay needed to make it
work reliably on my workstation is about 100ms, so a simple patch
along the lines of the attached might be good enough. I find this
approach conceptually dissatisfying, though, since it's still
potentially vulnerable to the failure under sufficient load.
I wonder if there is an easy way to improve that ... maybe convert
to something involving poll_query_until?
This should do the trick:
$node_standby_1->poll_query_until('postgres', q[SELECT xmin IS NULL
from pg_replication_slots WHERE slot_name = '] . $slotname_2 . q[']);
--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Mon, Jun 26, 2017 at 10:48 AM, Craig Ringer <craig@2ndquadrant.com> wrote:
$node_standby_1->poll_query_until('postgres', q[SELECT xmin IS NULL
from pg_replication_slots WHERE slot_name = '] . $slotname_2 . q[']);
+1 for avoiding a sleep call if it is not necessary. Fast platforms
would always pay a cost on that, and slow platforms would wait 1s (or
more!) when polling for the result.
Could it be possible to remove as well the second sleep(2) call in
this test please? This could be replaced by a similar poll using the
query Craig has just given as this needs to wait until the xmon is
cleared. Other tests expect a value so this poll cannot happen in
get_slot_xmins.
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Michael Paquier <michael.paquier@gmail.com> writes:
On Mon, Jun 26, 2017 at 10:48 AM, Craig Ringer <craig@2ndquadrant.com> wrote:
$node_standby_1->poll_query_until('postgres', q[SELECT xmin IS NULL
from pg_replication_slots WHERE slot_name = '] . $slotname_2 . q[']);
+1 for avoiding a sleep call if it is not necessary. Fast platforms
would always pay a cost on that, and slow platforms would wait 1s (or
more!) when polling for the result.
Could it be possible to remove as well the second sleep(2) call in
this test please?
Yes, I'd like to see those fixed sleeps go away too. Want to work
on a concrete patch?
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 26 June 2017 at 10:09, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Michael Paquier <michael.paquier@gmail.com> writes:
On Mon, Jun 26, 2017 at 10:48 AM, Craig Ringer <craig@2ndquadrant.com> wrote:
$node_standby_1->poll_query_until('postgres', q[SELECT xmin IS NULL
from pg_replication_slots WHERE slot_name = '] . $slotname_2 . q[']);+1 for avoiding a sleep call if it is not necessary. Fast platforms
would always pay a cost on that, and slow platforms would wait 1s (or
more!) when polling for the result.Could it be possible to remove as well the second sleep(2) call in
this test please?Yes, I'd like to see those fixed sleeps go away too. Want to work
on a concrete patch?
Attached.
--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
Attachments:
fix-timing-in-tap-001-stream-rep.plapplication/x-perl; name=fix-timing-in-tap-001-stream-rep.plDownload
diff --git a/src/test/recovery/t/001_stream_rep.pl b/src/test/recovery/t/001_stream_rep.pl
index 266d27c..b397e3e 100644
--- a/src/test/recovery/t/001_stream_rep.pl
+++ b/src/test/recovery/t/001_stream_rep.pl
@@ -196,7 +196,7 @@ $node_standby_2->safe_psql('postgres',
'ALTER SYSTEM SET hot_standby_feedback = on;');
$node_standby_2->reload;
replay_check();
-sleep(2);
+$node_master->poll_query_until('postgres', q[SELECT xmin IS NOT NULL AND catalog_xmin IS NULL from pg_replication_slots WHERE slot_name = '] . $slotname_1 . q[']);
($xmin, $catalog_xmin) = get_slot_xmins($node_master, $slotname_1);
isnt($xmin, '', 'non-cascaded slot xmin non-null with hs feedback');
@@ -236,7 +236,7 @@ $node_standby_2->safe_psql('postgres',
'ALTER SYSTEM SET hot_standby_feedback = off;');
$node_standby_2->reload;
replay_check();
-sleep(2);
+$node_master->poll_query_until('postgres', q[SELECT xmin IS NULL AND catalog_xmin IS NULL from pg_replication_slots WHERE slot_name = '] . $slotname_1 . q[']);
($xmin, $catalog_xmin) = get_slot_xmins($node_master, $slotname_1);
is($xmin, '', 'non-cascaded slot xmin null with hs feedback reset');
@@ -265,6 +265,8 @@ isnt($xmin, '', 'cascaded slot xmin non-null with postgres shut down');
# Xmin from a previous run should be cleared on startup.
$node_standby_2->start;
+$node_standby_1->poll_query_until('postgres', q[SELECT xmin IS NULL from pg_replication_slots WHERE slot_name = '] . $slotname_2 . q[']);
+
($xmin, $catalog_xmin) = get_slot_xmins($node_standby_1, $slotname_2);
is($xmin, '',
'cascaded slot xmin reset after startup with hs feedback reset');
On Mon, Jun 26, 2017 at 11:44 AM, Craig Ringer <craig@2ndquadrant.com> wrote:
On 26 June 2017 at 10:09, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Michael Paquier <michael.paquier@gmail.com> writes:
On Mon, Jun 26, 2017 at 10:48 AM, Craig Ringer <craig@2ndquadrant.com> wrote:
$node_standby_1->poll_query_until('postgres', q[SELECT xmin IS NULL
from pg_replication_slots WHERE slot_name = '] . $slotname_2 . q[']);+1 for avoiding a sleep call if it is not necessary. Fast platforms
would always pay a cost on that, and slow platforms would wait 1s (or
more!) when polling for the result.Could it be possible to remove as well the second sleep(2) call in
this test please?Yes, I'd like to see those fixed sleeps go away too. Want to work
on a concrete patch?Attached.
Thanks for the patch.
As long as we are on it, there is this code block in the test:
my ($xmin, $catalog_xmin) = get_slot_xmins($node_master, $slotname_1);
is($xmin, '', 'non-cascaded slot xmin null with no hs_feedback');
is($catalog_xmin, '', 'non-cascaded slot xmin null with no hs_feedback');
($xmin, $catalog_xmin) = get_slot_xmins($node_standby_1, $slotname_2);
is($xmin, '', 'cascaded slot xmin null with no hs_feedback');
is($catalog_xmin, '', 'cascaded slot xmin null with no hs_feedback');
This should be more verbose as the 2nd and 4th test should say
"catalog xmin" instead of xmin.
Also, wouldn't it be better to poll as well node_standby_1's
pg_replication_slot on slotname_2? It would really seem better to make
the nullness check conditional in get_slot_xmins instead. Sorry for
changing opinion here.
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 26 June 2017 at 11:06, Michael Paquier <michael.paquier@gmail.com> wrote:
As long as we are on it, there is this code block in the test:
my ($xmin, $catalog_xmin) = get_slot_xmins($node_master, $slotname_1);
is($xmin, '', 'non-cascaded slot xmin null with no hs_feedback');
is($catalog_xmin, '', 'non-cascaded slot xmin null with no hs_feedback');($xmin, $catalog_xmin) = get_slot_xmins($node_standby_1, $slotname_2);
is($xmin, '', 'cascaded slot xmin null with no hs_feedback');
is($catalog_xmin, '', 'cascaded slot xmin null with no hs_feedback');This should be more verbose as the 2nd and 4th test should say
"catalog xmin" instead of xmin.
Agree, should. Mind posting a revision?
Also, wouldn't it be better to poll as well node_standby_1's
pg_replication_slot on slotname_2? It would really seem better to make
the nullness check conditional in get_slot_xmins instead. Sorry for
changing opinion here.
I'm not sure I understand this.
--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Mon, Jun 26, 2017 at 12:12 PM, Craig Ringer <craig@2ndquadrant.com> wrote:
On 26 June 2017 at 11:06, Michael Paquier <michael.paquier@gmail.com> wrote:
As long as we are on it, there is this code block in the test:
my ($xmin, $catalog_xmin) = get_slot_xmins($node_master, $slotname_1);
is($xmin, '', 'non-cascaded slot xmin null with no hs_feedback');
is($catalog_xmin, '', 'non-cascaded slot xmin null with no hs_feedback');($xmin, $catalog_xmin) = get_slot_xmins($node_standby_1, $slotname_2);
is($xmin, '', 'cascaded slot xmin null with no hs_feedback');
is($catalog_xmin, '', 'cascaded slot xmin null with no hs_feedback');This should be more verbose as the 2nd and 4th test should say
"catalog xmin" instead of xmin.Agree, should. Mind posting a revision?
Sure.
Also, wouldn't it be better to poll as well node_standby_1's
pg_replication_slot on slotname_2? It would really seem better to make
the nullness check conditional in get_slot_xmins instead. Sorry for
changing opinion here.I'm not sure I understand this.
The patch attached may explain that better. Your patch added 3 poll
phases. I think that a 4th is needed. At the same time I have found
cleaner to put the poll calls into a small wrapper.
--
Michael
Attachments:
fix-timing-in-tap-001-stream-rep-v2.pltext/x-perl-script; charset=US-ASCII; name=fix-timing-in-tap-001-stream-rep-v2.plDownload
diff --git a/src/test/recovery/t/001_stream_rep.pl b/src/test/recovery/t/001_stream_rep.pl
index 266d27c8a2..d97db659cb 100644
--- a/src/test/recovery/t/001_stream_rep.pl
+++ b/src/test/recovery/t/001_stream_rep.pl
@@ -146,6 +146,17 @@ $node_standby_2->append_conf('postgresql.conf',
"wal_receiver_status_interval = 1");
$node_standby_2->restart;
+sub wait_slot_xmins_update
+{
+ my ($node, $slot_name, $check_expr) = @_;
+
+ $node->poll_query_until('postgres', qq[
+ SELECT $check_expr
+ FROM pg_replication_slots
+ WHERE slot_name = '$slot_name';
+]);
+}
+
sub get_slot_xmins
{
my ($node, $slotname) = @_;
@@ -156,12 +167,12 @@ sub get_slot_xmins
# There's no hot standby feedback and there are no logical slots on either peer
# so xmin and catalog_xmin should be null on both slots.
my ($xmin, $catalog_xmin) = get_slot_xmins($node_master, $slotname_1);
-is($xmin, '', 'non-cascaded slot xmin null with no hs_feedback');
-is($catalog_xmin, '', 'non-cascaded slot xmin null with no hs_feedback');
+is($xmin, '', 'xmin of non-cascaded slot null with no hs_feedback');
+is($catalog_xmin, '', 'catalog xmin of non-cascaded slot null with no hs_feedback');
($xmin, $catalog_xmin) = get_slot_xmins($node_standby_1, $slotname_2);
-is($xmin, '', 'cascaded slot xmin null with no hs_feedback');
-is($catalog_xmin, '', 'cascaded slot xmin null with no hs_feedback');
+is($xmin, '', 'xmin of cascaded slot null with no hs_feedback');
+is($catalog_xmin, '', 'catalog xmin of cascaded slot null with no hs_feedback');
# Replication still works?
$node_master->safe_psql('postgres', 'CREATE TABLE replayed(val integer);');
@@ -196,15 +207,17 @@ $node_standby_2->safe_psql('postgres',
'ALTER SYSTEM SET hot_standby_feedback = on;');
$node_standby_2->reload;
replay_check();
-sleep(2);
+wait_slot_xmins_update($node_master, $slotname_1,
+ "xmin IS NOT NULL AND catalog_xmin IS NULL");
($xmin, $catalog_xmin) = get_slot_xmins($node_master, $slotname_1);
-isnt($xmin, '', 'non-cascaded slot xmin non-null with hs feedback');
-is($catalog_xmin, '', 'non-cascaded slot xmin still null with hs_feedback');
+isnt($xmin, '', 'xmin of non-cascaded slot non-null with hs feedback');
+is($catalog_xmin, '',
+ 'catalog xmin of non-cascaded slot still null with hs_feedback');
($xmin, $catalog_xmin) = get_slot_xmins($node_standby_1, $slotname_2);
-isnt($xmin, '', 'cascaded slot xmin non-null with hs feedback');
-is($catalog_xmin, '', 'cascaded slot xmin still null with hs_feedback');
+isnt($xmin, '', 'xmin of cascaded slot non-null with hs feedback');
+is($catalog_xmin, '', 'catalog xmin cascaded slot still null with hs_feedback');
note "doing some work to advance xmin";
for my $i (10000 .. 11000)
@@ -216,15 +229,15 @@ $node_master->safe_psql('postgres', 'CHECKPOINT;');
my ($xmin2, $catalog_xmin2) = get_slot_xmins($node_master, $slotname_1);
note "new xmin $xmin2, old xmin $xmin";
-isnt($xmin2, $xmin, 'non-cascaded slot xmin with hs feedback has changed');
+isnt($xmin2, $xmin, 'xmin of non-cascaded slot with hs feedback has changed');
is($catalog_xmin2, '',
- 'non-cascaded slot xmin still null with hs_feedback unchanged');
+ 'catalog xmin of non-cascaded slot still null with hs_feedback unchanged');
($xmin2, $catalog_xmin2) = get_slot_xmins($node_standby_1, $slotname_2);
note "new xmin $xmin2, old xmin $xmin";
-isnt($xmin2, $xmin, 'cascaded slot xmin with hs feedback has changed');
+isnt($xmin2, $xmin, 'xmin of cascaded slot with hs feedback has changed');
is($catalog_xmin2, '',
- 'cascaded slot xmin still null with hs_feedback unchanged');
+ 'catalog xmin of cascaded slot still null with hs_feedback unchanged');
note "disabling hot_standby_feedback";
@@ -236,16 +249,21 @@ $node_standby_2->safe_psql('postgres',
'ALTER SYSTEM SET hot_standby_feedback = off;');
$node_standby_2->reload;
replay_check();
-sleep(2);
+wait_slot_xmins_update($node_master, $slotname_1,
+ "xmin IS NULL AND catalog_xmin IS NULL");
($xmin, $catalog_xmin) = get_slot_xmins($node_master, $slotname_1);
-is($xmin, '', 'non-cascaded slot xmin null with hs feedback reset');
+is($xmin, '', 'xmin of non-cascaded slot null with hs feedback reset');
is($catalog_xmin, '',
- 'non-cascaded slot xmin still null with hs_feedback reset');
+ 'catalog xmin of non-cascaded slot still null with hs_feedback reset');
+
+wait_slot_xmins_update($node_standby_1, $slotname_2,
+ "xmin IS NULL AND catalog_xmin IS NULL");
($xmin, $catalog_xmin) = get_slot_xmins($node_standby_1, $slotname_2);
-is($xmin, '', 'cascaded slot xmin null with hs feedback reset');
-is($catalog_xmin, '', 'cascaded slot xmin still null with hs_feedback reset');
+is($xmin, '', 'xmin of cascaded slot null with hs feedback reset');
+is($catalog_xmin, '',
+ 'catalog xmin of cascaded slot still null with hs_feedback reset');
note "re-enabling hot_standby_feedback and disabling while stopped";
$node_standby_2->safe_psql('postgres',
@@ -260,11 +278,12 @@ $node_standby_2->safe_psql('postgres',
$node_standby_2->stop;
($xmin, $catalog_xmin) = get_slot_xmins($node_standby_1, $slotname_2);
-isnt($xmin, '', 'cascaded slot xmin non-null with postgres shut down');
+isnt($xmin, '', 'xmin of cascaded slot non-null with postgres shut down');
# Xmin from a previous run should be cleared on startup.
$node_standby_2->start;
+wait_slot_xmins_update($node_standby_1, $slotname_2, "xmin IS NULL");
($xmin, $catalog_xmin) = get_slot_xmins($node_standby_1, $slotname_2);
is($xmin, '',
- 'cascaded slot xmin reset after startup with hs feedback reset');
+ 'xmin of cascaded slot reset after startup with hs feedback reset');
Michael Paquier <michael.paquier@gmail.com> writes:
On Mon, Jun 26, 2017 at 12:12 PM, Craig Ringer <craig@2ndquadrant.com> wrote:
I'm not sure I understand this.
The patch attached may explain that better. Your patch added 3 poll
phases. I think that a 4th is needed. At the same time I have found
cleaner to put the poll calls into a small wrapper.
Looks good as far as it goes, but I wonder whether any of the other
get_slot_xmins calls need polling too. Don't feel a need to add such
calls until someone exhibits a failure there, but I won't be very
surprised if someone does.
Anyway, pushed this patch with minor editing. Thanks!
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, Jun 27, 2017 at 3:44 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Looks good as far as it goes, but I wonder whether any of the other
get_slot_xmins calls need polling too. Don't feel a need to add such
calls until someone exhibits a failure there, but I won't be very
surprised if someone does.
I got the same thought, wondering as well if get_slot_xmins should be
renamed check_slot_xmins with the is() tests moved inside it as well.
Not sure if that's worth the API ugliness though.
Anyway, pushed this patch with minor editing. Thanks!
Thanks.
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Michael Paquier <michael.paquier@gmail.com> writes:
On Tue, Jun 27, 2017 at 3:44 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Looks good as far as it goes, but I wonder whether any of the other
get_slot_xmins calls need polling too. Don't feel a need to add such
calls until someone exhibits a failure there, but I won't be very
surprised if someone does.
And behold, we have here
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=nightjar&dt=2017-08-08%2020%3A54%3A09
# Failed test 'xmin of cascaded slot non-null with hs feedback'
# at t/001_stream_rep.pl line 224.
# got: ''
# expected: anything else
That's one of only four calls lacking a preceding wait_slot_xmins call.
The ones at lines 173 and 177 seem safe because nothing has happened yet.
I think the one at line 300 should be safe because the standby_2 server is
shut down at that point, but is there any way that the standby_1's view
hasn't updated by the time that happens?
I got the same thought, wondering as well if get_slot_xmins should be
renamed check_slot_xmins with the is() tests moved inside it as well.
Not sure if that's worth the API ugliness though.
Mmm, doesn't seem like that's worth doing, but I'm half tempted to merge
wait_slot_xmins into get_slot_xmins so you can't skip it ...
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, Aug 8, 2017 at 11:33 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Michael Paquier <michael.paquier@gmail.com> writes:
I got the same thought, wondering as well if get_slot_xmins should be
renamed check_slot_xmins with the is() tests moved inside it as well.
Not sure if that's worth the API ugliness though.Mmm, doesn't seem like that's worth doing, but I'm half tempted to merge
wait_slot_xmins into get_slot_xmins so you can't skip it ...
Let's do that please. Merging both was my first feeling when
refactoring this test upthread. Should I send a patch?
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Michael Paquier <michael.paquier@gmail.com> writes:
On Tue, Aug 8, 2017 at 11:33 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Michael Paquier <michael.paquier@gmail.com> writes:
I got the same thought, wondering as well if get_slot_xmins should be
renamed check_slot_xmins with the is() tests moved inside it as well.
Not sure if that's worth the API ugliness though.
Mmm, doesn't seem like that's worth doing, but I'm half tempted to merge
wait_slot_xmins into get_slot_xmins so you can't skip it ...
Let's do that please. Merging both was my first feeling when
refactoring this test upthread. Should I send a patch?
Sure, have at it.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Aug 9, 2017 at 3:39 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Michael Paquier <michael.paquier@gmail.com> writes:
On Tue, Aug 8, 2017 at 11:33 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Michael Paquier <michael.paquier@gmail.com> writes:
I got the same thought, wondering as well if get_slot_xmins should be
renamed check_slot_xmins with the is() tests moved inside it as well.
Not sure if that's worth the API ugliness though.Mmm, doesn't seem like that's worth doing, but I'm half tempted to merge
wait_slot_xmins into get_slot_xmins so you can't skip it ...Let's do that please. Merging both was my first feeling when
refactoring this test upthread. Should I send a patch?Sure, have at it.
And here you go.
--
Michael
Attachments:
tap-simplify-repslot.patchapplication/octet-stream; name=tap-simplify-repslot.patchDownload
diff --git a/src/test/recovery/t/001_stream_rep.pl b/src/test/recovery/t/001_stream_rep.pl
index 840af66f75..e57badbf5c 100644
--- a/src/test/recovery/t/001_stream_rep.pl
+++ b/src/test/recovery/t/001_stream_rep.pl
@@ -146,35 +146,34 @@ $node_standby_2->append_conf('postgresql.conf',
"wal_receiver_status_interval = 1");
$node_standby_2->restart;
-# Wait for given condition on slot's pg_replication_slots row --- useful for
-# ensuring we've reached a quiescent condition for reading slot xmins
-sub wait_slot_xmins
+# Fetch xmin columns from slot's pg_replication_slots row, waiting for
+# some conditions to be checked to ensure consistency in the result.
+sub get_slot_xmins
{
- my ($node, $slot_name, $check_expr) = @_;
+ my ($node, $slotname, $check_expr) = @_;
+ # Wait for given condition on slot's pg_replication_slots row --- useful
+ # for ensuring we've reached a quiescent condition for reading slot xmins.
$node->poll_query_until('postgres', qq[
SELECT $check_expr
FROM pg_catalog.pg_replication_slots
- WHERE slot_name = '$slot_name';
+ WHERE slot_name = '$slotname';
])
or die "Timed out waiting for slot xmins to advance";
-}
-# Fetch xmin columns from slot's pg_replication_slots row
-sub get_slot_xmins
-{
- my ($node, $slotname) = @_;
my $slotinfo = $node->slot($slotname);
return ($slotinfo->{'xmin'}, $slotinfo->{'catalog_xmin'});
}
# There's no hot standby feedback and there are no logical slots on either peer
# so xmin and catalog_xmin should be null on both slots.
-my ($xmin, $catalog_xmin) = get_slot_xmins($node_master, $slotname_1);
+my ($xmin, $catalog_xmin) = get_slot_xmins($node_master, $slotname_1,
+ "xmin IS NULL AND catalog_xmin IS NULL");
is($xmin, '', 'xmin of non-cascaded slot null with no hs_feedback');
is($catalog_xmin, '', 'catalog xmin of non-cascaded slot null with no hs_feedback');
-($xmin, $catalog_xmin) = get_slot_xmins($node_standby_1, $slotname_2);
+($xmin, $catalog_xmin) = get_slot_xmins($node_standby_1, $slotname_2,
+ "xmin IS NULL AND catalog_xmin IS NULL");
is($xmin, '', 'xmin of cascaded slot null with no hs_feedback');
is($catalog_xmin, '', 'catalog xmin of cascaded slot null with no hs_feedback');
@@ -212,18 +211,14 @@ $node_standby_2->safe_psql('postgres',
$node_standby_2->reload;
replay_check();
-wait_slot_xmins($node_master, $slotname_1,
- "xmin IS NOT NULL AND catalog_xmin IS NULL");
-
-($xmin, $catalog_xmin) = get_slot_xmins($node_master, $slotname_1);
+($xmin, $catalog_xmin) = get_slot_xmins($node_master, $slotname_1,
+ "xmin IS NOT NULL AND catalog_xmin IS NULL");
isnt($xmin, '', 'xmin of non-cascaded slot non-null with hs feedback');
is($catalog_xmin, '',
'catalog xmin of non-cascaded slot still null with hs_feedback');
-wait_slot_xmins($node_standby_1, $slotname_2,
- "xmin IS NOT NULL AND catalog_xmin IS NULL");
-
-my ($xmin1, $catalog_xmin1) = get_slot_xmins($node_standby_1, $slotname_2);
+my ($xmin1, $catalog_xmin1) = get_slot_xmins($node_standby_1, $slotname_2,
+ "xmin IS NOT NULL AND catalog_xmin IS NULL");
isnt($xmin1, '', 'xmin of cascaded slot non-null with hs feedback');
is($catalog_xmin1, '',
'catalog xmin of cascaded slot still null with hs_feedback');
@@ -246,17 +241,15 @@ end$$;
$node_master->safe_psql('postgres', 'VACUUM;');
$node_master->safe_psql('postgres', 'CHECKPOINT;');
-wait_slot_xmins($node_master, $slotname_1, "xmin <> '$xmin'");
-
-my ($xmin2, $catalog_xmin2) = get_slot_xmins($node_master, $slotname_1);
+my ($xmin2, $catalog_xmin2) = get_slot_xmins($node_master, $slotname_1,
+ "xmin <> '$xmin'");
note "master slot's new xmin $xmin2, old xmin $xmin";
isnt($xmin2, $xmin, 'xmin of non-cascaded slot with hs feedback has changed');
is($catalog_xmin2, '',
'catalog xmin of non-cascaded slot still null with hs_feedback unchanged');
-wait_slot_xmins($node_standby_1, $slotname_2, "xmin <> '$xmin1'");
-
-($xmin2, $catalog_xmin2) = get_slot_xmins($node_standby_1, $slotname_2);
+($xmin2, $catalog_xmin2) = get_slot_xmins($node_standby_1, $slotname_2,
+ "xmin <> '$xmin1'");
note "standby_1 slot's new xmin $xmin2, old xmin $xmin1";
isnt($xmin2, $xmin1, 'xmin of cascaded slot with hs feedback has changed');
is($catalog_xmin2, '',
@@ -273,18 +266,14 @@ $node_standby_2->safe_psql('postgres',
$node_standby_2->reload;
replay_check();
-wait_slot_xmins($node_master, $slotname_1,
- "xmin IS NULL AND catalog_xmin IS NULL");
-
-($xmin, $catalog_xmin) = get_slot_xmins($node_master, $slotname_1);
+($xmin, $catalog_xmin) = get_slot_xmins($node_master, $slotname_1,
+ "xmin IS NULL AND catalog_xmin IS NULL");
is($xmin, '', 'xmin of non-cascaded slot null with hs feedback reset');
is($catalog_xmin, '',
'catalog xmin of non-cascaded slot still null with hs_feedback reset');
-wait_slot_xmins($node_standby_1, $slotname_2,
- "xmin IS NULL AND catalog_xmin IS NULL");
-
-($xmin, $catalog_xmin) = get_slot_xmins($node_standby_1, $slotname_2);
+($xmin, $catalog_xmin) = get_slot_xmins($node_standby_1, $slotname_2,
+ "xmin IS NULL AND catalog_xmin IS NULL");
is($xmin, '', 'xmin of cascaded slot null with hs feedback reset');
is($catalog_xmin, '',
'catalog xmin of cascaded slot still null with hs_feedback reset');
@@ -301,16 +290,14 @@ $node_standby_2->safe_psql('postgres',
'ALTER SYSTEM SET hot_standby_feedback = off;');
$node_standby_2->stop;
-wait_slot_xmins($node_standby_1, $slotname_2, "xmin IS NOT NULL");
-
-($xmin, $catalog_xmin) = get_slot_xmins($node_standby_1, $slotname_2);
+($xmin, $catalog_xmin) = get_slot_xmins($node_standby_1, $slotname_2,
+ "xmin IS NOT NULL");
isnt($xmin, '', 'xmin of cascaded slot non-null with postgres shut down');
# Xmin from a previous run should be cleared on startup.
$node_standby_2->start;
-wait_slot_xmins($node_standby_1, $slotname_2, "xmin IS NULL");
-
-($xmin, $catalog_xmin) = get_slot_xmins($node_standby_1, $slotname_2);
+($xmin, $catalog_xmin) = get_slot_xmins($node_standby_1, $slotname_2,
+ "xmin IS NULL");
is($xmin, '',
'xmin of cascaded slot reset after startup with hs feedback reset');
Michael Paquier <michael.paquier@gmail.com> writes:
On Wed, Aug 9, 2017 at 3:39 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Michael Paquier <michael.paquier@gmail.com> writes:
Let's do that please. Merging both was my first feeling when
refactoring this test upthread. Should I send a patch?
Sure, have at it.
And here you go.
Pushed with a bit of work on the comments.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Sun, Aug 13, 2017 at 1:09 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Michael Paquier <michael.paquier@gmail.com> writes:
On Wed, Aug 9, 2017 at 3:39 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Michael Paquier <michael.paquier@gmail.com> writes:
Let's do that please. Merging both was my first feeling when
refactoring this test upthread. Should I send a patch?Sure, have at it.
And here you go.
Pushed with a bit of work on the comments.
Thanks.
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers