Re: Synchronizing slots from primary to standby

Here are some review comments for the patch v58-0001

======
doc/src/sgml/catalogs.sgml

1.
+ <para>
+ If true, the associated replication slots (i.e. the main slot and the
+ table sync slots) in the upstream database are enabled to be
+ synchronized to the physical standbys.
+ </para></entry>

It seems the other single-sentence descriptions on this page have no
period (.) so for consistency maybe you should remove it here also.

======
src/backend/commands/subscriptioncmds.c

2. AlterSubscription

+ /*
+ * Do not allow changing the failover state if the
+ * subscription is enabled. This is because the failover
+ * state of the slot on the publisher cannot be modified if
+ * the slot is currently being acquired by the apply
+ * worker.
+ */

/being acquired/acquired/

~~~

3.
values[Anum_pg_subscription_subfailover - 1] =
BoolGetDatum(opts.failover);
replaces[Anum_pg_subscription_subfailover - 1] = true;

/*
* The failover state of the slot should be changed after
* the catalog update is completed.
*/
set_failover = true;
AFAICT you don't need to introduce a new variable 'set_failover'.
Instead, you can test like:

BEFORE
if (set_failover)

AFTER
if (replaces[Anum_pg_subscription_subfailover - 1])

======
src/backend/replication/logical/tablesync.c

4.
walrcv_create_slot(LogRepWorkerWalRcvConn,
slotname, false /* permanent */ , false /* two_phase */ ,
+ MySubscription->failover /* failover */ ,
CRS_USE_SNAPSHOT, origin_startpos);

The "/* failover */ comment is unnecessary now that you pass the
boolean field with the same descriptive name.

======
src/include/catalog/pg_subscription.h

5. CATALOG

+ bool subfailover; /* True if the associated replication slots
+ * (i.e. the main slot and the table sync
+ * slots) in the upstream database are enabled
+ * the upstream database are enabled to be
+ * synchronized to the physical standbys. */
+

The wording of the comment is broken (it says "are enabled" 2x).

SUGGESTION
True if the associated replication slots (i.e. the main slot and the
table sync slots) in the upstream database are enabled to be
synchronized to the physical standbys.

~~~

6. Subscription

+ bool failover; /* Indicates if the associated replication
+ * slots (i.e. the main slot and the table sync
+ * slots) in the upstream database are enabled
+ * to be synchronized to the physical
+ * standbys. */

This comment can say "True if...", so it will be the same as the
earlier CATALOG comment for 'subfailover'.

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