Here are some review comments for v72-0001

======
doc/src/sgml/ref/alter_subscription.sgml

1.
+      parameter value of the subscription. Otherwise, the slot on the
+      publisher may behave differently from what subscription's
+      <link 
linkend="sql-createsubscription-params-with-failover"><literal>failover</literal></link>
+      option says. The slot on the publisher could either be
+      synced to the standbys even when the subscription's
+      <link 
linkend="sql-createsubscription-params-with-failover"><literal>failover</literal></link>
+      option is disabled or could be disabled for sync
+      even when the subscription's
+      <link 
linkend="sql-createsubscription-params-with-failover"><literal>failover</literal></link>
+      option is enabled.
+     </para>

It is a bit wordy to keep saying "disabled/enabled"

BEFORE
The slot on the publisher could either be synced to the standbys even
when the subscription's failover option is disabled or could be
disabled for sync even when the subscription's failover option is
enabled.

SUGGESTION
The slot on the publisher could be synced to the standbys even when
the subscription's failover = false or may not be syncing even when
the subscription's failover = true.

======
.../t/040_standby_failover_slots_sync.pl

2.
+# Enable subscription
+$subscriber1->safe_psql('postgres',
+ "ALTER SUBSCRIPTION regress_mysub1 ENABLE");
+
+# Disable failover for enabled subscription
+my ($result, $stdout, $stderr) = $subscriber1->psql('postgres',
+ "ALTER SUBSCRIPTION regress_mysub1 SET (failover = false)");
+ok( $stderr =~ /ERROR:  cannot set failover for enabled subscription/,
+ "altering failover is not allowed for enabled subscription");
+

Currently, those tests are under scope the big comment:

+##################################################
+# Test that changing the failover property of a subscription updates the
+# corresponding failover property of the slot.
+##################################################

But that comment is not quite relevant to these tests. So, add another
one just these:

SUGGESTION:
##################################################
# Test that cannot modify the failover option for enabled subscriptions.
##################################################

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


Reply via email to