public inbox for [email protected]  
help / color / mirror / Atom feed
From: Peter Smith <[email protected]>
To: Bharath Rupireddy <[email protected]>
Cc: Amit Kapila <[email protected]>
Cc: Ajin Cherian <[email protected]>
Cc: Bertrand Drouvot <[email protected]>
Cc: shveta malik <[email protected]>
Cc: Masahiko Sawada <[email protected]>
Cc: Nathan Bossart <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: Re: Introduce XID age and inactive timeout based replication slot invalidation
Date: Tue, 10 Sep 2024 11:34:24 +1000
Message-ID: <CAHut+PsHmfuDQt_gN2udjb9Ln00EvWw-eiRag5prB7JOgn9eEA@mail.gmail.com> (raw)
In-Reply-To: <CAHut+PtJxxaVhjDCFPqE_V63VaAN_bL+PHRMak=dkghoGrZc0Q@mail.gmail.com>
References: <CAD21AoAd0BX0L17YxSCbSNbj=3V7dvLVGY0WhwLHAt724rNCZA@mail.gmail.com>
	<[email protected]>
	<CAJpy0uBthb-RG=Xg+mM-G6Xy01JLcBsizJuz+xbti08qP_W7AQ@mail.gmail.com>
	<CALj2ACVfDBcbybgyga7ia_KiQPAfEi_3=VDOGf2VU8RkJZSQpw@mail.gmail.com>
	<[email protected]>
	<CALj2ACUmp-F+irHYfNhmo_9yaCo7aGi7ZB20KQcDR3S9eT56hg@mail.gmail.com>
	<Zg1WqRS/[email protected]>
	<CALj2ACUBiRLHF7VYD-0nYVHSvVBSCQ_5OMbP4tE8Ys_rpvQ--A@mail.gmail.com>
	<[email protected]>
	<CALj2ACXbUzp-9tixeUK_dws5k+ARJaXxj8cj9W3adA0XNUS4Hg@mail.gmail.com>
	<[email protected]>
	<CALj2ACW8SYiRL8vGAhez=FdrzxO0W5pVFvN8GF7yKCO8_c=QdQ@mail.gmail.com>
	<CAA4eK1+-ipXB+LuUiDNQ5L+sWJFpzK89k-Oc7xOG37AefrptoQ@mail.gmail.com>
	<CALj2ACWhTY9mRc7sOpuao=GGidRjCVXc0SNtJ_DNman4nh5CcQ@mail.gmail.com>
	<CALj2ACVY+Fd5vC0VjW=5VDK9mmt-Y+PDZxnBp8ngGAZc24Vv9g@mail.gmail.com>
	<CALj2ACX0FVrx9f0c2g19Rb-WAJhm3wdyttde3YQtkotL6bUveA@mail.gmail.com>
	<CALj2ACXQNGXokgx8APwdxrG4MHMF=cOz6XQtUL7EHua9oUfkgA@mail.gmail.com>
	<CAFPTHDZPOc7pY4E0zJ2om0MZLMt+UuiX-VLsJyRO_aSyR8DsnA@mail.gmail.com>
	<CALj2ACVu0EJ+QQ7W0oHbL_+hCtk+p-00MNhE4hfjFcXaMepMfg@mail.gmail.com>
	<CAA4eK1KVbicrQQbMLEQiHEi9=VJpxrErrK7CiMamwaMPfff3WA@mail.gmail.com>
	<CALj2ACXe8+xSNdMXTMaSRWUwX7v61Ad4iddUwnn=djSwx3GLLg@mail.gmail.com>
	<CAHut+PuFzCHPCiZbpoQX59kgZbebuWT0gR0O7rOe4t_sdYu=OA@mail.gmail.com>
	<CALj2ACUJg2faYd-_Wr3NWoj_GtbW5Bje3G9+o595GyEhcBkbrg@mail.gmail.com>
	<CAHut+PuC7nwuT3+36zsLxtBAcUHrSaysrHb0_dxTXqMnFBKZBA@mail.gmail.com>
	<CALj2ACWXQT3_HY40ceqKf1DadjLQP6b1r=0sZRh-xhAOd-b0pA@mail.gmail.com>
	<CAHut+PtJxxaVhjDCFPqE_V63VaAN_bL+PHRMak=dkghoGrZc0Q@mail.gmail.com>

