Re: Synchronizing slots from primary to standby

Here are some comments for the patch v49-0002.

(This is in addition to my review comments for v48-0002 [1])

======
src/backend/access/transam/xlogrecovery.c

1. FinishWalRecovery

+ *
+ * We do not update the sync_state from READY to NONE here, as any failed
+ * update could leave some slots in the 'NONE' state, causing issues during
+ * slot sync after restarting the server as a standby. While updating after
+ * switching to the new timeline is an option, it does not simplify the
+ * handling for both READY and NONE state slots. Therefore, we retain the
+ * READY state slots after promotion as they can provide useful information
+ * about their origin.
+ */

Do you know if that wording is correct? e.g., If you were updating
from READY to NONE and there was a failed update, that would leave
some slots still in a READY state, right? So why does the comment say
"could leave some slots in the 'NONE' state"?

======
src/backend/replication/slot.c

2. ReplicationSlotAlter

+ /*
+ * Do not allow users to drop the slots which are currently being synced
+ * from the primary to the standby.
+ */
+ if (RecoveryInProgress() &&
+ MyReplicationSlot->data.sync_state != SYNCSLOT_STATE_NONE)
+ ereport(ERROR,
+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("cannot alter replication slot \"%s\"", name),
+ errdetail("This slot is being synced from the primary server.")));
+

The comment looks wrong -- should say "Do not allow users to alter..."

======

3.
+##################################################
+# Test that synchronized slot can neither be decoded nor dropped by the user
+##################################################
+

3a,
/Test that synchronized slot/Test that a synchronized slot/

3b.
Isn't there a missing test? Should this part also check that it cannot
ALTER the replication slot being synced? e.g. test for the new v49
error message that was added in ReplicationSlotAlter()

~~~

4.
+# Disable hot_standby_feedback
+$standby1->safe_psql('postgres', 'ALTER SYSTEM SET
hot_standby_feedback = off;');
+$standby1->restart;
+

Can there be a comment added to explain why you are doing the
'hot_standby_feedback' toggle?

~~~

5.
+##################################################
+# Promote the standby1 to primary. Confirm that:
+# a) the sync-ready('r') slot 'lsub1_slot' is retained on the new primary
+# b) the initiated('i') slot 'logical_slot' is dropped on promotion
+# c) logical replication for regress_mysub1 is resumed succesfully
after failover
+##################################################

/succesfully/successfully/

~~~

6.
+
+# Confirm that data in tab_mypub3 replicated on subscriber
+is( $subscriber1->safe_psql('postgres', q{SELECT count(*) FROM tab_int;}),
+ "$primary_row_count",
+ 'data replicated from the new primary');

The comment is wrong -- it names a different table ('tab_mypub3' ?) to
what the SQL says.

======
[1] My v48-0002 review comments.
https://www.postgresql.org/message-id/CAHut%2BPsyZQZ1A4XcKw-D%3DvcTg16pN9Dw0PzE8W_X7Yz_bv00rQ%40mail.gmail.com

Kind Regards,
Peter Smith.
Fujitsu Australia