On Mon, Jun 26, 2017 at 12:12 PM, Craig Ringer <cr...@2ndquadrant.com> wrote:
> On 26 June 2017 at 11:06, Michael Paquier <michael.paqu...@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
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');
-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Reply via email to