Hi, here is the remainder of my v45-0001 review. These comments are
for the test code only.

======
Testcase #1

1.
+# Testcase start
+#
+# Invalidate streaming standby slot and logical failover slot on primary due to
+# inactive timeout. Also, check logical failover slot synced to standby from
+# primary doesn't invalidate on its own, but gets the invalidated
state from the
+# primary.

nit - s/primary/the primary/ (in a couple of places)
nit - s/standby/the standby/
nit - other trivial tweaks.

~~~

2.
+# Create sync slot on primary
+$primary->psql('postgres',
+ q{SELECT pg_create_logical_replication_slot('sync_slot1',
'test_decoding', false, false, true);}
+);

nit - s/primary/the primary/

~~~

3.
+$primary->safe_psql(
+ 'postgres', qq[
+    SELECT pg_create_physical_replication_slot(slot_name :=
'sb_slot1', immediately_reserve := true);
+]);

Should this have a comment?

~~~

4.
+# Wait until standby has replayed enough data
+$primary->wait_for_catchup($standby1);

nit - s/standby/the standby/

~~~

5.
+# Sync primary slot to standby
+$standby1->safe_psql('postgres', "SELECT pg_sync_replication_slots();");

nit - /Sync primary slot to standby/Sync the primary slots to the standby/

~~~

6.
+# Confirm that logical failover slot is created on standby

nit - s/Confirm that logical failover slot is created on
standby/Confirm that the logical failover slot is created on the
standby/

~~~

7.
+is( $standby1->safe_psql(
+ 'postgres',
+ q{SELECT count(*) = 1 FROM pg_replication_slots
+   WHERE slot_name = 'sync_slot1' AND synced AND NOT temporary;}
+ ),
+ "t",
+ 'logical slot sync_slot1 has synced as true on standby');

IMO here you should also be checking that the sync slot state is NOT
invalidated, just as a counterpoint for the test part later that
checks that it IS invalidated.

~~~

8.
+my $inactive_timeout = 1;
+
+# Set timeout so that next checkpoint will invalidate inactive slot
+$primary->safe_psql(
+ 'postgres', qq[
+    ALTER SYSTEM SET replication_slot_inactive_timeout TO
'${inactive_timeout}s';
+]);
+$primary->reload;

8a.
nit - I think that $inactive_timeout assignment belongs below your comment.

~

8b.
nit - s/Set timeout so that next checkpoint will invalidate inactive
slot/Set timeout GUC so that the next checkpoint will invalidate
inactive slots/

~~~

9.
+# Check for logical failover slot to become inactive on primary. Note that
+# nobody has acquired slot yet, so it must get invalidated due to
+# inactive timeout.

nit - /Check for logical failover slot to become inactive on
primary./Wait for logical failover slot to become inactive on the
primary./
nit - /has acquired slot/has acquired the slot/

~~~

10.
+# Sync primary slot to standby. Note that primary slot has already been
+# invalidated due to inactive timeout. Standby must just sync inavalidated
+# state.

nit - minor, add "the". fix typo "inavalidated", etc. suggestion:

Re-sync the primary slots to the standby. Note that the primary slot was already
invalidated (above) due to inactive timeout. The standby must just
sync the invalidated
state.

~~~

11.
+# Make standby slot on primary inactive and check for invalidation
+$standby1->stop;

nit - /standby slot/the standby slot/
nit - /on primary/on the primary/

======
Testcase #2

12.
I'm not sure it is necessary to do all this extra work. IIUC, there
was already almost everything you needed in the previous Testcase #1.
So, I thought you could just combine this extra standby timeout test
in Testcase #1.

Indeed, your Testcase #1 comment still says it is doing this: ("Also,
check logical failover slot synced to standby from primary doesn't
invalidate on its own,...")

