Re: Synchronizing slots from primary to standby - Mailing list pgsql-hackers

Here are some review comments for v41-0001.

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

1.
+     <para>
+      When altering the
+      <link linkend="sql-createsubscription-params-with-slot-name"><literal>slot_name</literal></link>,
+      the <literal>failover</literal> property of the new slot may
differ from the
+      <link linkend="sql-createsubscription-params-with-failover"><literal>failover</literal></link>
+      parameter specified in the subscription, you need to adjust the
+      <literal>failover</literal> property when creating the slot so that it
+      matches the value specified in subscription.
+     </para>

For the second part a) it should be a separate sentence, and b) IMO
you are not really "adjusting" something if you are "creating" it.

SUGGESTION
When altering the slot_name, the failover property of the new slot may
differ from the failover parameter specified in the subscription. When
creating the slot  ensure the slot failover property matches the
failover parameter value of the subscription.

======
src/backend/catalog/pg_subscription.c

2. AlterSubscription

+ if (sub->failoverstate == LOGICALREP_FAILOVER_STATE_ENABLED && opts.copy_data)
+ ereport(ERROR,
+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("ALTER SUBSCRIPTION with refresh and copy_data is not allowed
when failover is enabled"),

This is another example where the "two_phase" and "failover" should be
extracted to make a common message for the translator.

(Already posted this comment before -- see [1] #13)

~~~

3.
+ /*
+ * See comments above for twophasestate, same holds true for
+ * 'failover'
+ */
+ if (sub->failoverstate == LOGICALREP_FAILOVER_STATE_ENABLED && opts.copy_data)
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("ALTER SUBSCRIPTION ... REFRESH with copy_data is not allowed
when failover is enabled"),
+ errhint("Use ALTER SUBSCRIPTION ... REFRESH with copy_data = false,
or use DROP/CREATE SUBSCRIPTION.")));

This is another example where the "two_phase" and "failover" should be
extracted to make a common message for the translator.

(Already posted this comment before -- see [1] #14)

======
src/backend/replication/walsender.c

4.
+/*
+ * Wake up logical walsenders with failover-enabled slots if the physical slot
+ * of the current walsender is specified in standby_slot_names GUC.
+ */
+void
+PhysicalWakeupLogicalWalSnd(void)

Is it better to refer to "walsender processes" being woken instead of
just walsenders?

e.g.
"Wake up logical walsenders..." --> "Wake up the logical walsender processes..."

======
[1] v35-0001 review.
https://www.postgresql.org/message-id/CAHut%2BPv-yu71ogj_hRi6cCtmD55bsyw7XTxj1Nq8yVFKpY3NDQ%40mail.gmail.com

Kind Regards,
Peter Smith.
Fujitsu Australia