e.g.
- NEW: set the GUC timeout on the standby
- sync the sync_slot (already doing in test #1)
- ensure the synced slot is NOT invalid (already suggested above for test #1)
- NEW: then do a standby sleep > timeout duration
- NEW: then do a standby CHECKPOINT...
- NEW: then ensure the sync slot invalidation did NOT happen
- then proceed with the rest of test #1...

======
Testcase #3

13.
nit - remove a few blank lines to group associated statements together.

~~~

14.
+$publisher->safe_psql(
+ 'postgres', qq[
+    ALTER SYSTEM SET replication_slot_inactive_timeout TO '
${inactive_timeout}s';
+]);
+$publisher->reload;

nit - this deserves a comment, the same as in Testcase #1

======
sub wait_for_slot_invalidation

15.
+# Check for slot to first become inactive and then get invalidated
+sub check_for_slot_invalidation

nit - IMO the previous name was better (e.g. "wait_for.." instead of
"check_for...") because that describes exactly what the subroutine is
doing.

suggestion:
# Wait for the slot to first become inactive and then get invalidated
sub wait_for_slot_invalidation

~~~

16.
+{
+ my ($node, $slot, $offset, $inactive_timeout) = @_;
+ my $name = $node->name;

The variable $name seems too vague. How about $node_name?

~~~

17.
+ # Wait for invalidation reason to be set
+ $node->poll_query_until(
+ 'postgres', qq[
+ SELECT COUNT(slot_name) = 1 FROM pg_replication_slots
+ WHERE slot_name = '$slot' AND
+ invalidation_reason = 'inactive_timeout';
+ ])
+   or die
+   "Timed out while waiting for invalidation reason of slot $slot to
be set on node $name";

17a.
nit - /# Wait for invalidation reason to be set/# Check that the
invalidation reason is 'inactive_timeout'/

IIUC, the 'trigger_slot_invalidation' function has already invalidated
the slot at this point, so we are not really "Waiting..."; we are
"Checking..." that the reason was correctly set.

~

17b.
I think this code fragment maybe would be better put inside the
'trigger_slot_invalidation' function. (I've done this in the nitpicks
attachment)

~~~

18.
+ # Check that invalidated slot cannot be acquired
+ my ($result, $stdout, $stderr);
+
+ ($result, $stdout, $stderr) = $node->psql(
+ 'postgres', qq[
+ SELECT pg_replication_slot_advance('$slot', '0/1');
+ ]);

18a.
s/Check that invalidated slot/Check that an invalidated slot/

~

18b.
nit - Remove some blank lines, because the comment applies to all below it.

======
sub trigger_slot_invalidation

19.
+# Trigger slot invalidation and confirm it in server log
+sub trigger_slot_invalidation

nit - s/confirm it in server log/confirm it in the server log/

~

20.
+{
+ my ($node, $slot, $offset, $inactive_timeout) = @_;
+ my $name = $node->name;
+ my $invalidated = 0;

(same as the other subroutine)
nit - The variable $name seems too vague. How about $node_name?

======

Please refer to the attached nitpicks top-up patch which implements
most of the above nits.

======
Kind Regards,
Peter Smith.
Fujitsu Australia

diff --git a/src/test/recovery/t/050_invalidate_slots.pl b/src/test/recovery/t/050_invalidate_slots.pl
index 669d6cc..34b46d5 100644
--- a/src/test/recovery/t/050_invalidate_slots.pl
+++ b/src/test/recovery/t/050_invalidate_slots.pl
@@ -12,10 +12,10 @@ use Time::HiRes qw(usleep);
 # =============================================================================
 # Testcase start
 #
-# Invalidate streaming standby slot and logical failover slot on primary due to
-# inactive timeout. Also, check logical failover slot synced to standby from
-# primary doesn't invalidate on its own, but gets the invalidated state from the
-# primary.
+# Invalidate a streaming standby slot and logical failover slot on the primary
+# due to inactive timeout. Also, check that a logical failover slot synced to
+# the standby from the primary doesn't invalidate on its own, but gets the
+# invalidated state from the primary.
 
 # Initialize primary
 my $primary = PostgreSQL::Test::Cluster->new('primary');
@@ -45,7 +45,7 @@ primary_slot_name = 'sb_slot1'
 primary_conninfo = '$connstr dbname=postgres'
 ));
 
-# Create sync slot on primary
+# Create sync slot on the primary
 $primary->psql('postgres',
 	q{SELECT pg_create_logical_replication_slot('sync_slot1', 'test_decoding', false, false, true);}
 );
@@ -57,13 +57,13 @@ $primary->safe_psql(
 
 $standby1->start;
 
-# Wait until standby has replayed enough data
+# Wait until the standby has replayed enough data
 $primary->wait_for_catchup($standby1);
 
-# Sync primary slot to standby
+# Sync the primary slots to the standby
 $standby1->safe_psql('postgres', "SELECT pg_sync_replication_slots();");
 
-# Confirm that logical failover slot is created on standby
+# Confirm that the logical failover slot is created on the standby
 is( $standby1->safe_psql(
 		'postgres',
 		q{SELECT count(*) = 1 FROM pg_replication_slots
@@ -73,24 +73,24 @@ is( $standby1->safe_psql(
 	'logical slot sync_slot1 has synced as true on standby');
 
 my $logstart = -s $primary->logfile;
-my $inactive_timeout = 1;
 
-# Set timeout so that next checkpoint will invalidate inactive slot
+# Set timeout GUC so that that next checkpoint will invalidate inactive slots
+my $inactive_timeout = 1;
 $primary->safe_psql(
 	'postgres', qq[
     ALTER SYSTEM SET replication_slot_inactive_timeout TO '${inactive_timeout}s';
 ]);
 $primary->reload;
 
-# Check for logical failover slot to become inactive on primary. Note that
+# Wait for logical failover slot to become inactive on the primary. Note that
 # nobody has acquired slot yet, so it must get invalidated due to
 # inactive timeout.
-check_for_slot_invalidation($primary, 'sync_slot1', $logstart,
+wait_for_slot_invalidation($primary, 'sync_slot1', $logstart,
 	$inactive_timeout);
 
-# Sync primary slot to standby. Note that primary slot has already been
-# invalidated due to inactive timeout. Standby must just sync inavalidated
-# state.
+# Re-sync the primary slots to the standby. Note that the primary slot was
+# already invalidated (above) due to inactive timeout. The standby must just
+# sync the invalidated state.
 $standby1->safe_psql('postgres', "SELECT pg_sync_replication_slots();");
 $standby1->poll_query_until(
 	'postgres', qq[
@@ -101,9 +101,9 @@ $standby1->poll_query_until(
   or die
   "Timed out while waiting for sync_slot1 invalidation to be synced on standby";
 
-# Make standby slot on primary inactive and check for invalidation
+# Make the standby slot on the primary inactive and check for invalidation
 $standby1->stop;
-check_for_slot_invalidation($primary, 'sb_slot1', $logstart,
+wait_for_slot_invalidation($primary, 'sb_slot1', $logstart,
 	$inactive_timeout);
 
 # Testcase end
@@ -223,14 +223,12 @@ $publisher->safe_psql(
 $subscriber->safe_psql('postgres',
 	"CREATE SUBSCRIPTION sub CONNECTION '$publisher_connstr' PUBLICATION pub WITH (slot_name = 'lsub1_slot', create_slot = false)"
 );
-
 $subscriber->wait_for_subscription_sync($publisher, 'sub');
-
 my $result =
   $subscriber->safe_psql('postgres', "SELECT count(*) FROM test_tbl");
-
 is($result, qq(5), "check initial copy was done");
 
+# Set timeout GUC so that that next checkpoint will invalidate inactive slots
 $publisher->safe_psql(
 	'postgres', qq[
     ALTER SYSTEM SET replication_slot_inactive_timeout TO ' ${inactive_timeout}s';
@@ -241,17 +239,17 @@ $logstart = -s $publisher->logfile;
 
 # Make subscriber slot on publisher inactive and check for invalidation
 $subscriber->stop;
-check_for_slot_invalidation($publisher, 'lsub1_slot', $logstart,
+wait_for_slot_invalidation($publisher, 'lsub1_slot', $logstart,
 	$inactive_timeout);
 
 # Testcase end
 # =============================================================================
 
-# Check for slot to first become inactive and then get invalidated
-sub check_for_slot_invalidation
+# Wait for slot to first become inactive and then get invalidated
+sub wait_for_slot_invalidation
 {
 	my ($node, $slot, $offset, $inactive_timeout) = @_;
-	my $name = $node->name;
+	my $node_name = $node->name;
 
 	# Wait for slot to become inactive
 	$node->poll_query_until(
@@ -261,45 +259,33 @@ sub check_for_slot_invalidation
 				  inactive_since IS NOT NULL;
 	])
 	  or die
-	  "Timed out while waiting for slot $slot to become inactive on node $name";
+	  "Timed out while waiting for slot $slot to become inactive on node $node_name";
 
 	trigger_slot_invalidation($node, $slot, $offset, $inactive_timeout);
 
-	# Wait for invalidation reason to be set
-	$node->poll_query_until(
-		'postgres', qq[
-		SELECT COUNT(slot_name) = 1 FROM pg_replication_slots
-			WHERE slot_name = '$slot' AND
-			invalidation_reason = 'inactive_timeout';
-	])
-	  or die
-	  "Timed out while waiting for invalidation reason of slot $slot to be set on node $name";
-
-	# Check that invalidated slot cannot be acquired
+	# Check that an invalidated slot cannot be acquired
 	my ($result, $stdout, $stderr);
-
 	($result, $stdout, $stderr) = $node->psql(
 		'postgres', qq[
 			SELECT pg_replication_slot_advance('$slot', '0/1');
 	]);
-
 	ok( $stderr =~
 		  /can no longer get changes from replication slot "$slot"/,
-		"detected error upon trying to acquire invalidated slot $slot on node $name"
+		"detected error upon trying to acquire invalidated slot $slot on node $node_name"
 	  )
 	  or die
-	  "could not detect error upon trying to acquire invalidated slot $slot on node $name";
+	  "could not detect error upon trying to acquire invalidated slot $slot on node $node_name";
 }
 
-# Trigger slot invalidation and confirm it in server log
+# Trigger slot invalidation and confirm it in the server log
 sub trigger_slot_invalidation
 {
 	my ($node, $slot, $offset, $inactive_timeout) = @_;
-	my $name = $node->name;
+	my $node_name = $node->name;
 	my $invalidated = 0;
 
 	# Give enough time to avoid multiple checkpoints
-	sleep($inactive_timeout+1);
+	sleep($inactive_timeout + 1);
 
 	for (my $i = 0; $i < 10 * $PostgreSQL::Test::Utils::timeout_default; $i++)
 	{
@@ -314,8 +300,18 @@ sub trigger_slot_invalidation
 		usleep(100_000);
 	}
 	ok($invalidated,
-		"check that slot $slot invalidation has been logged on node $name"
+		"check that slot $slot invalidation has been logged on node $node_name"
 	);
+
+	# Check that the invalidation reason is 'inactive_timeout'
+	$node->poll_query_until(
+		'postgres', qq[
+		SELECT COUNT(slot_name) = 1 FROM pg_replication_slots
+			WHERE slot_name = '$slot' AND
+			invalidation_reason = 'inactive_timeout';
+	])
+	  or die
+	  "Timed out while waiting for invalidation reason of slot $slot to be set on node $node_name";
 }
 
 done_testing();


Attachments:

  [text/plain] PS_NITPICKS_20240910_SLOT_V450001_TESTS.txt (7.4K, ../CAHut+PsHmfuDQt_gN2udjb9Ln00EvWw-eiRag5prB7JOgn9eEA@mail.gmail.com/2-PS_NITPICKS_20240910_SLOT_V450001_TESTS.txt)
  download | inline diff:
diff --git a/src/test/recovery/t/050_invalidate_slots.pl b/src/test/recovery/t/050_invalidate_slots.pl
index 669d6cc..34b46d5 100644
--- a/src/test/recovery/t/050_invalidate_slots.pl
+++ b/src/test/recovery/t/050_invalidate_slots.pl
@@ -12,10 +12,10 @@ use Time::HiRes qw(usleep);
 # =============================================================================
 # Testcase start
 #
-# Invalidate streaming standby slot and logical failover slot on primary due to
-# inactive timeout. Also, check logical failover slot synced to standby from
-# primary doesn't invalidate on its own, but gets the invalidated state from the
-# primary.
+# Invalidate a streaming standby slot and logical failover slot on the primary
+# due to inactive timeout. Also, check that a logical failover slot synced to
+# the standby from the primary doesn't invalidate on its own, but gets the
+# invalidated state from the primary.
 
 # Initialize primary
 my $primary = PostgreSQL::Test::Cluster->new('primary');
@@ -45,7 +45,7 @@ primary_slot_name = 'sb_slot1'
 primary_conninfo = '$connstr dbname=postgres'
 ));
 
-# Create sync slot on primary
+# Create sync slot on the primary
 $primary->psql('postgres',
 	q{SELECT pg_create_logical_replication_slot('sync_slot1', 'test_decoding', false, false, true);}
 );
@@ -57,13 +57,13 @@ $primary->safe_psql(
 
 $standby1->start;
 
-# Wait until standby has replayed enough data
+# Wait until the standby has replayed enough data
 $primary->wait_for_catchup($standby1);
 
-# Sync primary slot to standby
+# Sync the primary slots to the standby
 $standby1->safe_psql('postgres', "SELECT pg_sync_replication_slots();");
 
-# Confirm that logical failover slot is created on standby
+# Confirm that the logical failover slot is created on the standby
 is( $standby1->safe_psql(
 		'postgres',
 		q{SELECT count(*) = 1 FROM pg_replication_slots
@@ -73,24 +73,24 @@ is( $standby1->safe_psql(
 	'logical slot sync_slot1 has synced as true on standby');
 
 my $logstart = -s $primary->logfile;
-my $inactive_timeout = 1;
 
-# Set timeout so that next checkpoint will invalidate inactive slot
+# Set timeout GUC so that that next checkpoint will invalidate inactive slots
+my $inactive_timeout = 1;
 $primary->safe_psql(
 	'postgres', qq[
     ALTER SYSTEM SET replication_slot_inactive_timeout TO '${inactive_timeout}s';
 ]);
 $primary->reload;
 
-# Check for logical failover slot to become inactive on primary. Note that
+# Wait for logical failover slot to become inactive on the primary. Note that
 # nobody has acquired slot yet, so it must get invalidated due to
 # inactive timeout.
-check_for_slot_invalidation($primary, 'sync_slot1', $logstart,
+wait_for_slot_invalidation($primary, 'sync_slot1', $logstart,
 	$inactive_timeout);
 
-# Sync primary slot to standby. Note that primary slot has already been
-# invalidated due to inactive timeout. Standby must just sync inavalidated
-# state.
+# Re-sync the primary slots to the standby. Note that the primary slot was
+# already invalidated (above) due to inactive timeout. The standby must just
+# sync the invalidated state.
 $standby1->safe_psql('postgres', "SELECT pg_sync_replication_slots();");
 $standby1->poll_query_until(
 	'postgres', qq[
@@ -101,9 +101,9 @@ $standby1->poll_query_until(
   or die
   "Timed out while waiting for sync_slot1 invalidation to be synced on standby";
 
-# Make standby slot on primary inactive and check for invalidation
+# Make the standby slot on the primary inactive and check for invalidation
 $standby1->stop;
-check_for_slot_invalidation($primary, 'sb_slot1', $logstart,
+wait_for_slot_invalidation($primary, 'sb_slot1', $logstart,
 	$inactive_timeout);
 
 # Testcase end
@@ -223,14 +223,12 @@ $publisher->safe_psql(
 $subscriber->safe_psql('postgres',
 	"CREATE SUBSCRIPTION sub CONNECTION '$publisher_connstr' PUBLICATION pub WITH (slot_name = 'lsub1_slot', create_slot = false)"
 );
-
 $subscriber->wait_for_subscription_sync($publisher, 'sub');
-
 my $result =
   $subscriber->safe_psql('postgres', "SELECT count(*) FROM test_tbl");
-
 is($result, qq(5), "check initial copy was done");
 
+# Set timeout GUC so that that next checkpoint will invalidate inactive slots
 $publisher->safe_psql(
 	'postgres', qq[
     ALTER SYSTEM SET replication_slot_inactive_timeout TO ' ${inactive_timeout}s';
@@ -241,17 +239,17 @@ $logstart = -s $publisher->logfile;
 
 # Make subscriber slot on publisher inactive and check for invalidation
 $subscriber->stop;
-check_for_slot_invalidation($publisher, 'lsub1_slot', $logstart,
+wait_for_slot_invalidation($publisher, 'lsub1_slot', $logstart,
 	$inactive_timeout);
 
 # Testcase end
 # =============================================================================
 
-# Check for slot to first become inactive and then get invalidated
-sub check_for_slot_invalidation
+# Wait for slot to first become inactive and then get invalidated
+sub wait_for_slot_invalidation
 {
 	my ($node, $slot, $offset, $inactive_timeout) = @_;
-	my $name = $node->name;
+	my $node_name = $node->name;
 
 	# Wait for slot to become inactive
 	$node->poll_query_until(
@@ -261,45 +259,33 @@ sub check_for_slot_invalidation
 				  inactive_since IS NOT NULL;
 	])
 	  or die
-	  "Timed out while waiting for slot $slot to become inactive on node $name";
+	  "Timed out while waiting for slot $slot to become inactive on node $node_name";
 
 	trigger_slot_invalidation($node, $slot, $offset, $inactive_timeout);
 
-	# Wait for invalidation reason to be set
-	$node->poll_query_until(
-		'postgres', qq[
-		SELECT COUNT(slot_name) = 1 FROM pg_replication_slots
-			WHERE slot_name = '$slot' AND
-			invalidation_reason = 'inactive_timeout';
-	])
-	  or die
-	  "Timed out while waiting for invalidation reason of slot $slot to be set on node $name";
-
-	# Check that invalidated slot cannot be acquired
+	# Check that an invalidated slot cannot be acquired
 	my ($result, $stdout, $stderr);
-
 	($result, $stdout, $stderr) = $node->psql(
 		'postgres', qq[
 			SELECT pg_replication_slot_advance('$slot', '0/1');
 	]);
-
 	ok( $stderr =~
 		  /can no longer get changes from replication slot "$slot"/,
-		"detected error upon trying to acquire invalidated slot $slot on node $name"
+		"detected error upon trying to acquire invalidated slot $slot on node $node_name"
 	  )
 	  or die
-	  "could not detect error upon trying to acquire invalidated slot $slot on node $name";
+	  "could not detect error upon trying to acquire invalidated slot $slot on node $node_name";
 }
 
-# Trigger slot invalidation and confirm it in server log
+# Trigger slot invalidation and confirm it in the server log
 sub trigger_slot_invalidation
 {
 	my ($node, $slot, $offset, $inactive_timeout) = @_;
-	my $name = $node->name;
+	my $node_name = $node->name;
 	my $invalidated = 0;
 
 	# Give enough time to avoid multiple checkpoints
-	sleep($inactive_timeout+1);
+	sleep($inactive_timeout + 1);
 
 	for (my $i = 0; $i < 10 * $PostgreSQL::Test::Utils::timeout_default; $i++)
 	{
@@ -314,8 +300,18 @@ sub trigger_slot_invalidation
 		usleep(100_000);
 	}
 	ok($invalidated,
-		"check that slot $slot invalidation has been logged on node $name"
+		"check that slot $slot invalidation has been logged on node $node_name"
 	);
+
+	# Check that the invalidation reason is 'inactive_timeout'
+	$node->poll_query_until(
+		'postgres', qq[
+		SELECT COUNT(slot_name) = 1 FROM pg_replication_slots
+			WHERE slot_name = '$slot' AND
+			invalidation_reason = 'inactive_timeout';
+	])
+	  or die
+	  "Timed out while waiting for invalidation reason of slot $slot to be set on node $node_name";
 }
 
 done_testing();


view thread (98+ messages)  latest in thread

reply

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: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
  Subject: Re: Introduce XID age and inactive timeout based replication slot invalidation
  In-Reply-To: <CAHut+PsHmfuDQt_gN2udjb9Ln00EvWw-eiRag5prB7JOgn9eEA@mail.gmail.com>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox