Subject: Re: Synchronizing slots from primary to standby

From: Peter Smith <[email-masked]>
Date: Tue, 21 Nov 2023 16:39:21 +1100

Here are some review comments for the patch v35-0001.

======
0. GENERAL documentation

I felt that the documentation gave details of the individual changes
(e.g. GUC 'standby_slot_names' and API, CREATE SUBSCRIPTION option,
and pg_replication_slots 'failover' attribute etc.) but there is
nothing that seemed to bring all these parts together to give examples
for user "when" and "how" to make all these parts work. I'm not sure
if there is some overview missing from this patch 00001 or if you are
planning that extra documentation for subsequent patches.

======
Commit message

1.
A new property 'failover' is added at the slot level which
is persistent information which specifies that this logical slot
is enabled to be synced to the physical standbys so that logical
replication can be resumed after failover. It is always false
for physical slots.

~

SUGGESTION
A new property 'failover' is added at the slot level. This is
persistent information to indicate that this logical slot...

~~~

2.
Users can set it during the create subscription or during
pg_create_logical_replication_slot. Examples:

create subscription mysub  connection '..' publication mypub
WITH (failover = true);

--last arg
SELECT * FROM pg_create_logical_replication_slot('myslot',
'pgoutput', false, true, true);

~

2a.
Add a blank line before this

~

2b.
Use uppercase for the SQL

~

2c.
SUGGESTION
Users can set this flag during CREATE SUBSCRIPTION or during
pg_create_logical_replication_slot API.

Ex1.
CREATE SUBSCRIPTION mysub CONNECTION '...' PUBLICATION mypub
WITH (failover = true);

Ex2. (failover is the last arg)
SELECT * FROM pg_create_logical_replication_slot('myslot',
'pgoutput', false, true, true);

~~~

3.
This 'failover' is displayed as part of pg_replication_slots
view.

~

SUGGESTION
The value of the 'failover' flag is displayed as part of
pg_replication_slots view.

~~~

4.
A new GUC standby_slot_names has been added. It is the list of
physical replication slots that logical replication with failover
enabled waits for. The intent of this wait is that no logical
replication subscribers (with failover=true) should go
ahead of physical replication standbys (corresponding to the
physical slots in standby_slot_names).

~

4a.
SUGGESTION
A new GUC standby_slot_names has been added. This is a list of
physical replication slots that logical replication with failover
enabled will wait for.

~

4b.
/no logical replication subscribers/no logical replication subscriptions/

~

4c
/should go ahead of physical/should get ahead of physical/

======
contrib/test_decoding/sql/slot.sql

5.
+
+-- Test logical slots creation with 'failover'=true (last arg)
+SELECT 'init' FROM
pg_create_logical_replication_slot('failover_slot', 'test_decoding',
false, false, true);
+SELECT slot_name, slot_type, failover FROM pg_replication_slots;
+
+SELECT pg_drop_replication_slot('failover_slot');

How about a couple more simple tests:
a) pass false arg to confirm it is false in the view.
b) according to the docs this failover is optional, so try API without
passing it
c) create a physical slot to confirm it is false in the view.

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

6.
+     <row>
+      <entry role="catalog_table_entry"><para role="column_definition">
+       <structfield>subfailoverstate</structfield> <type>char</type>
+      </para>
+      <para>
+       State codes for failover mode:
+       <literal>d</literal> = disabled,
+       <literal>p</literal> = pending enablement,
+       <literal>e</literal> = enabled
+      </para></entry>
+     </row>
+

This attribute is very similar to the 'subtwophasestate' so IMO it
would be better to be adjacent to that one in the docs.

(probably this means putting it in the same order in the catalog also,
assuming that is allowed)

======
doc/src/sgml/config.sgml

7.
+       <para>
+        List of physical replication slots that logical replication slots with
+        failover enabled waits for. If a logical replication connection is
+        meant to switch to a physical standby after the standby is promoted,
+        the physical replication slot for the standby should be listed here.
+       </para>
+       <para>
+        The standbys corresponding to the physical replication slots in
+        <varname>standby_slot_names</varname> must enable
+        <varname>enable_syncslot</varname> for the standbys to receive
+        failover logical slots changes from the primary.
+       </para>

That sentence mentioning 'enable_syncslot' seems premature because
AFAIK that GUC is not introduced until patch 0002. So this part should
be moved into the 0002 patch.

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

8.
    These commands also cannot be executed when the subscription has
    <link linkend="sql-createsubscription-params-with-two-phase"><literal>two_phase</literal></link>
-   commit enabled, unless
+   commit enabled or
+   <link linkend="sql-createsubscription-params-with-failover"><literal>failover</literal></link>
+   enabled, unless
    <link linkend="sql-createsubscription-params-with-copy-data"><literal>copy_data</literal></link>
    is <literal>false</literal>. See column
<structfield>subtwophasestate</structfield>
-   of <link linkend="catalog-pg-subscription"><structname>pg_subscription</structname></link>
+   and <structfield>subfailoverstate</structfield> of
+   <link linkend="catalog-pg-subscription"><structname>pg_subscription</structname></link>
    to know the actual two-phase state.
I think the last sentence doesn't make sense anymore because it is no
longer talking about only two-phase state.

BEFORE
See column subtwophasestate and subfailoverstate of pg_subscription to
know the actual two-phase state.

SUGGESTION
See column subtwophasestate and subfailoverstate of pg_subscription to
know the actual states.

======
doc/src/sgml/ref/create_subscription.sgml

9.
+
+       <varlistentry id="sql-createsubscription-params-with-failover">
+        <term><literal>failover</literal> (<type>boolean</type>)</term>
+        <listitem>
+         <para>
+          Specifies whether the replication slot assocaited with the
subscription
+          is enabled to be synced to the physical standbys so that logical
+          replication can be resumed from the new primary after failover.
+          The default is <literal>false</literal>.
+         </para>
+
+         <para>
+          The implementation of failover requires that replication
+          has successfully finished the initial table synchronization
+          phase. So even when <literal>failover</literal> is enabled for a
+          subscription, the internal failover state remains
+          temporarily <quote>pending</quote> until the initialization phase
+          completes. See column <structfield>subfailoverstate</structfield>
+          of <link
linkend="catalog-pg-subscription"><structname>pg_subscription</structname></link>
+          to know the actual failover state.
+         </para>
+
+        </listitem>
+       </varlistentry>

9a.
/assocaited/associated/

~

9b.
Unnecessary blank line before </listitem>

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

10.
 #define SUBOPT_ORIGIN 0x00004000
+#define SUBOPT_FAILOVER 0x00008000

Bad indentation

~~~

11. CreateSubscription
+ /*
+ * If only the slot_name is specified, it is possible that the user intends to
+ * use an existing slot on the publisher, so here we enable failover for the
+ * slot if requested.
+ */
+ else if (opts.slot_name && failover_enabled)
+ {
+ walrcv_alter_slot(wrconn, opts.slot_name, opts.failover);
+ ereport(NOTICE,
+ (errmsg("enabled failover for replication slot \"%s\" on publisher",
+ opts.slot_name)));
+ }

11a.
How does this code ensure that *only* slot_name was set (e.g the
comment says "only the slot_name is specified")

~

11b.
Should 3rd arg to walrcv_alter_slot be 'failover_enabled', or maybe just 'true'?

~~~

12. 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"),
+ errhint("Use ALTER SUBSCRIPTION ... SET PUBLICATION with refresh =
false, or with copy_data = false, or use DROP/CREATE
SUBSCRIPTION.")));

12a.
This should have a comment like what precedes the sub->twophasestate
error. Or maybe group them both and use the same common comment.

~

12b.
AFAIK when there are messages like this that differ only by
non-translatable things ("failover" option) then that non-translatable
thing should be extracted as a parameter so the messages are common.
And, don't forget to add a /* translator: %s is a subscription option
like 'failover' */ comment.

SUGGESTION like:
errmsg("ALTER SUBSCRIPTION with refresh and copy_data is not allowed
when %s is enabled", "two_phase")
errmsg("ALTER SUBSCRIPTION with refresh and copy_data is not allowed
when %s is enabled", "failover")

~~~

13.
+ 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"),
+ /* translator: %s is an SQL ALTER command */
+ errhint("Use %s with refresh = false, or with copy_data = false, or
use DROP/CREATE SUBSCRIPTION.",
+ isadd ?
+ "ALTER SUBSCRIPTION ... ADD PUBLICATION" :
+ "ALTER SUBSCRIPTION ... DROP PUBLICATION")));

Same comment as above #12b.

SUGGESTION like:
errmsg("ALTER SUBSCRIPTION with refresh and copy_data is not allowed
when %s is enabled", "two_phase")
errmsg("ALTER SUBSCRIPTION with refresh and copy_data is not allowed
when %s is enabled", "failover")

~~~

14.
+ /*
+ * 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.")));

IMO this is another message where the option should be extracted to
make a common message for the translators. And don't forget to add a
/* translator: %s is a subscription option like 'failover' */ comment.

SUGGESTION like:
errmsg("ALTER SUBSCRIPTION ... REFRESH with copy_data is not allowed
when %s is enabled", "two_phase"),
errmsg("ALTER SUBSCRIPTION ... REFRESH with copy_data is not allowed
when %s is enabled", "failover"),

======
.../libpqwalreceiver/libpqwalreceiver.c

15. libpqrcv_create_slot

+ if (failover)
+ {
+ appendStringInfoString(&cmd, "FAILOVER");
+ if (use_new_options_syntax)
+ appendStringInfoString(&cmd, ", ");
+ else
+ appendStringInfoChar(&cmd, ' ');
+ }

15a.
Isn't failover a new option that is unsupported pre-PG17? Why is it
necessary to support an old-style syntax for something that was not
supported on old servers? (I'm confused).

~

15b.
Also IIRC, this FAILOVER wasn't not listed in the old-style syntax of
doc/src/sgml/protocol.sgml. Was that deliberate?

======
.../replication/logical/logicalfuncs.c

16. pg_logical_slot_get_changes_guts

+ if (XLogRecPtrIsInvalid(upto_lsn))
+ wal_to_wait = end_of_wal;
+ else
+ wal_to_wait = Min(upto_lsn, end_of_wal);
+
+ /*
+ * Wait for specified streaming replication standby servers (if any)
+ * to confirm receipt of WAL upto wal_to_wait.
+ */
+ WalSndWaitForStandbyConfirmation(wal_to_wait);
+

16a.
/WAL upto wal_to_wait./WAL up to wal_to_wait./

~

16b.
Is there another name for this variable (wal_to_wait) that conveys
more meaning? Maybe 'wal_received_pos' or 'wait_for_wal_lsn' or
something better.

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

17. process_syncing_tables_for_apply

  CommandCounterIncrement(); /* make updates visible */
  if (AllTablesyncsReady())
  {
+ char buf[100];
+
+ buf[0] = '\0';
+
+ if (MySubscription->twophasestate == LOGICALREP_TWOPHASE_STATE_PENDING)
+ strcat(buf, "twophase");
+ if (MySubscription->failoverstate == LOGICALREP_FAILOVER_STATE_PENDING)
+ {
+ if (buf[0] != '\0')
+ strcat(buf, " and ");
+ strcat(buf, "failover");
+ }
+
  ereport(LOG,
- (errmsg("logical replication apply worker for subscription \"%s\"
will restart so that two_phase can be enabled",
- MySubscription->name)));
+ (errmsg("logical replication apply worker for subscription \"%s\"
will restart so that %s can be enabled",
+ MySubscription->name, buf)));
  should_exit = true;
  }
~

IMO you cannot build up a log buffer using " and " like this because
the translation would be a mess. IIUC, you might have to do it the
long way with multiple errmsg.

SUGGESTION
twophase_pending = MySubscription->twophasestate ==
LOGICALREP_TWOPHASE_STATE_PENDING;
failover_pending = MySubscription->failoverstate ==
LOGICALREP_FAILOVER_STATE_PENDING;

if (twophase_pending || twophase_pending)
  ereport(LOG,
    twophase_pending && twophase_pending
  /* translator: 'two_phase' or 'failover' are subscription options */
? errmsg("logical replication apply worker for subscription \"%s\"
will restart so that two_phase and failover can be enabled")
: errmsg("logical replication apply worker for subscription \"%s\"
will restart so that %s can be enabled",
         twophase_pending ? "two_phase" : "failover"));

~~~

18. UpdateTwoPhaseFailoverStates

-UpdateTwoPhaseState(Oid suboid, char new_state)
+UpdateTwoPhaseFailoverStates(Oid suboid,
+ bool update_twophase, char new_state_twophase,
+ bool update_failover, char new_state_failover)

Although this function is written to update to *any* specified state,
in practice it only ever seems called to update from PENDING to ENABLE
state and nothing else.

Therefore it can be simplified by not even passing those states, and
by changing the function name like 'EnableTwoPhaseFailoverTriState'

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

19. File header comment

There is a lot of talk here about two_phase tri-state and the special
ALTER REFRESH considerations for the two-phase transactions. IIUC,
there should be lots of similar commentary for the failover tri-sate
and ALTER REFRESH.

~~~

20.
  * PENDING, which allows ALTER SUBSCRIPTION ... REFRESH PUBLICATION to
  * work.
  */
- if (MySubscription->twophasestate == LOGICALREP_TWOPHASE_STATE_PENDING &&
- AllTablesyncsReady())
+ twophase_pending = (MySubscription->twophasestate
+ == LOGICALREP_TWOPHASE_STATE_PENDING) ? true : false;
+ failover_pending = (MySubscription->failoverstate
+ == LOGICALREP_FAILOVER_STATE_PENDING) ? true : false;
+

The comment preceding this is only talking about 'two_phase', so
should be expanded to mention also 'failover'

~~~

21. run_apply_worker

+ twophase_pending = (MySubscription->twophasestate
+ == LOGICALREP_TWOPHASE_STATE_PENDING) ? true : false;
+ failover_pending = (MySubscription->failoverstate
+ == LOGICALREP_FAILOVER_STATE_PENDING) ? true : false;

These ternaries are not necessary.

SUGGESTION (has the same meaning)
twophase_pending = (MySubscription->twophasestate ==
LOGICALREP_TWOPHASE_STATE_PENDING);
failover_pending = (MySubscription->failoverstate ==
LOGICALREP_FAILOVER_STATE_PENDING);

~~~

22.
- UpdateTwoPhaseState(MySubscription->oid, LOGICALREP_TWOPHASE_STATE_ENABLED);
- MySubscription->twophasestate = LOGICALREP_TWOPHASE_STATE_ENABLED;
+
+ /* Update twophase and/or failover */
+ if (twophase_pending || failover_pending)
+ UpdateTwoPhaseFailoverStates(MySubscription->oid,
+ twophase_pending,
+ LOGICALREP_TWOPHASE_STATE_ENABLED,
+ failover_pending,
+ LOGICALREP_FAILOVER_STATE_ENABLED);
+ if (twophase_pending)
+ MySubscription->twophasestate = LOGICALREP_TWOPHASE_STATE_ENABLED;
+
+ if (failover_pending)
+ MySubscription->failoverstate = LOGICALREP_FAILOVER_STATE_ENABLED;

There seem rather too many checks for 'twophase_pending' and 'failover_pending'.

With some refactoring this could be done with less code I think. For example,
1. Unconditionally call UpdateTwoPhaseFailoverStates() but just quick
return if nothing to do
2. Pass address of MySubscription->twophasestate/failoverstate, and
let function UpdateTwoPhaseFailoverStates() set those

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

23.
+char    *standby_slot_names;
+static List    *standby_slot_names_list = NIL;

Should there be a comment for the new GUC?

~~~

24. ReplicationSlotAlter
+/*
+ * Change the definition of the slot identified by the passed in name.
+ */
+void
+ReplicationSlotAlter(const char *name, bool failover)

/passed in/specified/

/the definition/the failover state/

~~~

25. validate_standby_slots
+
+/*
+ * A helper function to validate slots specified in standby_slot_names GUCs.
+ */
+static bool
+validate_standby_slots(char **newval)

/in standby_slot_names GUCs./in GUC standby_slot_names./

~

26. validate_standby_slots

+ /*
+ * Verify 'type' of slot now.
+ *
+ * Skip check if replication slots' data is not initialized yet i.e. we
+ * are in startup process.
+ */
+ if (!ReplicationSlotCtl)
+ return true;

26a.
This code seems to neglect doing memory cleanup.

+ pfree(rawname);
+ list_free(elemlist);

~

26b.
Indeed, most of this function's return points seem to be neglecting
some memory cleanup, so IMO it would be better to write this function
with some common goto labels that do all this common cleanup:

SUGGESTION
ret_standby_slot_names_ok:
  pfree(rawname);
  list_free(elemlist);
  return true;

ret_standby_slot_names_ng:
  pfree(rawname);
  list_free(elemlist);
  return false;

~

27. validate_standby_slots

+ if (SlotIsLogical(slot))
+ {
+ GUC_check_errdetail("cannot have logical replication slot \"%s\" "
+ "in this parameter", name);
+ list_free(elemlist);
+ return false;
+ }

IIUC, the GUC is for physical replication slots only, so somehow I
felt it was better to keep everything from that (physical)
perspective. YMMV.

SUGGESTION
if (!SlotIsPhysical(slot))
{
  GUC_check_errdetail("\"%s\" is not a physical replication slot", name);
  list_free(elemlist);
  return false;
}

~~~

28. check_standby_slot_names

+bool
+check_standby_slot_names(char **newval, void **extra, GucSource source)
+{
+ if (strcmp(*newval, "") == 0)
+ return true;
+
+ /*
+ * "*" is not accepted as in that case primary will not be able to know
+ * for which all standbys to wait for. Even if we have physical-slots
+ * info, there is no way to confirm whether there is any standby
+ * configured for the known physical slots.
+ */
+ if (strcmp(*newval, "*") == 0)
+ {
+ GUC_check_errdetail("\"%s\" is not accepted for standby_slot_names",
+ *newval);
+ return false;
+ }
+
+ /* Now verify if the specified slots really exist and have correct type */
+ if (!validate_standby_slots(newval))
+ return false;
+
+ *extra = guc_strdup(ERROR, *newval);
+
+ return true;
+}

Is it really necessary to have a special test for the special value
"*" which you are going to reject? I don't see why this should be any
different from checking for other values like "." or "$" or "?" etc.
Why not just let validate_standby_slots() handle all of these?

~~~

29. assign_standby_slot_names

+ /* No value is specified for standby_slot_names. */
+ if (standby_slot_names_cpy == NULL)
+ return;

Is this possible? IIUC the check_standby_slot_names() did:
*extra = guc_strdup(ERROR, *newval);

Maybe this code also needs a similar elog and comment like already in
this function:
/* This should not happen if GUC checked check_standby_slot_names. */

~

30. assign_standby_slot_names

+ char    *standby_slot_names_cpy = extra;

IIUC, the 'extra' was unconditionally guc_strdup()'ed in the check
hook, so should we also free it here before leaving this function?

~~~

31. GetStandbySlotList

+/*
+ * Return a copy of standby_slot_names_list if the copy flag is set to true,
+ * otherwise return the original list.
+ */
+List *
+GetStandbySlotList(bool copy)
+{
+ if (copy)
+ return list_copy(standby_slot_names_list);
+ else
+ return standby_slot_names_list;
+}

Why is this better than just exposing the standby_slot_names_list. The
caller can make a copy or not.
e.g. why is calling GetStandbySlotList(true) better than just doing
list_copy(standby_slot_names_list)?

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

32. parseCreateReplSlotOptions

 static void WalSndSegmentOpen(XLogReaderState *state, XLogSegNo nextSegNo,
    TimeLineID *tli_p);

-
 /* Initialize walsender process before entering the main command loop */
 void

~

Unnecessary changing of whitespace unrelated to this patch.

~~~

33. WalSndWakeupNeeded

+/*
+ * Does this Wal Sender need to wake up logical walsender.
+ *
+ * Check if the physical slot of this walsender is specified in
+ * standby_slot_names GUC.
+ */
+static bool
+WalSndWakeupNeeded()

/Wal Sender/physical walsender process/  (maybe??)

~~~

34. WalSndFilterStandbySlots

+ /* Log warning if no active_pid for this physical slot */
+ if (slot->active_pid == 0)
+ ereport(WARNING,

Other nearby code is guarding the slot in case it was NULL, so why not
here? Is it a potential NPE?

~~~

35.
+ /*
+ * If logical slot name is given in standby_slot_names, give WARNING
+ * and skip it. Since it is harmless, so WARNING should be enough, no
+ * need to error-out.
+ */
+ else if (SlotIsLogical(slot))
+ warningfmt = _("cannot have logical replication slot \"%s\" in
parameter \"%s\", ignoring");

Is this possible? Doesn't the function 'validate_standby_slots' called
by the GUC hook prevent specifying logical slots in the GUC? Maybe
this warning should be changed to Assert?

~~~

36.
+ /*
+ * Reaching here indicates that either the slot has passed the
+ * wait_for_lsn or there is an issue with the slot that requires a
+ * warning to be reported.
+ */
+ if (warningfmt)
+ ereport(WARNING, errmsg(warningfmt, name, "standby_slot_names"));
+
+ standby_slots_cpy = foreach_delete_current(standby_slots_cpy, lc);

If something was wrong with the slot that required a warning, is it
really OK to remove this slot from the list? This seems contrary to
the function comment which only talks about removing slots that have
caught up.

~~~

37. WalSndWaitForStandbyConfirmation

+/*
+ * Wait for physical standby to confirm receiving given lsn.
+ *
+ * Here logical walsender associated with failover logical slot waits
+ * for physical standbys corresponding to physical slots specified in
+ * standby_slot_names GUC.
+ */

/given/the given/

~~~

38. WalSndWaitForStandbyConfirmation

+ ConditionVariablePrepareToSleep(&WalSndCtl->wal_confirm_rcv_cv);
+
+ for (;;)

This ConditionVariablePrepareToSleep was already called in the
WalSndWait() function. Did it need to be called 2 times?

~~~

39.
+ WalSndFilterStandbySlots(wait_for_lsn, &standby_slots);
+
+ /* Exit if done waiting for every slot. */
+ if (standby_slots == NIL)
+ break;
+
+ CHECK_FOR_INTERRUPTS();
+
+ if (ConfigReloadPending)
+ {
+ ConfigReloadPending = false;
+ WalSndRereadConfigAndSlots(&standby_slots);
+ }

Shouldn't all the config reload stuff come first before the filter and
NIL check, just in case after the reload there is nothing to do?
Otherwise, it might cause unnecessary sleep.

~~~

40. WalSndWaitForWal

 /*
  * Wait till WAL < loc is flushed to disk so it can be safely sent to client.
  *
- * Returns end LSN of flushed WAL.  Normally this will be >= loc, but
- * if we detect a shutdown request (either from postmaster or client)
- * we will return early, so caller must always check.
+ * If the walsender holds a logical slot that has enabled failover, the
+ * function also waits for all the specified streaming replication standby
+ * servers to confirm receipt of WAL upto RecentFlushPtr.
+ *
+ * Returns end LSN of flushed WAL.  Normally this will be >= loc, but if we
+ * detect a shutdown request (either from postmaster or client) we will return
+ * early, so caller must always check.
  */
 static XLogRecPtr
 WalSndWaitForWal(XLogRecPtr loc)

~

/upto/up to/

~~~

41.
  /*
- * Fast path to avoid acquiring the spinlock in case we already know we
- * have enough WAL available. This is particularly interesting if we're
- * far behind.
+ * Check if all the standby servers have confirmed receipt of WAL upto
+ * RecentFlushPtr if we already know we have enough WAL available.
+ *
+ * Note that we cannot directly return without checking the status of
+ * standby servers because the standby_slot_names may have changed, which
+ * means there could be new standby slots in the list that have not yet
+ * caught up to the RecentFlushPtr.
  */
  if (RecentFlushPtr != InvalidXLogRecPtr &&
  loc <= RecentFlushPtr)
- return RecentFlushPtr;
+ {
+ WalSndFilterStandbySlots(RecentFlushPtr, &standby_slots);

41a.
/upto/up to/

~

41b.
IMO there is some missing information in this comment because it
wasn't clear to me that calling WalSndFilterStandbySlots was going to
side-efect that list to give it a different meaning. e.g. it seems it
no longer means "standby slots" but instead means something like
"standby slots that are not caught up". Perhaps that local variable
can have a name that helps to convey that better?

~~~

42.
+ /*
+ * Fast path to entering the loop in case we already know we have
+ * enough WAL available and all the standby servers has confirmed
+ * receipt of WAL upto RecentFlushPtr. This is particularly
+ * interesting if we're far behind.
+ */
+ if (standby_slots == NIL)
+ return RecentFlushPtr;

42a.
/has/have/

~

42b.
For entering what loop? There's no context for this comment. I assume
it means the loop that comes later in this function, but then isn't
this a typo? /Fast path to entering the loop/Fast path to avoid
entering the loop/. Alternatively, just don't even mention the loop -
just say "Quick return" etc.

~~~

43. WalSndWait

-WalSndWait(uint32 socket_events, long timeout, uint32 wait_event)
+WalSndWait(uint32 socket_events, long timeout, uint32 wait_event,
+    bool wait_for_standby)

Does this need the 'wait_for_standby' parameter? AFAICT this was only
set true when the event enum was
WAIT_EVENT_WAL_SENDER_WAIT_FOR_STANDBY_CONFIRMATION, so why do we need
an extra boolean to be passed when there is already enough information
in the event to know when it is waiting for standby?

~~~

44.
+ if (wait_for_standby)
+ ConditionVariablePrepareToSleep(&WalSndCtl->wal_confirm_rcv_cv);
+ else if (MyWalSnd->kind == REPLICATION_KIND_PHYSICAL)
  ConditionVariablePrepareToSleep(&WalSndCtl->wal_flush_cv);
  else if (MyWalSnd->kind == REPLICATION_KIND_LOGICAL)
  ConditionVariablePrepareToSleep(&WalSndCtl->wal_replay_cv);
~

A walsender is either physical or logical, but here the
'wait_for_standby' flag overrides everything. Is it OK for this to be
if/else/else or should this code call for wal_confirm_rcv_cv AND the
other one?

e.g. The function comment for WalSndWaitForWal said "the function also waits..."

======
src/backend/utils/misc/guc_tables.c

45.
+ {"standby_slot_names", PGC_SIGHUP, REPLICATION_PRIMARY,
+ gettext_noop("List of streaming replication standby server slot "
+ "names that logical walsenders waits for."),

/walsenders waits for./walsender processes will wait for./

======
src/backend/utils/misc/postgresql.conf.sample

46.
+#standby_slot_names = '' # streaming replication standby server slot names that
+ # logical walsenders waits for

(same as the msg in guc_tables)

/walsenders waits for/walsender processes will wait for/

======
src/bin/pg_upgrade/info.c

47.
@@ -681,6 +681,7 @@ get_old_cluster_logical_slot_infos(DbInfo *dbinfo,
bool live_check)
  int i_twophase;
  int i_caught_up;
  int i_invalid;
+ int i_failover;

~

IMO it would be better if all these were coded to use the same order
as the SQL -- so put each of the "failover" code immediately after the
'two_phase" code.

~~~

48.
@@ -689,6 +690,7 @@ get_old_cluster_logical_slot_infos(DbInfo *dbinfo,
bool live_check)
  i_twophase = PQfnumber(res, "two_phase");
  i_caught_up = PQfnumber(res, "caught_up");
  i_invalid = PQfnumber(res, "invalid");
+ i_failover = PQfnumber(res, "failover");

~

ditto #47.

~~~

49.
@@ -699,6 +701,7 @@ get_old_cluster_logical_slot_infos(DbInfo *dbinfo,
bool live_check)
  curr->two_phase = (strcmp(PQgetvalue(res, slotnum, i_twophase), "t") == 0);
  curr->caught_up = (strcmp(PQgetvalue(res, slotnum, i_caught_up), "t") == 0);
  curr->invalid = (strcmp(PQgetvalue(res, slotnum, i_invalid), "t") == 0);
+ curr->failover = (strcmp(PQgetvalue(res, slotnum, i_failover), "t") == 0);

~

ditto #47.
======
src/bin/pg_upgrade/pg_upgrade.c

50.
+ if (GET_MAJOR_VERSION(new_cluster.major_version) >= 1700)
+ appendPQExpBuffer(query, ", false, %s, %s);",
+   slot_info->two_phase ? "true" : "false",
+   slot_info->failover ? "true" : "false");
+ else
+ appendPQExpBuffer(query, ", false, %s);",
+   slot_info->two_phase ? "true" : "false");

IMO this would be easier to read if it was written the other way around like

if (GET_MAJOR_VERSION(new_cluster.major_version) < 1700)
  ... old args
else
  ... new args

======
src/bin/pg_upgrade/pg_upgrade.h

51.
+ bool failover; /* is the slot designated to be synced
+ * to the physical standby */
 } LogicalSlotInfo;

The comment is missing a question mark (?) which the others have.

======
src/bin/psql/describe.c

52.
    ", suborigin AS \"%s\"\n"
    ", subpasswordrequired AS \"%s\"\n"
-   ", subrunasowner AS \"%s\"\n",
+   ", subrunasowner AS \"%s\"\n"
+   ", subfailoverstate AS \"%s\"\n",
    gettext_noop("Origin"),
    gettext_noop("Password required"),
-   gettext_noop("Run as owner?"));
+   gettext_noop("Run as owner?"),
+   gettext_noop("Enable failover?"));

I didn't think "Enable failover?" should not have a question mark. IMO
"run as owner?" is the odd one out so should not have been copied.
Anyway,  the subfailoverstate is a 'state' rather than a simple
boolean, so it should be more like subtwophasestate than anything
else.

======
src/bin/psql/tab-complete.c

53.
  COMPLETE_WITH("binary", "connect", "copy_data", "create_slot",
    "disable_on_error", "enabled", "origin",
    "password_required", "run_as_owner", "slot_name",
-   "streaming", "synchronous_commit", "two_phase");
+   "streaming", "synchronous_commit", "two_phase",
+   "failover");

All these tab completion options are supposed to be in alphabetical
order, so this 'failover' has been added in the wrong position.

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

54.
/*
 * two_phase tri-state values. See comments atop worker.c to know more about
 * these states.
 */
#define LOGICALREP_TWOPHASE_STATE_DISABLED 'd'
#define LOGICALREP_TWOPHASE_STATE_PENDING 'p'
#define LOGICALREP_TWOPHASE_STATE_ENABLED 'e'

#define LOGICALREP_FAILOVER_STATE_DISABLED 'd'
#define LOGICALREP_FAILOVER_STATE_PENDING 'p'
#define LOGICALREP_FAILOVER_STATE_ENABLED 'e'

~

54a.
There should either be another comment (like the 'two_phase tri-state'
one) added for the FAILOVER states or that existing comment should be
expanded so that it also mentions the 'failover' tri-states.

~

54b.
Idea: If you are willing to change the constant names (not the values)
of the current tri-states then now both the 'two_phase' and 'failover'
could share them -- I also think this might give the ability to create
macros (if wanted) or to share more code instead of always handling
failover and two_phase separately.

SUGGESTION
#define LOGICALREP_TRISTATE_DISABLED 'd'
#define LOGICALREP_TRISTATE_PENDING 'p'
#define LOGICALREP_TRISTATE_ENABLED 'e'

~

54c.
The header comment at the top of worker.c should give more details
about the 'failover' tri-state. (also mentioned in another review
comment)

~~~

55. FormData_pg_subscription

+ char subfailoverstate; /* Enable Failover State */
+

/Enable Failover State/Failover state/

======
src/include/replication/slot.h

56.
+
+ /*
+ * Is this a failover slot (sync candidate for physical standbys)?
+ * Relevant for logical slots on the primary server.
+ */
+ bool failover;
 } ReplicationSlotPersistentData;

~

/Relevant/Only relevant/

======
src/include/replication/walreceiver.h

57.
+#define walrcv_create_slot(conn, slotname, temporary, two_phase,
failover, snapshot_action, lsn) \
+ WalReceiverFunctions->walrcv_create_slot(conn, slotname, temporary,
two_phase, failover,  snapshot_action, lsn)

double whitespace after the 'failover' parameter?

======
src/include/replication/walsender_private.h

58.
  ConditionVariable wal_flush_cv;
  ConditionVariable wal_replay_cv;

+ ConditionVariable wal_confirm_rcv_cv;

Should this new field have a comment? Or should it be grouped with the
2 preceding fields (if that same group comment is valid for all of
them)?

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

---

From: "Zhijie Hou (Fujitsu)" <[email-masked]>
Date: Mon, 27 Nov 2023 04:02:35 +0000

On Tuesday, November 21, 2023 1:39 PM Peter Smith <[email-masked]> wrote:

Hi,

Thanks for the comments.

> This attribute is very similar to the 'subtwophasestate' so IMO it would be
> better to be adjacent to that one in the docs.
> 
> (probably this means putting it in the same order in the catalog also, assuming
> that is allowed)

It's allowed, but I think the functionality of two fields are different and I didn’t find
the correlation between two fields except for the type of value. So I didn't change the order.

> 
> ~~~
> 
> 12. AlterSubscription
> 
> + if (sub->failoverstate == LOGICALREP_FAILOVER_STATE_ENABLED &&
> + opts.copy_data) ereport(ERROR,
> false, or with copy_data = false, or use DROP/CREATE SUBSCRIPTION.")));
> 
> ~
> 
> 12b.
> AFAIK when there are messages like this that differ only by non-translatable
> things ("failover" option) then that non-translatable thing should be extracted
> as a parameter so the messages are common.
> And, don't forget to add a /* translator: %s is a subscription option like
> 'failover' */ comment.
> 
> SUGGESTION like:
> when %s is enabled", "two_phase") errmsg("ALTER SUBSCRIPTION with refresh
> and copy_data is not allowed when %s is enabled", "failover")

I am not sure about changing the existing message here, I feel you can start a
separate thread to change the twophase related messages, and we can change accordingly
if it's accepted.

> + appendStringInfoString(&cmd, "FAILOVER"); if (use_new_options_syntax)
> + appendStringInfoString(&cmd, ", "); else appendStringInfoChar(&cmd, '
> + '); }
> 
> 15a.
> Isn't failover a new option that is unsupported pre-PG17? Why is it necessary to
> support an old-style syntax for something that was not supported on old
> servers? (I'm confused).
> 
> ~
> 
> 15b.

We don't support FAILOVER for old-style syntax and pre-PG17,
libpqrcv_create_slot is only building the replication command string and we
will add failover in the string so that the publisher will report errors if it
doesn't support these options ,the same is true for two_phase.

> /the definition/the failover state/

I kept this as it's a general function but we only
support changing failover state for now.

> +{  if (strcmp(*newval, "") == 0)  return true;
> +
> + /*
> + *newval); return false; }
> +
> + /* Now verify if the specified slots really exist and have correct
> + type */ if (!validate_standby_slots(newval)) return false;
> +
> Is it really necessary to have a special test for the special value "*" which you are
> going to reject? I don't see why this should be any different from checking for
> other values like "." or "$" or "?" etc.

SplitIdentifierString() does not give error for '*' and '*' can be considered
as valid value which if accepted can mislead user that all the standbys's slots
are now considered, which is not the case here. So we want to explicitly call
out this case i.e. '*' is not accepted as valid value for standby_slot_names.

> + /* No value is specified for standby_slot_names. */ if
> + (standby_slot_names_cpy == NULL) return;
> 
> Maybe this code also needs a similar elog and comment like already in this
> function:

This case is possible, standby_slot_names_cpy(e.g. extra pointer) is NULL if no
value("") is specified for the GUC.(see the code in check_standby_slot_names).

> IIUC, the 'extra' was unconditionally guc_strdup()'ed in the check hook, so
> should we also free it here before leaving this function?

No, as mentioned in src/backend/utils/misc/README, the space of extra
will be automatically freed when the associated GUC setting is no longer of interest.

> 
> ~~~
> 
> 31. GetStandbySlotList
> 
> +/*
> + * Return a copy of standby_slot_names_list if the copy flag is set to
> +true,
> Why is this better than just exposing the standby_slot_names_list. The caller
> can make a copy or not.

I think either way is fine, but I prefer not to add one global
variable if possible.

> + /* Log warning if no active_pid for this physical slot */ if
> + (slot->active_pid == 0) ereport(WARNING,
> 
> Other nearby code is guarding the slot in case it was NULL, so why not here? Is
> it a potential NPE?

I think it will not pass the check for restart_lsn before the active_pid if slot is NULL.

> Is this possible? Doesn't the function 'validate_standby_slots' called by the GUC
> hook prevent specifying logical slots in the GUC? Maybe this warning should be
> changed to Assert?

I think user could drop the logical slot and recreate a physical slot with the same name
without changing the GUC.

> If something was wrong with the slot that required a warning, is it really OK to
> remove this slot from the list? This seems contrary to the function comment
> which only talks about removing slots that have caught up.

I think it's OK to remove slots if it's invalidated, dropped, or was
changed to logical one as we don't need to wait for these slots to catch up anymore.

> + * standby servers because the standby_slot_names may have changed,
> + which
> IMO there is some missing information in this comment because it wasn't clear
> to me that calling WalSndFilterStandbySlots was going to side-efect that list to
> give it a different meaning. e.g. it seems it no longer means "standby slots" but
> instead means something like "standby slots that are not caught up". Perhaps
> that local variable can have a name that helps to convey that better?

I am not sure about this, WalSndFilterStandbySlots already indicates it will
filter the slot list which seems clear to me. But if you have better ideas, we can
adjust in next version.

> A walsender is either physical or logical, but here the 'wait_for_standby' flag
> overrides everything. Is it OK for this to be if/else/else or should this code call
> for wal_confirm_rcv_cv AND the other one?

No, we cannot prepare to sleep twice(see the comment in
ConditionVariablePrepareToSleep()).

> Idea: If you are willing to change the constant names (not the values) of the
> current tri-states then now both the 'two_phase' and 'failover'
> could share them -- I also think this might give the ability to create macros (if
> wanted) or to share more code instead of always handling failover and
> two_phase separately.
> 
> SUGGESTION

I am not sure about the idea, but if others also prefer this then
we can adjust the code.

~~~ 
On Wednesday, November 22, 2023 3:42 PM Peter Smith <[email-masked]> wrote:
> 6.
> +# The subscription that's up and running and is enabled for failover #
> +doesn't get the data from primary and keeps waiting for the # standby
> +specified in standby_slot_names.
> +$result = $subscriber1->safe_psql('postgres',
> + "SELECT count(*) = 0 FROM tab_int;");
> +is($result, 't', "subscriber1 doesn't get data from primary until
> standby1 acknowledges changes");
> 
> Might it be better to write as "SELECT count(*) = $primary_row_count FROM
> tab_int;" and expect it to return false?

Ensuring the number is 0 looks better to me.

Attach the V38 patch set which addressed all comments in [1][2]
except for the ones that mentioned above.

[1] https://www.postgresql.org/message-id/CAHut%2BPv-yu71ogj_hRi6cCtmD55bsyw7XTxj1Nq8yVFKpY3NDQ%40mail.gmail.com
[2] https://www.postgresql.org/message-id/CAHut%2BPuEGX5kr0xh06yv8ndoAQvDNedoec1OqOq3GMxDN6p%3D9A%40mail.gmail.com

Best Regards,
Hou zj

---

From: "Zhijie Hou (Fujitsu)" <[email-masked]>
Date: Mon, 27 Nov 2023 06:02:00 +0000

On Monday, November 27, 2023 12:03 PM Zhijie Hou (Fujitsu) <[email-masked]> wrote:
> 
> Attach the V38 patch set which addressed all comments in [1][2] except for the
> ones that mentioned above.
> 
> [1]
> https://www.postgresql.org/message-id/CAHut%2BPv-yu71ogj_hRi6cCtmD5
> 5bsyw7XTxj1Nq8yVFKpY3NDQ%40mail.gmail.com
> [2]
> https://www.postgresql.org/message-id/CAHut%2BPuEGX5kr0xh06yv8ndoA
> QvDNedoec1OqOq3GMxDN6p%3D9A%40mail.gmail.com

I didn't increment the patch version, sorry for that. Attach the same patch set
but increment the patch version to V39.

Best Regards,
Hou zj

---

From: "Drouvot, Bertrand" <[email-masked]>
Date: Mon, 27 Nov 2023 09:45:33 +0100

Hi,

> but increment the patch version to V39.

Thanks!

It looks like v39 does not contain (some / all?) the changes that have been
done in v38 [1].

For example, slot_exists_in_list() still exists in v39 while it was renamed to
validate_sync_slot() in v38.

[1]: https://www.postgresql.org/message-id/CAJpy0uD6dWUvBgy8MGdugf_Am4pLXTL_vqcwSeHO13v%2BMzc9KA%40mail.gmail.com

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

---

From: shveta malik <[email-masked]>
Date: Mon, 27 Nov 2023 14:20:47 +0530

> validate_sync_slot() in v38.

Yes, I noticed that and informed Hou-san about this. New patches will
be posted soon with the correction. Meanwhile, please review v38
instead if you intend to review patch002 right now.  v39 is supposed
to have changes in patch001 alone.

thanks
Shveta

thanks
Shveta

---

From: "Zhijie Hou (Fujitsu)" <[email-masked]>
Date: Mon, 27 Nov 2023 08:57:33 +0000

On Monday, November 27, 2023 4:51 PM shveta malik <[email-masked]> wrote:
> > > I didn't increment the patch version, sorry for that. Attach the
> > > same patch set but increment the patch version to V39.
> >
> > Thanks!
> >
> > It looks like v39 does not contain (some / all?) the changes that have
> > been done in v38 [1].
> >
> Yes, I noticed that and informed Hou-san about this. New patches will be
> posted soon with the correction. Meanwhile, please review v38 instead if you
> intend to review patch002 right now.  v39 is supposed to have changes in
> patch001 alone.

Here is the updated version(v39_2) which include all the changes made in 0002.
Please use for review, and sorry for the confusion.

Best Regards,
Hou zj

---

From: Amit Kapila <[email-masked]>
Date: Mon, 27 Nov 2023 16:08:09 +0530

> Please use for review, and sorry for the confusion.

--- a/src/backend/replication/logical/launcher.c
+++ b/src/backend/replication/logical/launcher.c
@@ -8,20 +8,27 @@
  *   src/backend/replication/logical/launcher.c
  *
  * NOTES
- *   This module contains the logical replication worker launcher which
- *   uses the background worker infrastructure to start the logical
- *   replication workers for every enabled subscription.
+ *   This module contains the replication worker launcher which
+ *   uses the background worker infrastructure to:
+ *   a) start the logical replication workers for every enabled subscription
+ *      when not in standby_mode.
+ *   b) start the slot sync worker for logical failover slots synchronization
+ *      from the primary server when in standby_mode.

I was wondering do we really need a launcher on standby to invoke
sync-slot worker. If so, why? I guess it may be required for previous
versions where we were managing work for multiple slot-sync workers
which is also questionable in the sense of whether launcher is the
right candidate for the same but now with the single slot-sync worker,
it doesn't seem worth having it. What do you think?

-- 
With Regards,
Amit Kapila.

---

From: shveta malik <[email-masked]>
Date: Tue, 28 Nov 2023 08:43:39 +0530

> +++ b/src/backend/replication/logical/launcher.c

Yes, earlier a manager process was needed to manage multiple slot-sync
workers and distribute load among them, but now that does not seem
necessary. I gave it a try (PoC) and it seems to work well.  If  there
are no objections to this approach, I can share the patch soon.

thanks
Shveta

---

From: "Drouvot, Bertrand" <[email-masked]>
Date: Tue, 28 Nov 2023 07:49:48 +0100

Hi,

> are no objections to this approach, I can share the patch soon.

+1 on this new approach, thanks!

Regards,

---

From: shveta malik <[email-masked]>
Date: Tue, 28 Nov 2023 15:10:21 +0530

> +1 on this new approach, thanks!

PFA v40. This patch has removed Logical Replication Launcher support
to launch slotsync worker.  The slot-sync worker is now registered as
bgworker with postmaster, with
bgw_start_time=BgWorkerStart_ConsistentState and
bgw_restart_time=60sec.

On removal of launcher, now all the validity checks have been shifted
to slot-sync worker itself.  This brings us to some point of concerns:

a) We still need to maintain  RecoveryInProgress() check in slotsync
worker. Since worker has the start time of
BgWorkerStart_ConsistentState, it will be started on non-standby as
well. So to ensure that it exists on non-standby, "RecoveryInProgress"
has been introduced at the beginning of the worker. But once it exits,
postmaster will not restart it since it will be clean-exist i.e.
proc_exit(0) (the restart logic of postmaster comes into play only
when there is an abnormal exit). But to exit for the first time on
non-standby, we need that Recovery related check in worker.

b) "enable_syncslot" check is moved to slotsync worker now. Since
enable_syncslot is PGC_SIGHUP, so proc_exit(1) is currently used to
exit the worker if 'enable_syncslot' is found to be disabled.
'proc_exit(1)' has been used in order to ensure that the worker is
restarted and GUCs are checked again after restart_time. Downside of
this approach is, if someone has kept "enable_syncslot" as disabled
permanently even on standby, slotsync worker will keep on restarting
and exiting.

So to overcome the above pain-points, I think a potential approach
will be to start slotsync worker only if 'enable_syncslot' is on and
the system is non-standby. Potential ways (each with some issues) are:

1) Use the current way i.e. register slot-sync worker as bgworker with
postmaster, but introduce extra checks in 'maybe_start_bgworkers'. But
this seems more like a hack. This will need extra changes as currently
once 'maybe_start_bgworkers' is attempted by postmaster, it will
attempt again to start any worker only if the worker had abnormal exit
and restart_time !=0. The current postmatser will not attempt to start
worker on any GUC change.

2) Another way maybe to treat slotsync worker as special case and
separate out the start/restart of slotsync worker from bgworker, and
follow what we do for autovacuum launcher(StartAutoVacLauncher) to
keep starting it in the postmaster loop(ServerLoop). In this way, we
may be able to add more checks before starting worker. But by opting
this approach, we will have to manage slotsync worker completely by
ourself as it will be no longer be part of existing
bgworker-registration infra. If this seems okay and there are no other
better options, it can be analyzed further in detail.

3) Another approach could be, in order to solve issue (a), introduce a
new start_time 'BgWorkerStart_ConsistentState_HotStandby' which means
start a bgworker only if consistent state is reached and the system is
standby. And for issue (b), lets retain check of enable_syncslot in
the worker itself but make it 'PGC_POSTMASTER'. This will ensure we
can safely exit the worker(proc_exit(0) if enable_syncslot is disabled
and postmaster will not restart it. But I'm not sure if making it
"PGC_POSTMASTER" is acceptable from the user's perspective.

thanks
Shveta

---

From: shveta malik <[email-masked]>
Date: Tue, 28 Nov 2023 15:14:08 +0530

> the system is non-standby. Potential ways (each with some issues) are:

Correction here:  start slotsync worker only if 'enable_syncslot' is
on and the system is standby.

---

From: "Drouvot, Bertrand" <[email-masked]>
Date: Tue, 28 Nov 2023 13:06:47 +0100

Hi,

Thanks!

As far v39_2-0001:

"
     Altering the failover option of the subscription is currently not
     permitted. However, this restriction may be lifted in future versions.
"

Should we mention that we can alter the related replication slot?

+         <para>
+          of <link linkend="catalog-pg-subscription"><structname>pg_subscription</structname></link>

I think we have a corner case here. If one alter the replication slot on the primary
then "subfailoverstate" is not updated accordingly on the subscriber. Given the 2 remarks above
would that make sense to prevent altering a replication slot associated to a subscription?

Regards,

---

From: "Drouvot, Bertrand" <[email-masked]>
Date: Tue, 28 Nov 2023 16:58:15 +0100

Hi,

On 11/28/23 10:40 AM, shveta malik wrote:
> to launch slotsync worker.

Thanks!

>  The slot-sync worker is now registered as
> the system is non-standby. 

That makes sense to me.

> Potential ways (each with some issues) are:
> 

I had the same idea (means make enable_syncslot as 'PGC_POSTMASTER')
when reading b). I'm +1 on it (at least for V1) as I don't think that
this parameter value would change frequently. Curious to know what others
think too.

Then as far a) is concerned, I'd vote for introducing a new
BgWorkerStart_ConsistentState_HotStandby.

Regards,

---

From: Peter Smith <[email-masked]>
Date: Wed, 29 Nov 2023 14:04:22 +1100

Hi. Here are some review comments for the patch v39_2-0001.

Multiple items from my previous review [1] seemed unanswered, so it
wasn't clear if they were discarded because they were wrong or maybe
accidently missed. I've repeated all those again here, as well as some
new comments.

======
1. General.

Previously (see [1] #0) I asked a question about if there is some
documentation missing. Seems not yet answered.

======
Commit message

2.
Users can set this flag during CREATE SUBSCRIPTION or during
pg_create_logical_replication_slot API. Examples:

CREATE SUBSCRIPTION mysub CONNECTION '..' PUBLICATION mypub
WITH (failover = true);

(failover is the last arg)
SELECT * FROM pg_create_logical_replication_slot('myslot',
'pgoutput', false, true, true);

~

I felt it is better to say "Ex1" / "Ex2" (or "1" / "2" or something
similar) to indicate better where these examples start and finish,
otherwise they just sort of get lost among the text.

======

3.
>From previous review ([1] #6) I suggested reordering fields. Hous-san
wrote: "but I think the functionality of two fields are different and
I didn’t find the correlation between two fields except for the type
of value."

Yes, that is true. OTOH, I felt grouping the attributes by the same
types made the docs easier to read.

======

4. CreateSubscription

+ /*
+ * If only the slot_name is specified (without create_slot option),
+ * it is possible that the user intends to use an existing slot on
+ * the publisher, so here we enable failover for the slot if
+ * requested.
+ */
+ else if (opts.slot_name && failover_enabled)
+ {

Unanswered question from previous review (see [1] #11a). i.e. How does
this condition ensure that *only* the slot name was specified (like
the comment is saying)?

~~~

5. AlterSubscription

  errmsg("ALTER SUBSCRIPTION with refresh and copy_data is not allowed
when two_phase is enabled"),
  errhint("Use ALTER SUBSCRIPTION ... SET PUBLICATION with refresh =
false, or with copy_data = false, or use DROP/CREATE
SUBSCRIPTION.")));

There are translations issues same as reported in my previous review
(see [1] #12b and also several other places as noted in [1]). Hou-san
replied that I "can start a separate thread to change the twophase
related messages, and we can change accordingly if it's accepted.",
but that's not right IMO because it is only the fact that this
sysncslot patch is reusing a similar message that warrants the need to
extract a "common" message part in the first place. So I think it is
responsibility if this sycslot patch to make this change.

======

6. process_syncing_tables_for_apply

+ if (MySubscription->twophasestate == LOGICALREP_TWOPHASE_STATE_PENDING)
+ ereport(LOG,
+ (errmsg("logical replication apply worker for subscription \"%s\"
will restart so that two_phase can be enabled",
+ MySubscription->name)));
+
+ if (MySubscription->failoverstate == LOGICALREP_FAILOVER_STATE_PENDING)
+ ereport(LOG,
+ (errmsg("logical replication apply worker for subscription \"%s\"
will restart so that failover can be enabled",
+ MySubscription->name)));

6a.
You may end up log 2 restart messages for the same restart. Is it OK?

~

6b.
This is another example where you should share the same common message
(for less translations)

======

7.
+ * The logical slot on the primary can be synced to the standby by specifying
+ * the failover = true when creating the subscription. Enabling failover allows
+ * us to smoothly transition to the standby in case the primary gets promoted,
+ * ensuring that we can subscribe to the new primary without losing any data.

/the failover = true/the failover = true option/

or

/the failover = true/failover = true/

~~~

8.
+
 #include "postgres.h"

Unnecessary extra blank line

======

9. validate_standby_slots

There was no reply to the comment in my previous review (see [1] #27).
Maybe you disagree or maybe accidentally overlooked?

~~~

10. check_standby_slot_names

In previous review I asked ([1] #28) why a special check was needed
for "*". Hou-san replied that "SplitIdentifierString() does not give
error for '*' and '*' can be considered as valid value which if
accepted can mislead user".

Sure, but won't the code then just try to find if there is a
replication slot called "*" and that will fail. That was my point, if
the slot name lookup is going to fail anyway then why have the extra
code for the special "*" case up-front? Note -- I haven't tried it, so
maybe code doesn't work like I think it does.

======

11. PhysicalWakeupLogicalWalSnd

No reply to my previous review comment ([1] #33). Not done? Disagreed,
or accidentally missed?

~~~

12. WalSndFilterStandbySlots

+ /*

I previously raised an issue (see [1] #35) thinking this could not
happen. Hou-san explained how it might happen ("user could drop the
logical slot and recreate a physical slot with the same name without
changing the GUC.") so this code was necessary. That is OK, but I
think your same explanation in the code commen.

~~~

13. WalSndFilterStandbySlots

I previously raised issue (see [1] #36). Hou-san replied "I think it's
OK to remove slots if it's invalidated, dropped, or was changed to
logical one as we don't need to wait for these slots to catch up
anymore."

Sure, maybe code is fine, but my point was that the code is removing
elements *more* scenarios than are mentioned by the function comment,
so maybe update that function comment for all the removal scenarios.

~~~

14. WalSndWaitForStandbyConfirmation

The comment change from my previous review ([1] #37) not done.
Disagreed, or accidentally missed?

~~~

15. WalSndWaitForStandbyConfirmation

The question about calling ConditionVariablePrepareToSleep in my
previous review ([1] #39) not answered. Accidentally missed?

~~~

16. WalSndWaitForWal

It is better to use XLogRecPtrIsInvalid macro here. I know it was not
strictly added by your patch, but so much else changed nearby so I
thought this should be fixed at the same time.

======

17. get_old_cluster_logical_slot_infos

+
  slotinfos = (LogicalSlotInfo *) pg_malloc(sizeof(LogicalSlotInfo) *
num_slots);

Excessive whitespace.

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

Kind Regards,
Peter Smith.
Fujitsu Australia

---

From: shveta malik <[email-masked]>
Date: Wed, 29 Nov 2023 09:47:58 +0530

> BgWorkerStart_ConsistentState_HotStandby.

+1 on PGC_POSTMASTER and BgWorkerStart_ConsistentState_HotStandby. A
clean solution as compared to the rest of the approaches. Will
implement it.

thanks
Shveta

---

From: "Zhijie Hou (Fujitsu)" <[email-masked]>
Date: Wed, 29 Nov 2023 05:58:06 +0000

On Tuesday, November 28, 2023 8:07 PM Drouvot, Bertrand <[email-masked]> wrote:

Hi,

> Should we mention that we can alter the related replication slot?

Will add.

> +          completes. See column
> <structfield>subfailoverstate</structfield>
> +          of <link
> I think we have a corner case here. If one alter the replication slot on the
> primary then "subfailoverstate" is not updated accordingly on the subscriber.
> Given the 2 remarks above would that make sense to prevent altering a
> replication slot associated to a subscription?

Thanks for the review!

I think we could not distinguish the user created logical slot or subscriber
created slot as there is no related info in slot's data. And user could change
the slot on subscription by "alter sub set (slot_name)", so maintaining this info
would need some efforts.

Besides, I think this case overlaps the previous discussed "alter sub set
(slot_name)" issue[1]. Both the cases are because the slot's failover is
different from the subscription's failover setting. I think we could handle
them similarly that user need to take care of not changing the failover to
wrong value. Or do you prefer another approach that mentioned in that thread[1]
? (always alter the slot at the startup of apply worker).

[1] https://www.postgresql.org/message-id/564b195a-180c-42e9-902b-b1a8b50218ee%40gmail.com

Best Regards,
Hou zj

---

From: "Zhijie Hou (Fujitsu)" <[email-masked]>
Date: Wed, 29 Nov 2023 09:11:30 +0000

On Tuesday, November 28, 2023 11:58 PM Drouvot, Bertrand <[email-masked]> wrote:
> >>>> sync-slot worker. If so, why? I guess it may be required for
> >>>> previous versions where we were managing work for multiple
> >>>> slot-sync workers which is also questionable in the sense of
> >>>> whether launcher is the right candidate for the same but now with
> >>>> the single slot-sync worker, it doesn't seem worth having it. What do you
> think?
> >>>>
> >>>> --
> >>>
> >>> Yes, earlier a manager process was needed to manage multiple
> >>> slot-sync workers and distribute load among them, but now that does
> >>> not seem necessary. I gave it a try (PoC) and it seems to work well.
> >>> If  there are no objections to this approach, I can share the patch soon.
> >>>
> >>
> >  The slot-sync worker is now registered as bgworker with postmaster,
> > with bgw_start_time=BgWorkerStart_ConsistentState and
> > bgw_restart_time=60sec.
> >
> when reading b). I'm +1 on it (at least for V1) as I don't think that this parameter
> value would change frequently. Curious to know what others think too.
> 

Here is the V41 patch set which includes the following changes.

V41-0001:
1) Based on the discussion[1], I update the document to remind user to
change the slot's failover option when ALTER SUBSCRIPTION ... SET
(slot_name = xx).

2) Address comments in [2][3][4].

V41-0002:
1) 'enable_syncslot' is changed from PGC_SIGHUP to PGC_POSTMASTER,
slot-sync worker will now clean exit (proc_exit(0)) if enable_syncslot is
found disabled.

2) BgWorkerStart_ConsistentState_HotStandby is introduced as new
start-time for bgworker. This will start worker only if it is standby_mode
and consistent state is reached.

3) 'SYNCSLOT_STATE_INITIATED' is now set in 'ReplicationSlotCreate' itself
in slot-sync worker case. Earlier it was set at later point of time giving
a window wherein even a synced slot was in 'n' state for quite some time,
which was not correct.

Thanks Shveta for working on the V41-0002.

[1] https://www.postgresql.org/message-id/CAA4eK1Jd9dk%3D5POTKM9p4EyYqYzLXe-AnLzHrUELjzZScLz7mw%40mail.gmail.com
[2] https://www.postgresql.org/message-id/eb09f682-db82-41cd-93bc-5d44e10e1d6d%40gmail.com
[3] https://www.postgresql.org/message-id/CAHut%2BPsuSWjm7U_sVnL8FXZ7ZQcfCcT44kAK7i6qMG35Cwjy3A%40mail.gmail.com
[4] https://www.postgresql.org/message-id/CAFPTHDbFqLgXS6Et%2BshNGPDjCKK66C%2BZSarqFHmQvfnAah3Qsw%40mail.gmail.com

Best Regards,
Hou zj

---

From: "Zhijie Hou (Fujitsu)" <[email-masked]>
Date: Wed, 29 Nov 2023 09:12:54 +0000

On Wednesday, November 29, 2023 11:04 AM Peter Smith <[email-masked]> wrote:

Thanks for the comments.

> ======
> 1. General.
> 
> Previously (see [1] #0) I asked a question about if there is some documentation
> missing. Seems not yet answered.

The document was add in V39-0002 in logicaldecoding.sgml
because some necessary GUCs for slotsync are not in 0001.

> CREATE SUBSCRIPTION mysub CONNECTION '..' PUBLICATION mypub WITH
> (failover = true);
> 
> similar) to indicate better where these examples start and finish, otherwise they
> just sort of get lost among the text.

Changed.

> wrote: "but I think the functionality of two fields are different and I didn’t find
> the correlation between two fields except for the type of value."
> 
> Yes, that is true. OTOH, I felt grouping the attributes by the same types made
> the docs easier to read.

The document's order should be same as the pg_subscription catalog, and I
prefer not to move the new subfailoverstate in the middle of catalog as the
functionality of them is different.

> + else if (opts.slot_name && failover_enabled) {
> 
> Unanswered question from previous review (see [1] #11a). i.e. How does this
> condition ensure that *only* the slot name was specified (like the comment is
> saying)?

It is the else part of 'if (opts.create_slot)', so it means create_slot is not
specified while only slot_name is specified. I have improved the comment.

>   errhint("Use ALTER SUBSCRIPTION ... SET PUBLICATION with refresh = false,
> or with copy_data = false, or use DROP/CREATE SUBSCRIPTION.")));
> 
> There are translations issues same as reported in my previous review (see [1]
> #12b and also several other places as noted in [1]). Hou-san replied that I "can
> start a separate thread to change the twophase related messages, and we can
> change accordingly if it's accepted.", but that's not right IMO because it is only
> the fact that this sysncslot patch is reusing a similar message that warrants the
> need to extract a "common" message part in the first place. So I think it is

OK, changed.

> + if (MySubscription->twophasestate ==
> + LOGICALREP_TWOPHASE_STATE_PENDING)
> + ereport(LOG,
> + if (MySubscription->failoverstate ==
> + LOGICALREP_FAILOVER_STATE_PENDING)
> + ereport(LOG,

I think it's OK as it can provide complete information.

> This is another example where you should share the same common message

I adjusted the message there.

> + * The logical slot on the primary can be synced to the standby by
> + specifying
> + * the failover = true when creating the subscription. Enabling
> + failover allows
> + * us to smoothly transition to the standby in case the primary gets
> + promoted,

Changed.

> Unnecessary extra blank line

Removed.

> Maybe you disagree or maybe accidentally overlooked?

The error message has already been adjusted in V39.
I adjusted the check in this version as well to be consistent.

> In previous review I asked ([1] #28) why a special check was needed for "*".
> Hou-san replied that "SplitIdentifierString() does not give error for '*' and '*'
> can be considered as valid value which if accepted can mislead user".
> 
> Sure, but won't the code then just try to find if there is a replication slot called
> "*" and that will fail. That was my point, if the slot name lookup is going to fail
> anyway then why have the extra code for the special "*" case up-front? Note --
> I haven't tried it, so maybe code doesn't work like I think it does.

I think allowing "*" can mislead user because it normally means every slot, but
we don't want to support the "every slot" option as mentioned in the comment.
So I think reject it here is fine. Reporting ERROR because the slot named '*' was not
there may look confusing.

> No reply to my previous review comment ([1] #33). Not done? Disagreed, or
> accidentally missed?

The function mentioned in your previous comment has been removed in
previous version, so I am not sure are you pointing to some other codes
that has similar issues ?

> I previously raised an issue (see [1] #35) thinking this could not happen.
> Hou-san explained how it might happen ("user could drop the logical slot and
> recreate a physical slot with the same name without changing the GUC.") so this
> code was necessary. That is OK, but I think your same explanation in the code
> commen.

OK, I have added comments here.

> I previously raised issue (see [1] #36). Hou-san replied "I think it's OK to remove
> slots if it's invalidated, dropped, or was changed to logical one as we don't
> need to wait for these slots to catch up anymore."
> 
> Sure, maybe code is fine, but my point was that the code is removing elements
> *more* scenarios than are mentioned by the function comment, so maybe
> update that function comment for all the removal scenarios.

Updated the comments.

> Disagreed, or accidentally missed?

Thanks for pointing, this was missed.

> The question about calling ConditionVariablePrepareToSleep in my previous
> review ([1] #39) not answered. Accidentally missed?

I think V39 has already adjusted the order of reload and NIL check in this function.

> It is better to use XLogRecPtrIsInvalid macro here. I know it was not strictly
> added by your patch, but so much else changed nearby so I thought this should
> be fixed at the same time.

Changed.

>   slotinfos = (LogicalSlotInfo *) pg_malloc(sizeof(LogicalSlotInfo) * num_slots);
> 
> Excessive whitespace.

Removed.

Best Regards,
Hou zj

---

From: "Drouvot, Bertrand" <[email-masked]>
Date: Wed, 29 Nov 2023 10:54:45 +0100

Hi,

On 11/29/23 6:58 AM, Zhijie Hou (Fujitsu) wrote:
> created slot as there is no related info in slot's data.

Yeah that would need extra work.

> And user could change

Yes.
  
> different from the subscription's failover setting.

Yeah agree.

> I think we could handle

I think I'm fine with documenting the fact that the user should not change the failover
value. But if he does change it (because at the end nothing prevents it to do so) then
I think the meaning of subfailoverstate should still make sense.

One way to achieve this could be to change its meaning? Say rename it to
say subfailovercreationstate (to reflect the fact that it was the state at the creation time)
and change messages like:

"
ALTER SUBSCRIPTION with refresh and copy_data is not allowed when failover is enabled
"

to something like

"
ALTER SUBSCRIPTION with refresh and copy_data is not allowed for subscription created with failover enabled"
"

and change the doc accordingly.

What do you think?

Regards,

---

From: Peter Smith <[email-masked]>
Date: Thu, 30 Nov 2023 18:05:29 +1100

Here are some review comments for v41-0001.

======

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

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.
+ /*

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)

======

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.

Kind Regards,
Peter Smith.
Fujitsu Australia

---

From: "Zhijie Hou (Fujitsu)" <[email-masked]>
Date: Fri, 1 Dec 2023 04:10:21 +0000

On Wednesday, November 29, 2023 5:12 PM Zhijie Hou (Fujitsu) <[email-masked]> wrote:

I was reviewing slotsync worker design and here
are few comments on 0002 patch:

1.

+	if (!WalRcv ||
+		(WalRcv->slotname[0] == '\0') ||
+		XLogRecPtrIsInvalid(WalRcv->latestWalEnd))

I think we'd better take spinlock when accessing these shared memory fields.

2.

	/*
	 * The slot sync feature itself is disabled, exit.
	 */
	if (!enable_syncslot)
	{
		ereport(LOG,
				errmsg("exiting slot sync worker as enable_syncslot is disabled."));

Can we check the GUC when registering the worker(SlotSyncWorkerRegister),
so that the worker won't be started if enable_syncslot is false.

3. In synchronize_one_slot, do we need to skip the slot sync and drop if the
local slot is a physical one ?

4.

			*locally_invalidated =
				(remote_slot->invalidated == RS_INVAL_NONE) &&
				(local_slot->data.invalidated != RS_INVAL_NONE);

When reading the invalidated flag of local slot, I think we'd better take
spinlock.

Best Regards,
Hou zj

---

From: shveta malik <[email-masked]>
Date: Fri, 1 Dec 2023 10:21:03 +0530

> are few comments on 0002 patch:

Thanks for reviewing the patch.

> local slot is a physical one ?

IMO, if a local slot exists which is a physical one, it will be a user
created slot and in that case worker will error out on finding
existing slot with same name. And the case where local slot is
physical one but not user-created is not possible on standby (assuming
we have correct check on primary disallowing setting 'failover'
property for physical slot). Do you have some other scenario in mind,
which I am missing here?

---

From: "Zhijie Hou (Fujitsu)" <[email-masked]>
Date: Fri, 1 Dec 2023 05:47:29 +0000

On Friday, December 1, 2023 12:51 PM shveta malik <[email-masked]> wrote:

Hi,

> property for physical slot). Do you have some other scenario in mind,

I was thinking about the race condition when it has confirmed that the slot is
not a user created one and enter "sync_state == SYNCSLOT_STATE_READY" branch,
but at this moment, if someone uses "DROP_REPLICATION_SLOT" to drop this slot and
recreate another one(e.g. a physical one), then the slotsync worker will
overwrite the fields of this physical slot. Although this affects user created
logical slots in similar cases as well.

And the same is true for slotsync_drop_initiated_slots() and
drop_obsolete_slots(), as we don't lock the slots in the list, if user tri to
drop and re-create old slot concurrently, then we could drop user created slot
here.

Best Regards,
Hou zj

---

From: shveta malik <[email-masked]>
Date: Fri, 1 Dec 2023 12:47:11 +0530

> logical slots in similar cases as well.

User can not  drop the synced slots on standby. It should result in
ERROR. Currently we emit this error in pg_drop_replication_slot(),
same is needed in  "DROP_REPLICATION_SLOT" replication cmd. I will
change it. Thanks for raising this point.  I think, after this ERROR,
there is no need to worry about physical slots handling in
synchronize_one_slot().

---

From: shveta malik <[email-masked]>
Date: Fri, 1 Dec 2023 15:03:33 +0530

> > drop and re-create old slot concurrently, then we could drop user created slot

PFA v42. Changes:

v42-0001: addressed comments in [1]. Thanks Hou-San for working on this.

v42-0002: addressed comments in [2] and [3]

[1]: https://www.postgresql.org/message-id/CAHut%2BPsMTvrwUBtcHff0CG_j-ALSuEta8xC1R_k0kjR%2B9A6ehg%40mail.gmail.com
[2]: https://www.postgresql.org/message-id/CAFPTHDb8LW4i9-nyvz%2BXVkJmmciZwYGivpH%3DaDOrDkBfHR_q9w%40mail.gmail.com
[3]: https://www.postgresql.org/message-id/OS0PR01MB571678BABEDBE830062CAB119481A%40OS0PR01MB5716.jpnprd01.prod.outlook.com

thanks
Shveta

---

From: Amit Kapila <[email-masked]>
Date: Fri, 1 Dec 2023 16:36:05 +0530

> I think the meaning of subfailoverstate should still make sense.

How user can change the slot's failover property? Do we provide any
command for it?

-- 
With Regards,
Amit Kapila.

---

From: "Drouvot, Bertrand" <[email-masked]>
Date: Fri, 1 Dec 2023 12:15:16 +0100

Hi,

> How user can change the slot's failover property? Do we provide any

It's doable, using a replication connection:

"
$ psql replication=database
psql (17devel)
Type "help" for help.

postgres=# ALTER_REPLICATION_SLOT logical_slot6 (FAILOVER false);
ALTER_REPLICATION_SLOT
"

Regards,

---

From: Nisha Moond <[email-masked]>
Date: Fri, 1 Dec 2023 17:40:33 +0530

Review for v41 patch.

1.
======

+#enable_syncslot = on # enables slot synchronization on the physical
standby from the primary

enable_syncslot is disabled by default, so, it should be 'off' here.

~~~
2.
IIUC, the slotsyncworker's connection to the primary is to execute a
query. Its aim is not walsender type connection, but at primary when
queried, the 'backend_type' is set to 'walsender'.
Snippet from primary db-

datname  |   usename   | application_name | wait_event_type | backend_type
---------+-------------+------------------+-----------------+--------------
postgres | replication | slotsyncworker   | Client          | walsender

Is it okay?

~~~
3.
As per current logic, If there are slots on primary with disabled
subscriptions, then, when standby is created it replicates these slots
but can't make them sync-ready until any activity happens on the
slots.
So, such slots stay in 'i' sync-state and get dropped when failover
happens. Now, if the subscriber tries to enable their existing
subscription after failover, it gives an error that the slot does not
exist.

~~~
4. primary_slot_name GUC value test:

When standby is started with a non-existing primary_slot_name, the
wal-receiver gives an error but the slot-sync worker does not raise
any error/warning. It is no-op though as it has a check 'if
(XLogRecPtrIsInvalid(WalRcv->latestWalEnd)) do nothing'.   Is this
okay or shall the slot-sync worker too raise an error and exit?

In another case, when standby is started with valid primary_slot_name,
but it is changed to some invalid value in runtime, then walreceiver
starts giving error but the slot-sync worker keeps on running. In this
case, unlike the previous case, it even did not go to no-op mode (as
it sees valid WalRcv->latestWalEnd from the earlier run) and keep
pinging primary repeatedly for slots.  Shall here it should error out
or at least be no-op until we give a valid primary_slot_name?

--
Thanks,
Nisha

---

From: Nisha Moond <[email-masked]>
Date: Fri, 1 Dec 2023 21:30:59 +0530

> datname  |   usename   | application_name | wait_event_type | backend_type

Is this behavior expected? If yes, then is it worth documenting about
disabled subscription slots not being synced?

--
Thanks,
Nisha

---

From: Amit Kapila <[email-masked]>
Date: Sat, 2 Dec 2023 09:49:03 +0530

> disabled subscription slots not being synced?

This is expected behavior because even if we would retain such slots
(state 'i'), we won't be able to make them in the 'ready' state after
failover because we can't get the required WAL from the primary.

-- 
With Regards,
Amit Kapila.

---

From: "Zhijie Hou (Fujitsu)" <[email-masked]>
Date: Mon, 4 Dec 2023 03:33:36 +0000

On Wednesday, November 29, 2023 5:55 PM Drouvot, Bertrand <[email-masked]> wrote:

Hi,

> >>> Here is the updated version(v39_2) which include all the changes
> >>> made in
> >> 0002.
> >> +          temporarily <quote>pending</quote> until the
> >> + initialization
> >> phase
> >> +          completes. See column
> linkend="catalog-pg-subscription"><structname>pg_subscription</struct
> >> na
> >> me></link>
> >> I think we have a corner case here. If one alter the replication slot
> >> on the primary then "subfailoverstate" is not updated accordingly on the
> subscriber.
> > I think we could not distinguish the user created logical slot or
> > subscriber created slot as there is no related info in slot's data.
> 
> > the slot on subscription by "alter sub set (slot_name)", so
> > maintaining this info would need some efforts.
> >
> 
> Yes.
> 
> > Besides, I think this case overlaps the previous discussed "alter sub
> > set (slot_name)" issue[1]. Both the cases are because the slot's
> > failover is different from the subscription's failover setting.
> 
> Yeah agree.
> 
> > I think we could handle
> > them similarly that user need to take care of not changing the
> > failover to wrong value. Or do you prefer another approach that
> > mentioned in that thread[1] ? (always alter the slot at the startup of apply
> worker).
> >
> 
> I think I'm fine with documenting the fact that the user should not change the
> failover value. But if he does change it (because at the end nothing prevents it
> to do so) then I think the meaning of subfailoverstate should still make sense.
> 
> One way to achieve this could be to change its meaning? Say rename it to say
> subfailovercreationstate (to reflect the fact that it was the state at the creation
> time) and change messages like:
> 
> "
> ALTER SUBSCRIPTION with refresh and copy_data is not allowed when failover
> is enabled "
> 
> to something like
> 
> "
> ALTER SUBSCRIPTION with refresh and copy_data is not allowed for
> subscription created with failover enabled"
> "
> 

This idea may work for now, but I think we planned to support ALTER
SUBSCRIPTION (failover) in a later patch, which means the meaning of
subfailovercreationstate may be invalid after that because we will be able to
change this value using ALTER SUBSCRIPTION as well.

I think document the case is OK because:

Currently, user already can create similar inconsistency cases as we don't restrict
user to change the slot on publisher. E.g., User could drop and recreate the
slot used by subscription but with different setting. Or user ALTER
SUBSCRIPTION set (slot_name) to switch to a new slot with different setting.

For example, about two_phase option, user can create a subscription with
two_phase disabled, then later it can set subscription slot_name to a new slot
with two_phase enabled which is the similar case as the failover.

Best Regards,
Hou zj

---

From: shveta malik <[email-masked]>
Date: Mon, 4 Dec 2023 10:40:33 +0530

> Review for v41 patch.

Thanks for the feedback.

> enable_syncslot is disabled by default, so, it should be 'off' here.

Sure, I will change it.

> datname  |   usename   | application_name | wait_event_type | backend_type

Slot sync worker uses 'libpqrcv_connect' for connection which sends
'replication'-'database' key-value pair as one of the connection
options. And on the primary side, 'ProcessStartupPacket' on the basis
of this key-value pair sets the process as walsender one (am_walsender
= true).
And thus this reflects as backend_type='walsender' in
pg_stat_activity. I do not see any harm in this backend_type for
slot-sync worker currently. This is on a similar line of connections
used for logical-replications. And since a slot-sync worker also deals
with wals-positions (lsns), it is okay to maintain backend_type as
walsender unless you (or others) see any potential issue in doing
that. So let me know.

> subscription after failover, it gives an error that the slot does not

yes, this is expected as Amit explained in [1]. But let me review if
we need to document this case for disabled subscriptions. i.e.
disabled subscription if enabled after promotion might not work.

> or at least be no-op until we give a valid primary_slot_name?

 I reviewed it. There is no way to test the existence/validity of
'primary_slot_name' on standby without making a connection to primary.
If primary_slot_name is invalid from the start, slot-sync worker will
be no-op (as you tested) as WalRecv->latestWalENd will be invalid, and
if 'primary_slot_name' is changed to invalid on runtime, slot-sync
worker will still keep on pinging primary. But that should be okay (in
fact needed) as it needs to sync at-least the previous slot's
positions (in case it is delayed in doing so for some reason earlier).
And once the slots are up-to-date on standby, even if worker pings
primary, it will not see any change in slots lsns and thus go for
longer nap. I think, it is not worth the effort to introduce the
complexity of checking validity of 'primary_slot_name' on primary from
standby for this rare scenario.

It will be good to know thoughts of others on above 3 points.

thanks
Shveta

---

From: shveta malik <[email-masked]>
Date: Mon, 4 Dec 2023 10:45:18 +0530

> disabled subscription if enabled after promotion might not work.

Sorry, missed to mention the link earlier:
[1]: https://www.postgresql.org/message-id/CAA4eK1J5Hxp%2BzhvptyyjqQ4JSQzwnkFRXtQn8v9opxtZmmY_Ug%40mail.gmail.com

---

From: "Drouvot, Bertrand" <[email-masked]>
Date: Mon, 4 Dec 2023 15:37:48 +0100

Hi,

> with two_phase enabled which is the similar case as the failover.

Yeah, right, did not think that such "inconsistency" can already happen.

So agree to keep "subfailoverstate" and "just" document the case then.

Regards,

---

From: "Drouvot, Bertrand" <[email-masked]>
Date: Mon, 4 Dec 2023 17:36:56 +0100

Hi,

> walsender unless you (or others) see any potential issue in doing

I don't see any issue as well (though I understand it might
seems weird to see a walsender process being spawned doing non
replication stuff)

>> or at least be no-op until we give a valid primary_slot_name?

Nice catch, thanks!

> standby for this rare scenario.

Maybe another option could be to have the walreceiver a way to let the slot sync
worker knows that it (the walreceiver) was not able to start due to non existing
replication slot on the primary? (that way we'd avoid the slot sync worker having
to talk to the primary).

Not sure about the extra effort to make it works though.

Regards,

---

From: shveta malik <[email-masked]>
Date: Tue, 5 Dec 2023 10:38:13 +0530

> replication slot on the primary? (that way we'd avoid the slot sync worker having

Few points:
1) I think if we do it, we should do it in generic way i.e. slotsync
worker should go to no-op if walreceiver is not able to start due to
any reason and not only due to invalid primary_slot_name.
2) Secondly, slotsync worker needs to make sure it has synced the
slots so far i.e. worker should not go to no-op immediately on seeing
missing WalRcv process if there are pending slots to be synced.

So the generic way I see to have this optimization is:
1) Slotsync worker can use 'WalRcv->pid' to figure out if WalReceiver
is running or not.
2) Slotsync worker should check null 'WalRcv->pid' only when
no-activity is observed for threshold time i.e. it can do it during
existing logic of increasing naptime.
3) On finding null  'WalRcv->pid', worker can mark a flag to go to
no-op unless WalRcv->pid becomes valid again. Marking this flag during
increasing naptime will guarantee that the worker has taken all the
changes so far i.e. standby is not lagging in terms of slots.

Thoughts?

thanks
Shveta

---

From: "Drouvot, Bertrand" <[email-masked]>
Date: Tue, 5 Dec 2023 09:48:34 +0100

Hi,

> any reason and not only due to invalid primary_slot_name.

Agree.

> missing WalRcv process if there are pending slots to be synced.

Agree.

> 1) Slotsync worker can use 'WalRcv->pid' to figure out if WalReceiver

Not sure that would work because the walreceiver keeps try re-starting
and so get a pid before reaching the "could not start WAL streaming: ERROR:  replication slot "XXXX" does not exist"
error.

We may want to add an extra check on walrcv->walRcvState (or should/could be enough by its own).
But walrcv->walRcvState is set to WALRCV_STREAMING way before walrcv_startstreaming().

Wouldn't that make sense to move it once we are sure that
walrcv_startstreaming() returns true and first_stream is true, here?

"
                         if (first_stream)
+                       {
                                 ereport(LOG,
                                                 (errmsg("started streaming WAL from primary at %X/%X on timeline %u",
                                                                 LSN_FORMAT_ARGS(startpoint), startpointTLI)));
+                               SpinLockAcquire(&walrcv->mutex);
+                               walrcv->walRcvState = WALRCV_STREAMING;
+                               SpinLockRelease(&walrcv->mutex);
+                       }
"

> changes so far i.e. standby is not lagging in terms of slots.

2) and 3) looks good to me but with a check on walrcv->walRcvState
looking for WALRCV_STREAMING state instead of looking for a non null
WalRcv->pid.

And only if it makes sense to move the walrcv->walRcvState = WALRCV_STREAMING as
mentioned above (I think it does).

Thoughts?

Regards,

---

From: shveta malik <[email-masked]>
Date: Tue, 5 Dec 2023 15:59:25 +0530

> and so get a pid before reaching the "could not start WAL streaming: ERROR:  replication slot "XXXX" does not exist"

yes, right. pid will keep on toggling.

> But walrcv->walRcvState is set to WALRCV_STREAMING way before walrcv_startstreaming().

Agree. Check on 'walrcv->walRcvState' alone should suffice.

> +                               SpinLockRelease(&walrcv->mutex);

Yes, it makes sense and is the basis for current slot-sync worker
changes being discussed.

> looking for WALRCV_STREAMING state instead of looking for a non null

yes. But I think, the worker should enter no-op, when walRcvState is
WALRCV_STOPPED and not when walRcvState != WALRCV_STREAMING as it is
okay to have WALRCV_WAITING/STARTING/RESTARTING. But the worker should
exit no-op only when it finds walRcvState switched back to
WALRCV_STREAMING.

> mentioned above (I think it does).

yes, I agree.

thanks
Shveta

---

From: Amit Kapila <[email-masked]>
Date: Tue, 5 Dec 2023 17:02:49 +0530

> missing WalRcv process if there are pending slots to be synced.

Won't it be better to just ping and check the validity of
'primary_slot_name' at the start of slot-sync and if it is changed
anytime? I think it would be better to avoid adding dependency on
walreciever state as that sounds like needless complexity.

-- 
With Regards,
Amit Kapila.

---

From: "Drouvot, Bertrand" <[email-masked]>
Date: Tue, 5 Dec 2023 14:38:12 +0100

Hi,

> Yes, it makes sense and is the basis for current slot-sync worker

I think this change deserves its own dedicated thread and patch, does
that make sense?

If so, I'll submit one.

> exit no-op only when it finds walRcvState switched back to

Yeah, fully agree.

Regards,

---

From: "Drouvot, Bertrand" <[email-masked]>
Date: Tue, 5 Dec 2023 15:08:33 +0100

Hi,

> walreciever state as that sounds like needless complexity.

I think the overall extra complexity is linked to the fact that we first
want to ensure that the slots are in sync before shutting down the
sync slot worker.

I think than talking to the primary or relying on the walreceiver state
is "just" what would trigger the decision to shutdown the sync slot worker.

Relying on the walreceiver state looks better to me (as it avoids possibly
useless round trips with the primary).

Also the walreceiver could be down for multiple reasons, and I think there
is no point of having a sync slot worker running if the slots are in sync and
there is no walreceiver running (even if primary_slot_name is a valid one).

That said, I'm also ok with the "ping primary" approach if others have another
point of view and find it better.

Regards,

---

From: Amit Kapila <[email-masked]>
Date: Wed, 6 Dec 2023 10:56:47 +0530

> useless round trips with the primary).

But the round trip will only be once in the beginning and if the user
changes the GUC primary-slot_name which shouldn't be that often.

> there is no walreceiver running (even if primary_slot_name is a valid one).

I feel that is indirectly relying on the fact that the primary won't
advance logical slots unless physical standby has consumed data. Now,
it is possible that slot-sync worker lags behind and still needs to
sync more data for slots in which it makes sense for slot-sync worker
to be alive. I think we can try to avoid checking walreceiver status
till we can get more data to avoid the problem I mentioned but it
doesn't sound like a clean way to achieve our purpose.

-- 
With Regards,
Amit Kapila.

---

From: shveta malik <[email-masked]>
Date: Wed, 6 Dec 2023 11:48:47 +0530

On Wed, Dec 6, 2023 at 10:56 AM Amit Kapila <[email-masked]> wrote:
> advance logical slots unless physical standby has consumed data.

Yes, that is the basis of this discussion. But now on rethinking, if
the user has not set 'standby_slot_names' on primary at first pace,
then even if walreceiver on standby is down, slots on primary will
keep on advancing and thus we need to sync. We have no check currently
that mandates users to set standby_slot_names.

> Now,

---

From: "Drouvot, Bertrand" <[email-masked]>
Date: Wed, 6 Dec 2023 10:30:08 +0100

Hi,

On 12/6/23 7:18 AM, shveta malik wrote:
> Yes, that is the basis of this discussion.

Yes.

> But now on rethinking, if
> keep on advancing

Oh right, good point.

> and thus we need to sync. 

Yes and I think our current check "XLogRecPtrIsInvalid(WalRcv->latestWalEnd)"
in synchronize_slots() prevents us to do so (as I think WalRcv->latestWalEnd
would be invalid for a non started walreceiver).

> We have no check currently

Yeah and OTOH unset standby_slot_names is currently the only
way for users to "force" advance failover slots if they want to (in case
say the standby is down for a long time and they don't want to block logical decoding
on the primary) as we don't provide a way to alter the failover property
(unless connecting with replication which sounds more like a hack).

>> to be alive.

Right.

Regards,

---

From: shveta malik <[email-masked]>
Date: Wed, 6 Dec 2023 16:28:51 +0530

> would be invalid for a non started walreceiver).

But I think we do not need to deal with the case that walreceiver is
not started at all on standby. It is always started. Walreceiver not
getting started or down for long is a rare scenario. We have other
checks too for 'latestWalEnd' in slotsync worker and I think we should
retain those as is.

> (unless connecting with replication which sounds more like a hack).

yes, right.

---

From: shveta malik <[email-masked]>
Date: Wed, 6 Dec 2023 16:53:24 +0530

> > >> sync more data for slots in which it makes sense for slot-sync worker

PFA v43, changes are:

v43-001:
1) Support of  'failover' dump in pg_dump. It was missing earlier.

v43-002:
1) Slot-sync worker now  checks validity of primary_slot_name by
connecting to primary, once during its start and later if
primary_slot_name GUC is changed.
2) Doc improvement (see logicaldecoding.sgml). More details on overall
slot-sync feature is added along with Nisha's comment of documenting
disabled-subscription behaviour wrt to synced slots.

thanks
Shveta

---

From: "Drouvot, Bertrand" <[email-masked]>
Date: Thu, 7 Dec 2023 08:49:42 +0100

Hi,

> checks too for 'latestWalEnd' in slotsync worker and I think we should

Agree to not deal with the walreceiver being down for now (we can
still improve that part later if we encounter the case in the real
world).

Might be worth to add comments in the code (around the WalRcv->latestWalEnd
checks) that no "lagging" sync are possible if the walreceiver is not started
though?

Regards,

---

From: "Drouvot, Bertrand" <[email-masked]>
Date: Thu, 7 Dec 2023 08:53:36 +0100

Hi,

Thanks!

> primary_slot_name GUC is changed.

I gave a second thought on it and yeah that sounds like the best option
(as compare to relying on the walreceiver being up or down).

Regards,

---

From: Peter Smith <[email-masked]>
Date: Thu, 7 Dec 2023 19:03:05 +1100

Hi.

Here are my review comments for patch v43-0002.

======
Commit message

1.
The nap time of worker is tuned according to the activity on the primary.
The worker starts with nap time of 10ms and if no activity is observed on
the primary for some time, then nap time is increased to 10sec. And if
activity is observed again, nap time is reduced back to 10ms.

~
/nap time of worker/nap time of the worker/
/And if/If/

~~~

2.
Slots synced on the standby can be identified using 'sync_state' column of
pg_replication_slots view. The values are:
'n': none for user slots,
'i': sync initiated for the slot but waiting for the remote slot on the
     primary server to catch up.
'r': ready for periodic syncs.

~

/identified using/identified using the/

The meaning of "identified by" is unclear to me. It also seems to
clash with later descriptions in system-views.sgml. Please see my
later review comment about it (in the sgml file)

======
doc/src/sgml/bgworker.sgml

3.
bgw_start_time is the server state during which postgres should start
the process; it can be one of BgWorkerStart_PostmasterStart (start as
soon as postgres itself has finished its own initialization; processes
requesting this are not eligible for database connections),
BgWorkerStart_ConsistentState (start as soon as a consistent state has
been reached in a hot standby, allowing processes to connect to
databases and run read-only queries), and
BgWorkerStart_RecoveryFinished (start as soon as the system has
entered normal read-write state. Note that the
BgWorkerStart_ConsistentState and BgWorkerStart_RecoveryFinished are
equivalent in a server that's not a hot standby), and
BgWorkerStart_ConsistentState_HotStandby (same meaning as
BgWorkerStart_ConsistentState but it is more strict in terms of the
server i.e. start the worker only if it is hot-standby; if it is
consistent state in non-standby, worker will not be started). Note
that this setting only indicates when the processes are to be started;
they do not stop when a different state is reached.

~

3a.
This seems to have grown to become just one enormous sentence that is
too hard to read. IMO this should be changed to be a <variablelist> of
possible values instead of a big slab of text. I suspect it could also
be simplified quite a lot -- something like below

SUGGESTION
bgw_start_time is the server state during which postgres should start
the process. Note that this setting only indicates when the processes
are to be started; they do not stop when a different state is reached.
Possible values are:

- BgWorkerStart_PostmasterStart (start as soon as postgres itself has
finished its own initialization; processes requesting this are not
eligible for database connections)

- BgWorkerStart_ConsistentState (start as soon as a consistent state
has been reached in a hot-standby, allowing processes to connect to
databases and run read-only queries)

- BgWorkerStart_RecoveryFinished (start as soon as the system has
entered normal read-write state. Note that the
BgWorkerStart_ConsistentState and BgWorkerStart_RecoveryFinished are
equivalent in a server that's not a hot standby)

- BgWorkerStart_ConsistentState_HotStandby (same meaning as
BgWorkerStart_ConsistentState but it is more strict in terms of the
server i.e. start the worker only if it is hot-standby; if it is a
consistent state in non-standby, the worker will not be started).

~~~

3b.
"i.e. start the worker only if it is hot-standby; if it is consistent
state in non-standby, worker will not be started"

~

Why is it even necessary to say the 2nd part "if it is consistent
state in non-standby, worker will not be started". It seems redundant
given 1st part says the same, right?

======
doc/src/sgml/config.sgml

4.
+       <para>

4a.
Somehow "must enable enable_syncslot" seemed strange. Maybe re-word like:

"must enable slot synchronization (see enable_syncslot)"

OR

"must configure enable_syncslot = true"

~~~

4b.
(seems like repetitive use of "the standbys")

/for the standbys to/to/

OR

/for the standbys to/so they can/

~~~

5.
           <varname>primary_conninfo</varname> string, or in a separate
-          <filename>~/.pgpass</filename> file on the standby server (use
+          <filename>~/.pgpass</filename> file on the standby server. (use

This rearranged period seems unrelated to the current patch. Maybe
don't touch this.

~~~

6.
+         <para>
+          Specify <literal>dbname</literal> in
+          <varname>primary_conninfo</varname> string to allow synchronization
+          of slots from the primary server to the standby server.
+          This will only be used for slot synchronization. It is ignored
+          for streaming.
          </para>

The wording "to allow synchronization of slots" seemed misleading to
me. Isn't that more the purpose of the 'enable_syncslot' GUC? I think
the intended wording is more like below:

SUGGESTION
If slot synchronization is enabled then it is also necessary to
specify <literal>dbname</literal> in the
<varname>primary_conninfo</varname> string. This will only be used for
slot synchronization. It is ignored for streaming.

======
doc/src/sgml/logicaldecoding.sgml

7.
+    <para>
+     A logical replication slot on the primary can be synchronized to the hot
+     standby by enabling the failover option during slot creation and set
+     <varname>enable_syncslot</varname> on the standby. For the synchronization
+     to work, it is mandatory to have physical replication slot between the
+     primary and the standby. This physical replication slot for the standby
+     should be listed in <varname>standby_slot_names</varname> on the primary
+     to prevent the subscriber from consuming changes faster than the hot
+     standby. Additionally, similar to creating a logical replication slot
+     on the hot standby, <varname>hot_standby_feedback</varname> should be
+     set on the standby and a physical slot between the primary and the standby
+     should be used.
+    </para>

7a.
/creation and set/creation and setting/
/to have physical replication/to have a physical replication/

~

7b.
It's unclear why this is saying "should be listed in
standby_slot_names" and "hot_standby_feedback should be set on the
standby". Why is it saying "should" instead of MUST -- are these
optional? I thought the GUC validation function mandates these (???).

~

7c.
Why does the paragraph say "and a physical slot between the primary
and the standby should be used.";  isn't that exactly what was already
written earlier ("For the synchronization to work, it is mandatory to
have physical replication slot between the primary and the standby"

~~~

8.
+    <para>
+     By enabling synchronization of slots, logical replication can be resumed
+     after failover depending upon the
+     <link linkend="view-pg-replication-slots">pg_replication_slots</link>.<structfield>sync_state</structfield>
+     for the synchronized slots on the standby at the time of failover.
+     The slots which were in ready sync_state ('r') on the standby before
+     failover can be used for logical replication after failover. However,
+     the slots which were in initiated sync_state ('i) and were not
+     sync-ready ('r') at the time of failover will be dropped and logical
+     replication for such slots can not be resumed after failover. This applies
+     to the case where a logical subscription is disabled before
failover and is
+     enabled after failover. If the synchronized slot due to disabled
+     subscription could not be made sync-ready ('r') on standby, then the
+     subscription can not be resumed after failover even when enabled.

8a.
This feels overcomplicated -- too much information?

SUGGESTION
depending upon the ... sync_state for the synchronized slots on the
standby at the time of failover. Only slots that were in ready
sync_state ('r') on the standby before failover can be used for
logical replication after failover

~~~

8b.
+     the slots which were in initiated sync_state ('i) and were not
+     sync-ready ('r') at the time of failover will be dropped and logical
+     replication for such slots can not be resumed after failover. This applies
+     to the case where a logical subscription is disabled before
failover and is
+     enabled after failover. If the synchronized slot due to disabled
+     subscription could not be made sync-ready ('r') on standby, then the
+     subscription can not be resumed after failover even when enabled.

But isn't ALL that part pretty much redundant information for the
user? I thought these are not ready state, so they are not usable...
End-Of-Story. Isn't everything else just more like implementation
details, which the user does not need to know about?

~~~

9.
+     If the primary is idle, making the synchronized slot on the standby
+     as sync-ready ('r') for enabled subscription may take noticeable time.
+     This can be sped up by calling the
+     <function>pg_log_standby_snapshot</function> function on the primary.
+    </para>

SUGGESTION
If the primary is idle, then the synchronized slots on the standby may
take a noticeable time to reach the ready ('r') sync_state. This can
be sped up by calling the
<function>pg_log_standby_snapshot</function> function on the primary.

======
doc/src/sgml/system-views.sgml

10.
+
+     <row>
+      <entry role="catalog_table_entry"><para role="column_definition">
+       <structfield>sync_state</structfield> <type>char</type>
+      </para>
+      <para>
+      Defines slot synchronization state. This is meaningful on the physical
+      standby which has enabled slots synchronization.
+      </para>

I felt that this part "which has enabled slots synchronization" should
cross-reference to the 'sync_enabled' GUC.

~~~

11.
+      <para>
+       State code:
+       <literal>n</literal> = none for user created slots,
+       <literal>i</literal> = sync initiated for the slot but slot is not ready
+        yet for periodic syncs,
+       <literal>r</literal> = ready for periodic syncs.
+      </para>

I'm wondering why don't we just reuse 'd' (disabled), 'p' (pending),
'e' (enabled) like the other tri-state attributes are using.

~~~

12.
+      <para>
+      The hot standby can have any of these sync_state for the slots but on a
+      hot standby, the slots with state 'r' and 'i' can neither be
used for logical
+      decoded nor dropped by the user. The primary server will have sync_state
+      as 'n' for all the slots. But if the standby is promoted to become the
+      new primary server, sync_state can be seen 'r' as well. On this new
+      primary server, slots with sync_state as 'r' and 'n' will
behave the same.
+      </para></entry>
+     </row>

12a.
/logical decoded/logical decoding/

~

12b.
"sync_state as 'r' and 'n' will behave the same" sounds kind of hacky.
Is there no alternative?

Anyway, IMO mentioning about primary server states seems overkill,
because you already said "This is meaningful on the physical standby"
which I took as implying that it is *not* meaningful from the POV of
the primary server.

In light of this, I'm wondering if a better name for this attribute
would be: 'standby_sync_state'

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

13.
+ /*
+ * Shutdown the slot sync workers to prevent potential conflicts between
+ * user processes and slotsync workers after a promotion. Additionally,
+ * drop any slots that have initiated but not yet completed the sync
+ * process.
+ */
+ ShutDownSlotSync();
+ slotsync_drop_initiated_slots();
+

Is this where maybe the 'sync_state' should also be updated for
everything so you are not left with confusion about different states
on a node that is no longer a standby node?

======
src/backend/postmaster/postmaster.c

14. PostmasterMain

  ApplyLauncherRegister();

+ SlotSyncWorkerRegister();
+

Every other function call here is heavily commented but there is a
conspicuous absence of a comment here.

~~~

15. bgworker_should_start_now

  if (start_time == BgWorkerStart_ConsistentState)
  return true;
+ else if (start_time == BgWorkerStart_ConsistentState_HotStandby &&
+ pmState != PM_RUN)
+ return true;
  /* fall through */
Change "else if" to "if" would be simpler.

======

16.
+ for (opt = opts; opt->keyword != NULL; ++opt)
+ {
+ /*
+ * If multiple dbnames are specified, then the last one will be
+ * returned
+ */
+ if (strcmp(opt->keyword, "dbname") == 0 && opt->val &&
+ opt->val[0] != '\0')
+ dbname = pstrdup(opt->val);
+ }

This can use a tidier C99 style to declare 'opt' as the loop variable.

~~~

17.
 static void
 libpqrcv_alter_slot(WalReceiverConn *conn, const char *slotname,
- bool failover)
+ bool failover)

What is this change for? Or, if something is wrong with the indent
then anyway it should be fixed in patch 0001.

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

18.

+ /*
+ * Slots in state SYNCSLOT_STATE_INITIATED should have been dropped on
+ * promotion.
+ */
+ if (!RecoveryInProgress() && slot->data.sync_state ==
SYNCSLOT_STATE_INITIATED)
+ elog(ERROR, "replication slot \"%s\" was not synced completely from
the primary server",
+ NameStr(slot->data.name));
+
+ /*
+ * Do not allow consumption of a "synchronized" slot until the standby
+ * gets promoted.
+ */
+ if (RecoveryInProgress() && slot->data.sync_state != SYNCSLOT_STATE_NONE)
+ ereport(ERROR,
+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("cannot use replication slot \"%s\" for logical decoding",
+ NameStr(slot->data.name)),
+ errdetail("This slot is being synced from the primary server."),
+ errhint("Specify another replication slot.")));
+

18a.

Instead of having !RecoveryInProgress() and RecoveryInProgress() in
separate conditions is the code simpler like:

SUGGESTION

if (RecoveryInProgress())
{
  /* Do not allow ... */
  if (slot->data.sync_state != SYNCSLOT_STATE_NONE) ...
}
else
{
  /* Slots in state... */
  if (slot->data.sync_state == SYNCSLOT_STATE_INITIATED) ...
}

~

18b.
Should the errdetail give the current state?

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

19.
+/*
+ * Number of attempts for wait_for_primary_slot_catchup() after
+ * which it aborts the wait and the slot sync worker then moves
+ * to the next slot creation/sync.
+ */
+#define WORKER_PRIMARY_CATCHUP_WAIT_ATTEMPTS 5

Given this is only used within one static function, I'm wondering if
it would be tidier to also move this macro to within that function.

~~~

20. wait_for_primary_slot_catchup

+/*
+ * Wait for remote slot to pass locally reserved position.
+ *
+ * Ping and wait for the primary server for
+ * WORKER_PRIMARY_CATCHUP_WAIT_ATTEMPTS during a slot creation, if it still
+ * does not catch up, abort the wait. The ones for which wait is aborted will
+ * attempt the wait and sync in the next sync-cycle.
+ *
+ * *persist will be set to false if the slot has disappeared or was invalidated
+ * on the primary; otherwise, it will be set to true.
+ */

20a.
The comment doesn't say the meaning of the boolean returned.

~

20b.
/*persist will be set/If passed, *persist will be set/

~~~

21.
+ appendStringInfo(&cmd,
+ "SELECT conflicting, restart_lsn, confirmed_flush_lsn,"
+ " catalog_xmin FROM pg_catalog.pg_replication_slots"
+ " WHERE slot_name = %s",
+ quote_literal_cstr(remote_slot->name));

Somehow, I felt it is more readable if the " FROM" starts on a new line.

e.g.
"SELECT conflicting, restart_lsn, confirmed_flush_lsn, catalog_xmin"
" FROM pg_catalog.pg_replication_slots"
" WHERE slot_name = %s"

~~~

22.
+ ereport(ERROR,
+ (errmsg("could not fetch slot info for slot \"%s\" from the"
+ " primary server: %s",
+ remote_slot->name, res->err)));

Perhaps the message can be shortened like:
"could not fetch slot \"%s\" info from the primary server: %s"

~~~

23.
+ ereport(WARNING,
+ (errmsg("slot \"%s\" disappeared from the primary server,"
+ " slot creation aborted", remote_slot->name)));

Would this be better split into parts?

SUGGESTION
errmsg "slot \"%s\" creation aborted"
errdetail "slot was not found on the primary server"

~~~

24.
+ ereport(WARNING,
+ (errmsg("slot \"%s\" invalidated on the primary server,"
+ " slot creation aborted", remote_slot->name)));

(similar to previous)

SUGGESTION
errmsg "slot \"%s\" creation aborted"
errdetail "slot was invalidated on the primary server"

~~~

25.
+ /*
+ * Once we got valid restart_lsn, then confirmed_lsn and catalog_xmin
+ * are expected to be valid/non-null.
+ */

SUGGESTION
Having got a valid restart_lsn, the confirmed_lsn and catalog_xmin are
expected to be valid/non-null.

~~~

26. slotsync_drop_initiated_slots

+/*
+ * Drop the slots for which sync is initiated but not yet completed
+ * i.e. they are still waiting for the primary server to catch up.
+ */

I found "waiting for the primary server to catch up" to be difficult
to understand without knowing the full details, but it is not really
described properly until a much larger comment that is buried in the
synchronize_one_slot(). So I think all this needs explanation up-front
in the file, which you can refer to. I have repeated this same review
comment in a couple of places.

~~~

27. get_local_synced_slot_names

+static List *
+get_local_synced_slot_names(void)
+{
+ List    *localSyncedSlots = NIL;

27a.
It's not returning a list of "names" though, so is this an appropriate
function name?

~~~

27b.
Suggest just call that ('localSyncedSlots') differently.
- In slotsync_drop_initiated_slots() function they are just called 'slots'
- In drop_obsolete_slots() function it is called 'local_slot_list'

IMO it is better if all these are consistently named -- just all lists
'slots' or all 'local_slots' or whatever.

~~~

28. check_sync_slot_validity

+static bool
+check_sync_slot_validity(ReplicationSlot *local_slot, List *remote_slots,
+ bool *locally_invalidated)

Somehow this wording "validity" seems like a misleading function name,
because the return value has nothing to do with the slot field
invalidated.

The validity/locally_invalidated stuff is a secondary return as a side
effect for the "true" case.

A more accurate function name would be more like check_sync_slot_on_remote().

~~~

29. check_sync_slot_validity

+static bool
+check_sync_slot_validity(ReplicationSlot *local_slot, List *remote_slots,
+ bool *locally_invalidated)
+{
+ ListCell   *cell;

There is inconsistent naming --

ListCell lc; ListCell cell; ListCell lc_slot; etc..

IMO the more complicated names aren't of much value -- probably
everything can be changed to 'lc' for consistency.

~~~

30. drop_obsolete_slots

+ /*
+ * Get the list of local 'synced' slot so that those not on remote could
+ * be dropped.
+ */

/slot/slots/

Also, I don't think it is necessary to say "so that those not on
remote could be dropped." -- That is already described in the function
comment and again in a comment later in the loop. That seems enough.
If the function name get_local_synced_slot_names() is improved a bit
the comment seems redundant because it is obvious from the function
name.

~~~

31.
+ foreach(lc_slot, local_slot_list)
+ {
+ ReplicationSlot *local_slot = (ReplicationSlot *) lfirst(lc_slot);
+ bool local_exists = false;
+ bool locally_invalidated = false;
+
+ local_exists = check_sync_slot_validity(local_slot, remote_slot_list,
+ &locally_invalidated);

Shouldn't that 'local_exists' variable be called 'remote_exists'?
That's what the other comments seem to be saying.

~~~

32. construct_slot_query

+ appendStringInfo(s,
+ "SELECT slot_name, plugin, confirmed_flush_lsn,"
+ " restart_lsn, catalog_xmin, two_phase, failover,"
+ " database, pg_get_slot_invalidation_cause(slot_name)"
+ " FROM pg_catalog.pg_replication_slots"
+ " WHERE failover and sync_state != 'i'");

Just wondering if substituting the SYNCSLOT_STATE_INITIATED constant
here might be more appropriate than hardwiring 'i'. Why have a
constant but not use it?

~~~

33. synchronize_one_slot

+static void
+synchronize_one_slot(WalReceiverConn *wrconn, RemoteSlot *remote_slot,
+ bool *slot_updated)
+{
+ ReplicationSlot *s;
+ char sync_state = 0;

33a.
It seems strange that the sync_state is initially assigned something
other than the 3 legal values. Should this be defaulting to
SYNCSLOT_STATE_NONE instead?

~

33b.
I think it is safer to default the *slot_updated = false; because the
code appears to assume it was false already which may or may not be
true.

~~~

34.
+ /*
+ * Make sure that concerned WAL is received before syncing slot to target
+ * lsn received from the primary server.
+ *
+ * This check should never pass as on the primary server, we have waited
+ * for the standby's confirmation before updating the logical slot.
+ */

Maybe this comment should mention up-front that it is just a "Sanity check:"

~~~

35.
+ /*
+ * With hot_standby_feedback enabled and invalidations handled
+ * apropriately as above, this should never happen.
+ */
+ if (remote_slot->restart_lsn < MyReplicationSlot->data.restart_lsn)
+ {
+ ereport(ERROR,
+ errmsg("not synchronizing local slot \"%s\" LSN(%X/%X)"
+    " to remote slot's LSN(%X/%X) as synchronization "
+    " would move it backwards", remote_slot->name,
+    LSN_FORMAT_ARGS(MyReplicationSlot->data.restart_lsn),
+    LSN_FORMAT_ARGS(remote_slot->restart_lsn)));
+
+ goto cleanup;
+ }

35a.
IIUC then this another comment that should say it is just a "Sanity-check:".

~

35b.
I was wondering if there should be Assert(hot_standby_feedback) here
also. The comment "With hot_standby_feedback enabled" is a bit vague
whereas including an Assert will clarify that it must be set.

~

35c.
Since it says "this should never happen" then it appears elog is more
appropriate than ereport because translations are not needed, right?

~

35d.
The ERROR will make that goto cleanup unreachable, won't it?

~~~

36.
+ /*
+ * Already existing slot but not ready (i.e. waiting for the primary
+ * server to catch-up), lets attempt to make it sync-ready now.
+ */

/lets/let's/

~~~

37.
+ /*
+ * Refer the slot creation part (last 'else' block) for more details
+ * on this wait.
+ */
+ if (remote_slot->restart_lsn < MyReplicationSlot->data.restart_lsn ||
+ TransactionIdPrecedes(remote_slot->catalog_xmin,
+   MyReplicationSlot->data.catalog_xmin))
+ {
+ if (!wait_for_primary_slot_catchup(wrconn, remote_slot, NULL))
+ {
+ goto cleanup;
+ }
+ }

37a.
Having to jump forward to understand earlier code seems backward. IMO
there should be a big comment atop this module about this subject
which the comment here can just refer to. I will write more about this
topic later (below).

~

37b.
The extra code curly braces are not needed.

~~~

38.
+ ereport(LOG, errmsg("newly locally created slot \"%s\" is sync-ready "
+ "now", remote_slot->name));

Better to put the whole errmsg() on a newline instead of splitting the
string like that.

~~~

39.
+ /* User created slot with the same name exists, raise ERROR. */
+ else if (sync_state == SYNCSLOT_STATE_NONE)
+ {
+ ereport(ERROR,
+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("skipping sync of slot \"%s\" as it is a user created"
+ " slot", remote_slot->name),
+ errdetail("This slot has failover enabled on the primary and"
+    " thus is sync candidate but user created slot with"
+    " the same name already exists on the standby")));
+ }

I felt it would be better to eliminate this case immediately up-front
when you first searched for the slot names. e.g. code like below. IIUC
this refactor also means the default sync_state can be assigned a
normal value (as I suggested above) instead of the strange assignment
to 0.

+ /* Search for the named slot */
+ if ((s = SearchNamedReplicationSlot(remote_slot->name, true)))
+ {
+ SpinLockAcquire(&s->mutex);
+ sync_state = s->data.sync_state;
+ SpinLockRelease(&s->mutex);

INSERT HERE
+     /* User-created slot with the same name exists, raise ERROR. */
+     if (sync_state == SYNCSLOT_STATE_NONE)
+     ereport(ERROR, ...
+ }

~~~

40.
+ /* Otherwise create the slot first. */
+ else
+ {

Insert a blank line above that comment for better readability (same as
done for earlier 'else' in this same function)

~~~

41.
+ ReplicationSlotCreate(remote_slot->name, true, RS_EPHEMERAL,
+   remote_slot->two_phase,
+   remote_slot->failover,
+   SYNCSLOT_STATE_INITIATED);
+
+ slot = MyReplicationSlot;

In hindsight, the prior if/else code blocks in this function also
could have done "slot = MyReplicationSlot;" same as this -- then the
code would be much less verbose.

~~~

42.
+ SpinLockAcquire(&slot->mutex);
+ slot->data.database = get_database_oid(remote_slot->database, false);
+
+ namestrcpy(&slot->data.plugin, remote_slot->plugin);
+ SpinLockRelease(&slot->mutex);

IMO the code would be more readable *without* a blank line here
because the mutexed block is more obvious.

~~~

43.
+ /*
+ * If the local restart_lsn and/or local catalog_xmin is ahead of
+ * those on the remote then we cannot create the local slot in sync
+ * with the primary server because that would mean moving the local
+ * slot backwards and we might not have WALs retained for old LSN. In
+ * this case we will wait for the primary server's restart_lsn and
+ * catalog_xmin to catch up with the local one before attempting the
+ * sync.
+ */

43a.
This comment describes some fundamental concepts about how this logic
works. I felt this and other comments like this should be at the top
of this slotsync.c file. Then anything that needs to mention about it
can refer to the top comment. For example, I also found other comments
like "... they are still waiting for the primary server to catch up."
to be difficult to understand without knowing these details, but I
think describing core design stuff up-front and saying "refer to the
comment atop the fil" probably would help a lot.

~

43b.
Should "wait for the primary server's restart_lsn and..." be "wait for
the primary server slot's restart_lsn and..." ?

~~~

44.
+ {
+ bool persist;
+
+ if (!wait_for_primary_slot_catchup(wrconn, remote_slot, &persist))
+ {
+ /*
+ * The remote slot didn't catch up to locally reserved
+ * position.
+ *
+ * We do not drop the slot because the restart_lsn can be
+ * ahead of the current location when recreating the slot in
+ * the next cycle. It may take more time to create such a
+ * slot. Therefore, we persist it (provided remote-slot is
+ * still valid) and attempt the wait and synchronization in
+ * the next cycle.
+ */
+ if (persist)
+ {
+ ReplicationSlotPersist();
+ *slot_updated = true;
+ }
+
+ goto cleanup;
+ }
+ }

Looking at the way this 'persist' parameter is used I felt is it too
complicated. IIUC the wait_for_primary_slot_catchup can only return
*persist = true (for a false return) when it has reached/exceeded the
number of retries and still not yet caught up. Why should
wait_for_primary_slot_catchup() pretend to know about persistence?

In other words, I thought a more meaningful parameter/variable name
(instead of 'persist') is something like 'wait_attempts_exceeded'. IMO
that will make wait_for_primary_slot_catchup() code easier, and here
you can just say like below, where the code matches the comment
better. Thoughts?

+ if (wait_attempts_exceeded)
+ {
+ ReplicationSlotPersist();
+ *slot_updated = true;
+ }

~~~

45.
+
+
+ /*
+ * Wait for primary is either not needed or is over. Update the lsns
+ * and mark the slot as READY for further syncs.
+ */

Double blank lines?

~~~

46.
+ ereport(LOG, errmsg("newly locally created slot \"%s\" is sync-ready "
+ "now", remote_slot->name));
+ }
+
+cleanup:

Better to put the whole errmsg() on a newline instead of splitting the
string like that.

~~~

47. synchronize_slots

+/*
+ * Synchronize slots.
+ *
+ * Gets the failover logical slots info from the primary server and update
+ * the slots locally. Creates the slots if not present on the standby.
+ *
+ * Returns nap time for the next sync-cycle.
+ */
+static long
+synchronize_slots(WalReceiverConn *wrconn)

/update/updates/

~~~

48.
+ /* The primary_slot_name is not set yet or WALs not received yet */
+ SpinLockAcquire(&WalRcv->mutex);
+ if (!WalRcv ||
+ (WalRcv->slotname[0] == '\0') ||
+ XLogRecPtrIsInvalid(WalRcv->latestWalEnd))
+ {
+ SpinLockRelease(&WalRcv->mutex);
+ return naptime;
+ }
+ SpinLockRelease(&WalRcv->mutex);

Just wondering if the scenario of "WALS not received" is a bit more
like "no activity" so perhaps the naptime returned should be
WORKER_INACTIVITY_NAPTIME_MS here?

~~~

49.
+ /* Construct query to get slots info from the primary server */
+ initStringInfo(&s);
+ construct_slot_query(&s);

I did not like the construct_slot_query() to be separated from this
function because it makes it too difficult to see if the slot_attr
numbers and column types in this function are correct w.r.t. that
query. IMO better when everything is in the same place where you can
see it all together. e.g. Less risk of breaking something if changes
are made.

~~~

50.
+ /* Construct the remote_slot tuple and synchronize each slot locally */
+ slot = MakeSingleTupleTableSlot(res->tupledesc, &TTSOpsMinimalTuple);

Normally in all the other functions the variable 'slot' was the local
ReplicationSlot but IIUC here represents a remote tuple. Making a
different name would be better like 'remote_slottup' or something
else.

~~~

51.
+ /*
+ * If any of the slots get updated in this sync-cycle, retain default
+ * naptime and update 'last_update_time' in slot sync worker. But if no
+ * activity is observed in this sync-cycle, then increase naptime provided
+ * inactivity time reaches threshold.
+ */

I think "retain" is a slightly wrong word here because it might have
been WORKER_INACTIVITY_NAPTIME_MS in the previous cycle.

Maybe just /retain/use/

~~~

52.
+/*
+ * Connects primary to validate the slot specified in primary_slot_name.
+ *
+ * Exits the worker if physical slot with the specified name does not exist.
+ */
+static void
+validate_primary_slot(WalReceiverConn *wrconn)

There is already a connection, so not sure if this connect should be
saying "connects to"; Maybe is should be saying more like below:

SUGGESTION
Using the specified primary server connection, validate if the
physical slot identified by GUC primary_slot_name exists.

Exit the worker if the slot is not found.

~~~

53.
+ initStringInfo(&cmd);
+ appendStringInfo(&cmd,
+ "select count(*) = 1 from pg_replication_slots where "
+ "slot_type='physical' and slot_name=%s",
+ quote_literal_cstr(PrimarySlotName));

Write the SQL keywords in uppercase.

~~~

54.
+ if (res->status != WALRCV_OK_TUPLES)
+ ereport(ERROR,
+ (errmsg("could not fetch primary_slot_name info from the "
+ "primary: %s", res->err)));

Shouldn't the name of the unfound slot be shown in the ereport, or
will that already appear in the res->err?

~~~

55.
+ ereport(ERROR,
+ errmsg("exiting slots synchronization as slot specified in "
+    "primary_slot_name is not valid"));
+

IMO the format should be the same as I suggested (later) for all the
validate_slotsync_parameters() errors.

Also, I think the name of the unfound slot needs to be in this message.

So maybe result is like this:

SUGGESTION

ereport(ERROR,
  errmsg("exiting from slot synchronization due to bad configuration")
  /* translator: second %s is a GUC variable name */
  errhint("The primary slot \"%s\" specified by %s is not valid.",
slot_name, "primary_slot_name")
);

~~~

56.
+/*
+ * Checks if GUCs are set appropriately before starting slot sync worker
+ */
+static void
+validate_slotsync_parameters(char **dbname)
+{
+ /*
+ * Since 'enable_syncslot' is ON, check that other GUC settings
+ * (primary_slot_name, hot_standby_feedback, wal_level, primary_conninfo)
+ * are compatible with slot synchronization. If not, raise ERROR.
+ */
+

56a.
I thought that 2nd comment sort of belonged in the function comment.

~

56b.
It says "Since 'enable_syncslot' is ON", but I IIUC that is wrong
because the other function slotsync_reread_config() might detect a
change in this GUC and cause this validate_slotsync_parameters() to be
called when enable_syncslot was changed to false.

In other words, I think you also need to check 'enable_syncslot' and
exit with appropriate ERROR same as all the other config problems.

OTOH if this is not possible, then the slotsync_reread_config() might
need fixing instead.

~~~

57.
+ /*
+ * A physical replication slot(primary_slot_name) is required on the
+ * primary to ensure that the rows needed by the standby are not removed
+ * after restarting, so that the synchronized slot on the standby will not
+ * be invalidated.
+ */
+ if (PrimarySlotName == NULL || strcmp(PrimarySlotName, "") == 0)
+ ereport(ERROR,
+ errmsg("exiting slots synchronization as primary_slot_name is "
+    "not set"));
+
+ /*
+ * Hot_standby_feedback must be enabled to cooperate with the physical
+ * replication slot, which allows informing the primary about the xmin and
+ * catalog_xmin values on the standby.
+ */
+ if (!hot_standby_feedback)
+ ereport(ERROR,
+ errmsg("exiting slots synchronization as hot_standby_feedback "
+    "is off"));
+
+ /*
+ * Logical decoding requires wal_level >= logical and we currently only
+ * synchronize logical slots.
+ */
+ if (wal_level < WAL_LEVEL_LOGICAL)
+ ereport(ERROR,
+ errmsg("exiting slots synchronisation as it requires "
+    "wal_level >= logical"));
+
+ /*
+ * The primary_conninfo is required to make connection to primary for
+ * getting slots information.
+ */
+ if (PrimaryConnInfo == NULL || strcmp(PrimaryConnInfo, "") == 0)
+ ereport(ERROR,
+ errmsg("exiting slots synchronization as primary_conninfo "
+    "is not set"));
+
+ /*
+ * The slot sync worker needs a database connection for walrcv_exec to
+ * work.
+ */
+ *dbname = walrcv_get_dbname_from_conninfo(PrimaryConnInfo);
+ if (*dbname == NULL)
+ ereport(ERROR,
+ errmsg("exiting slots synchronization as dbname is not "
+    "specified in primary_conninfo"));
+
+}

IMO all these errors can be improved by:
- using a common format
- including errhint for the reason
- using the same tone for instructions on what to do (e.g saying must
be set, rather than what was not set)

SUGGESTION (something like this)

ereport(ERROR,
  errmsg("exiting from slot synchronization due to bad configuration")
  /* translator: %s is a GUC variable name */
  errhint("%s must be defined.", "primary_slot_name")
);

ereport(ERROR,
  errmsg("exiting from slot synchronization due to bad configuration")
  /* translator: %s is a GUC variable name */
  errhint("%s must be enabled.", "hot_standby_feedback")
);

ereport(ERROR,
  errmsg("exiting from slot synchronization due to bad configuration")
  /* translator: wal_level is a GUC variable name, 'logical' is a value */
  errhint("wal_level must be >= logical.")
);

ereport(ERROR,
  errmsg("exiting from slot synchronization due to bad configuration")
  /* translator: %s is a GUC variable name */
  errhint("%s must be defined.", "primary_conninfo")
);

ereport(ERROR,
  errmsg("exiting from slot synchronization due to bad configuration")
  /* translator: 'dbname' is a specific option; %s is a GUC variable name */
  errhint("'dbname' must be specified in %s.", "primary_conninfo")
);

~~~

58.
+ *dbname = walrcv_get_dbname_from_conninfo(PrimaryConnInfo);
+ if (*dbname == NULL)
+ ereport(ERROR,
+ errmsg("exiting slots synchronization as dbname is not specified in
primary_conninfo"));
+
+}

Unnecessary blank line at the end of the function

~~~

59.
+/*
+ * Re-read the config file.
+ *
+ * If any of the slot sync GUCs changed, validate the values again
+ * through validate_slotsync_parameters() which will exit the worker
+ * if validaity fails.
+ */

SUGGESTION
If any of the slot sync GUCs have changed, re-validate them. The
worker will exit if the check fails.

~~~

60.
+ char    *conninfo = pstrdup(PrimaryConnInfo);
+ char    *slotname = pstrdup(PrimarySlotName);
+ bool syncslot = enable_syncslot;
+ bool standbyfeedback = hot_standby_feedback;

For clarity, I would have used var names to match the old GUCs.

e.g.
/conninfo/old_primary_conninfo/
/slotname/old_primary_slot_name/
/syncslot/old_enable_syncslot/
/standbyfeedback/old_hot_standby_feedback/

~~~

61.
+ dbname = walrcv_get_dbname_from_conninfo(PrimaryConnInfo);
+ Assert(dbname);

This code seems premature. IIUC this is only needed to detect that the
dbname was changed. But I think the prerequisite is first that the
conninfoChanged is true. So really this code should be guarded by if
(conninfoChanged) so it can be done later in the function.

~~~

62.
+ if (conninfoChanged || slotnameChanged ||
+ (syncslot != enable_syncslot) ||
+ (standbyfeedback != hot_standby_feedback))
+ {
+ revalidate = true;
+ }

SUGGESTION

revalidate = conninfoChanged || slotnameChanged ||
 (syncslot != enable_syncslot) ||
 (standbyfeedback != hot_standby_feedback);

~~~

63.
+ /*
+ * Since we have initialized this worker with old dbname, thus exit if
+ * dbname changed. Let it get restarted and connect to new dbname
+ * specified.
+ */
+ if (conninfoChanged && strcmp(dbname, new_dbname) != 0)
+ {
+ ereport(ERROR,
+ errmsg("exiting slot sync woker as dbname in "
+    "primary_conninfo changed"));
+ }

63a.
/old dbname/the old dbname/
/new dbname/the new dbname/
/woker/worker/

~

63b.
This code feels awkward. Can't this dbname check and accompanying
ERROR message be moved down into validate_slotsync_parameters(), so it
lives along with all the other GUC validation logic? Maybe you'll need
to change the validate_slotsync_parameters() parameters slightly but I
think it is much better to keep all the validation together.

~~~

64.
+
+
+/*
+ * Interrupt handler for main loop of slot sync worker.
+ */
+static void
+ProcessSlotSyncInterrupts(WalReceiverConn **wrconn)

Double blank lines.

~~~

65.
+
+
+ if (ConfigReloadPending)
+ slotsync_reread_config();
+}

Double blank lines

~~~

66. slotsync_worker_onexit

+static void
+slotsync_worker_onexit(int code, Datum arg)
+{
+ SpinLockAcquire(&SlotSyncWorker->mutex);
+ SlotSyncWorker->pid = 0;
+ SpinLockRelease(&SlotSyncWorker->mutex);
+}

Should assignment use InvalidPid (-1) instead of 0?

~~~

67. ReplSlotSyncWorkerMain

+ SpinLockAcquire(&SlotSyncWorker->mutex);
+
+ Assert(SlotSyncWorker->pid == 0);
+
+ /* Advertise our PID so that the startup process can kill us on promotion */
+ SlotSyncWorker->pid = MyProcPid;
+
+ SpinLockRelease(&SlotSyncWorker->mutex);

Shouldn't pid start as InvalidPid (-1) instead of Assert 0?

~~~

68.
+ /* Connect to the primary server */
+ wrconn = remote_connect();
+
+ /*
+ * Connect to primary and validate the slot specified in
+ * primary_slot_name.
+ */
+ validate_primary_slot(wrconn);

Maybe needs some slight rewording in the 2nd comment. "Connect to
primary server" is already said and done in the 1st part.

~~~

69. IsSlotSyncWorker

+/*
+ * Is current process the slot sync worker?
+ */
+bool
+IsSlotSyncWorker(void)
+{
+ return SlotSyncWorker->pid == MyProcPid;
+}

69a.
For consistency with others like it, I thought this be called
IsLogicalSlotSyncWorker().

~

69b.
For consistency with the others like this, I think the extern should
be declared in logicalworker.h

~~~

70. ShutDownSlotSync

+ SpinLockAcquire(&SlotSyncWorker->mutex);
+ if (!SlotSyncWorker->pid)
+ {
+ SpinLockRelease(&SlotSyncWorker->mutex);
+ return;
+ }

IMO should be comparing with InvalidPid (-1) here; not 0.

~~~

71.
+ SpinLockAcquire(&SlotSyncWorker->mutex);
+
+ /* Is it gone? */
+ if (!SlotSyncWorker->pid)
+ break;
+
+ SpinLockRelease(&SlotSyncWorker->mutex);

Ditto. bad pids should be InvalidPid (-1), not 0.

~~~

72. SlotSyncWorkerShmemInit

+ if (!found)
+ {
+ memset(SlotSyncWorker, 0, size);
+ SpinLockInit(&SlotSyncWorker->mutex);
+ }

Probably here the unassigned pid should be set to InvalidPid (-1), not 0.

~~~

73. SlotSyncWorkerRegister

+ if (!enable_syncslot)
+ {
+ ereport(LOG,
+ errmsg("skipping slots synchronization as enable_syncslot is "
+    "disabled."));
+ return;
+ }

/as/because/

======

74.
 #include "commands/copy.h"
+#include "commands/subscriptioncmds.h"
 #include "miscadmin.h"

There were only #include changes but no code changes. Is the #include needed?

======

75. ReplicationSlotCreate

 void
 ReplicationSlotCreate(const char *name, bool db_specific,
    ReplicationSlotPersistency persistency,
-   bool two_phase, bool failover)
+   bool two_phase, bool failover, char sync_state)

The function comment goes to trouble to describe all the parameters
except for 'failover' and 'sync_slate'. I think a failover comment
should be added in patch 0001 and then the sync_state comment should
be added in patch 0002.

~~~

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

Should the errdetail give the current state?

======
src/backend/tcop/postgres.c

77.
+ else if (IsSlotSyncWorker())
+ {
+ ereport(DEBUG1,
+ (errmsg_internal("replication slot sync worker is shutting down due
to administrator command")));
+
+ /*
+ * Slot sync worker can be stopped at any time.
+ * Use exit status 1 so the background worker is restarted.
+ */
+ proc_exit(1);
+ }

Explicitly saying "ereport(DEBUG1, errmsg_internal(..." is a bit
overkill; it is simpler to write this as "elog(DEBUG1, ....);

======

78.
+/* The possible values for 'sync_state' in ReplicationSlotPersistentData */
+#define SYNCSLOT_STATE_NONE          'n' /* None for user created slots */
+#define SYNCSLOT_STATE_INITIATED     'i' /* Sync initiated for the slot but
+ * not completed yet, waiting for
+ * the primary server to catch-up */
+#define SYNCSLOT_STATE_READY         'r' /* Initialization complete, ready
+ * to be synced further */

Already questioned the same elsewhere. IIUC the same tri-state values
of other attributes might be used here too without needing to
introduce 3 new values.

e.g.

#define SYNCSLOT_STATE_DISABLED 'd' /* No syncing for this slot */
#define SYNCSLOT_STATE_PENDING  'p' /* Sync is enabled but we must
wait for the primary server to catch up */
#define SYNCSLOT_STATE_ENABLED  'e' /* Sync is enabled and the slot is
ready to be synced */

~~~

79.
+ /*
+ * Is this a slot created by a sync-slot worker?
+ *
+ * Relevant for logical slots on the physical standby.
+ */
+ char sync_state;
+

I assumed that "Relevant for" means "Only relevant for". It should say that.

If correct, IMO a better field name might be 'standby_sync_state'

======
src/test/recovery/t/050_verify_slot_order.pl

80.
+$backup_name = 'backup2';
+$primary->backup($backup_name);
+
+# Create standby3
+my $standby3 = PostgreSQL::Test::Cluster->new('standby3');
+$standby3->init_from_backup(
+ $primary, $backup_name,
+ has_streaming => 1,
+ has_restoring => 1);

The mixture of 'backup2' for 'standby3' seems confusing. Is there a
reason to call it backup2?

~~~

81.
+# Verify slot properties on the standby
+is( $standby3->safe_psql('postgres',
+ q{SELECT failover, sync_state FROM pg_replication_slots WHERE
slot_name = 'lsub1_slot';}
+  ),
+  "t|r",
+  'logical slot has sync_state as ready and failover as true on standby');

It might be better if the message has the same order as the SQL. Eg.
"failover as true and sync_state as ready".

~~~

82.
+# Verify slot properties on the primary
+is( $primary->safe_psql('postgres',
+    q{SELECT failover, sync_state FROM pg_replication_slots WHERE
slot_name = 'lsub1_slot';}
+  ),
+  "t|n",
+  'logical slot has sync_state as none and failover as true on primary');
+

It might be better if the message has the same order as the SQL. Eg.
"failover as true and sync_state as none".

~~~

83.
+# Test to confirm that restart_lsn of the logical slot on the primary
is synced to the standby

IMO the major test parts (like this one) may need more highlighting "#
---------------------" so those comments don't get lost among all the
other comments.

~~~

84.
+# let the slots get synced on the standby
+sleep 2;

Won't this make the test prone to failure on slow machines? Is there
not a more deterministic way to wait for the sync?

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

---

From: shveta malik <[email-masked]>
Date: Thu, 7 Dec 2023 14:37:26 +0530

> still improve that part later if we encounter the case in the real

yes, agreed.

> checks) that no "lagging" sync are possible if the walreceiver is not started

I am a bit confused. Do you mean as a TODO item? Otherwise the comment
will be opposite of the code we are writing.

---

From: "Drouvot, Bertrand" <[email-masked]>
Date: Thu, 7 Dec 2023 10:27:18 +0100

Hi,

> will be opposite of the code we are writing.

Sorry for the confusion: what I meant to say is that
synchronization (should it be lagging) is not possible if the walreceiver is not started
(as XLogRecPtrIsInvalid(WalRcv->latestWalEnd) would be true).

More precisely here (in synchronize_slots()):

     /* The primary_slot_name is not set yet or WALs not received yet */
     SpinLockAcquire(&WalRcv->mutex);
     if (!WalRcv ||
         (WalRcv->slotname[0] == '\0') ||
         XLogRecPtrIsInvalid(WalRcv->latestWalEnd))
     {
         SpinLockRelease(&WalRcv->mutex);
         return naptime;
     }

Regards,

---

From: Amit Kapila <[email-masked]>
Date: Thu, 7 Dec 2023 17:06:50 +0530

> 1) Support of  'failover' dump in pg_dump. It was missing earlier.

Review v43-0001
================
1.
+ * However, we do not enable failover for slots created by the table sync
+ * worker. This is because the table sync slot might not be fully synced on the
+ * standby.

The reason for not enabling failover for table sync slots is not
clearly mentioned.

2.
During syncing, the local restart_lsn and/or local catalog_xmin of
+ * the newly created slot on the standby are typically ahead of those on the
+ * primary. Therefore, the standby needs to wait for the primary server's
+ * restart_lsn and catalog_xmin to catch up, which takes time.

I think this part of the comment should be moved to 0002 patch. We can
probably describe a bit more about why slot on standby will be ahead
and about waiting time.

3.
validate_standby_slots()
{
...
+ slot = SearchNamedReplicationSlot(name, true);
+
+ if (!slot)
+ goto ret_standby_slot_names_ng;
+
+ if (!SlotIsPhysical(slot))
+ {
+ GUC_check_errdetail("\"%s\" is not a physical replication slot",
+ name);
+ goto ret_standby_slot_names_ng;
+ }

Why the first check (slot not found) doesn't have errdetail? The
goto's in this function look a bit odd, can we try to avoid those?

4.
+ /* Verify syntax and parse string into list of identifiers */
+ if (!SplitIdentifierString(rawname, ',', &elemlist))
+ {
+ /* syntax error in name list */
+ GUC_check_errdetail("List syntax is invalid.");
...
...
+ if (!SplitIdentifierString(standby_slot_names_cpy, ',', &standby_slots))
+ {
+ /* This should not happen if GUC checked check_standby_slot_names. */
+ elog(ERROR, "invalid list syntax");

Both are checking the same string but giving different error messages.
I think the error message should be the same in both cases. The first
one seems better.

5. In WalSndFilterStandbySlots(), the comments around else if checks
should move inside the checks. It is hard to read the code in the
current format. I have tried to change the same in the attached.

Apart from the above, I have changed the comments and made some minor
cosmetic changes in the attached. Kindly include in next version if
you are fine with it.

-- 
With Regards,
Amit Kapila.
diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c
index 58e298af89..ef486ca92c 100644
--- a/src/backend/replication/walsender.c
+++ b/src/backend/replication/walsender.c
@@ -1674,33 +1674,38 @@ WalSndFilterStandbySlots(XLogRecPtr wait_for_lsn, List **standby_slots)
 
 			continue;
 		}
-
-		/*
-		 * It may happen that the slot specified in standby_slot_names GUC
-		 * value is dropped, so let's skip over it.
-		 */
 		else if (!slot)
+		{
+			/*
+			 * It may happen that the slot specified in standby_slot_names GUC
+			 * value is dropped, so let's skip over it.
+			 */
 			warningfmt = _("replication slot \"%s\" specified in parameter \"%s\" does not exist, ignoring");
-
-		/*
-		 * If a logical slot name is provided in standby_slot_names, issue a
-		 * WARNING and skip it. Although logical slots are disallowed in the
-		 * GUC check_hook(validate_standby_slots), it is still possible for a
-		 * user to drop an existing physical slot and recreate a logical slot
-		 * with the same name. Since it is harmless, a WARNING should be
-		 * enough, no need to error-out.
-		 */
+		}
 		else if (SlotIsLogical(slot))
+		{
+			/*
+			 * If a logical slot name is provided in standby_slot_names, issue
+			 * a WARNING and skip it. Although logical slots are disallowed in
+			 * the GUC check_hook(validate_standby_slots), it is still possible
+			 * for a user to drop an existing physical slot and recreate a
+			 * logical slot with the same name. Since it is harmless, a WARNING
+			 * should be enough, no need to error-out.
+			 */
 			warningfmt = _("cannot have logical replication slot \"%s\" in parameter \"%s\", ignoring");
-
-		/*
-		 * Specified physical slot may have been invalidated, so no point in
-		 * waiting for it.
-		 */
+		}
 		else if (XLogRecPtrIsInvalid(restart_lsn) || invalidated)
+		{
+			/*
+			 * Specified physical slot may have been invalidated, so no point in
+			 * waiting for it.
+			 */
 			warningfmt = _("physical slot \"%s\" specified in parameter \"%s\" has been invalidated, ignoring");
+		}
 		else
+		{
 			Assert(restart_lsn >= wait_for_lsn);
+		}
 
 		/*
 		 * Reaching here indicates that either the slot has passed the
@@ -1768,9 +1773,9 @@ WalSndWaitForStandbyConfirmation(XLogRecPtr wait_for_lsn)
 /*
  * Wait till WAL < loc is flushed to disk so it can be safely sent to client.
  *
- * If the walsender holds a logical slot that has enabled failover, the
- * function also waits for all the specified streaming replication standby
- * servers to confirm receipt of WAL up to RecentFlushPtr.
+ * If the walsender holds a logical slot that has enabled failover, we also
+ * wait for all the specified streaming replication standby servers to
+ * confirm receipt of WAL up to RecentFlushPtr.
  *
  * Returns end LSN of flushed WAL.  Normally this will be >= loc, but if we
  * detect a shutdown request (either from postmaster or client) we will return
@@ -1790,7 +1795,7 @@ WalSndWaitForWal(XLogRecPtr loc)
 
 	/*
 	 * Check if all the standby servers have confirmed receipt of WAL up to
-	 * RecentFlushPtr if we already know we have enough WAL available.
+	 * RecentFlushPtr even when we already know we have enough WAL available.
 	 *
 	 * Note that we cannot directly return without checking the status of
 	 * standby servers because the standby_slot_names may have changed, which
@@ -1892,7 +1897,10 @@ WalSndWaitForWal(XLogRecPtr loc)
 			wait_for_standby = true;
 		}
 		else
+		{
+			/* already caught up and doesn't need to wait for standby_slots */
 			break;
+		}
 
 		/* Waiting for new WAL. Since we need to wait, we're now caught up. */
 		WalSndCaughtUp = true;

---

From: Peter Smith <[email-masked]>
Date: Fri, 8 Dec 2023 09:58:52 +1100

Hi.

Here is another review comment for the patch v43-0001.

======
src/bin/pg_dump/pg_dump.c

1. getSubscriptions

+ if (fout->remoteVersion >= 170000)
+ appendPQExpBufferStr(query,
+ " subfailoverstate\n");
+ else
+ appendPQExpBuffer(query,
+   " '%c' AS subfailoverstate\n",
+   LOGICALREP_FAILOVER_STATE_DISABLED);
+

That first appended string should include the table alias same as all
the nearby code does.

e.g. " subfailoverstate\n" should be " s.subfailoverstate\n"

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

---

From: shveta malik <[email-masked]>
Date: Fri, 8 Dec 2023 08:33:05 +0530

> (as XLogRecPtrIsInvalid(WalRcv->latestWalEnd) would be true).

Sure, I will add it. Thanks for the clarification.

---

From: Amit Kapila <[email-masked]>
Date: Fri, 8 Dec 2023 14:36:19 +0530

> PFA v43, changes are:

I wanted to discuss 0003 patch about cascading standby's. It is not
clear to me whether we want to allow physical standbys to further wait
for cascading standby to sync their slots. If we allow such a feature
one may expect even primary to wait for all the cascading standby's
because otherwise still logical subscriber can be ahead of one of the
cascading standby. I feel even if we want to allow such a behaviour we
can do it later once the main feature is committed. I think it would
be good to just allow logical walsenders on primary to wait for
physical standbys represented by GUC 'standby_slot_names'. If we agree
on that then it would be good to prohibit setting this GUC on standby
or at least it should be a no-op even if this GUC should be set on
physical standby.

Thoughts?

-- 
With Regards,
Amit Kapila.

---

From: "Zhijie Hou (Fujitsu)" <[email-masked]>
Date: Fri, 8 Dec 2023 12:05:50 +0000

On Thursday, December 7, 2023 7:37 PM Amit Kapila <[email-masked]> wrote:
> + * However, we do not enable failover for slots created by the table
> + sync
> + * worker. This is because the table sync slot might not be fully
> + synced on the
> + * standby.
> 
> The reason for not enabling failover for table sync slots is not clearly
> mentioned.
> 
> 2.
> + * the newly created slot on the standby are typically ahead of those
> + on the
> + * primary. Therefore, the standby needs to wait for the primary
> + server's
> probably describe a bit more about why slot on standby will be ahead and
> about waiting time.
> 
> 3.
> validate_standby_slots()
> {
> ...
> + name); goto ret_standby_slot_names_ng; }
> 
> Why the first check (slot not found) doesn't have errdetail? The goto's in this
> function look a bit odd, can we try to avoid those?
> 
> 4.
> + /* Verify syntax and parse string into list of identifiers */ if
> + (!SplitIdentifierString(rawname, ',', &elemlist)) {
> + if (!SplitIdentifierString(standby_slot_names_cpy, ',',
> + &standby_slots)) {
> I think the error message should be the same in both cases. The first one seems
> better.
> 
> 5. In WalSndFilterStandbySlots(), the comments around else if checks should
> move inside the checks. It is hard to read the code in the current format. I have
> tried to change the same in the attached.
> 
> cosmetic changes in the attached. Kindly include in next version if you are fine
> with it.

Thanks for the comments and changes, I have addressed them.
Here is the V44 patch set which addressed comments above and [1].

The new version patches also include the follow changes:

V44-0001
* Let the pg_replication_slot_advance also wait for the slots specified
in standby_slot_names to catch up.
* added few test cases to cover the wait/wakeup logic in
walsender related to standby_slot_names.
* ran pgindent.

V44-0002
* added few comments to explain the case when the slot is valid on primary
while is invalidated on standby.

Thanks Ajin for analyzing and making the tests.

The pending comments on 0002 will be addressed in next version.

[1] https://www.postgresql.org/message-id/CAHut%2BPvRD5V-zzTvffDdcnqB1T4JNATKGgw%2BwdQCKAgeCYr0xQ%40mail.gmail.com

Best Regards,
Hou zj

---

From: Amit Kapila <[email-masked]>
Date: Sun, 10 Dec 2023 16:33:35 +0530

> v43-002:

Review comments on v43-0002:
=========================
1.
synchronize_one_slot()
{
...
+ /*

After the error, the control won't return, so the above goto doesn't
make any sense.

2.
synchronize_one_slot()
{
...
+ /* Search for the named slot */
+ if ((s = SearchNamedReplicationSlot(remote_slot->name, true)))
+ {
+ SpinLockAcquire(&s->mutex);
+ sync_state = s->data.sync_state;
+ SpinLockRelease(&s->mutex);
+ }
...
...
+ ReplicationSlotAcquire(remote_slot->name, true);
+
+ /*
+ * Copy the invalidation cause from remote only if local slot is not
+ * invalidated locally, we don't want to overwrite existing one.
+ */
+ if (MyReplicationSlot->data.invalidated == RS_INVAL_NONE)
+ {
+ SpinLockAcquire(&MyReplicationSlot->mutex);
+ MyReplicationSlot->data.invalidated = remote_slot->invalidated;
+ SpinLockRelease(&MyReplicationSlot->mutex);
+ }
+
+ /* Skip the sync if slot has been invalidated locally. */
+ if (MyReplicationSlot->data.invalidated != RS_INVAL_NONE)
+ goto cleanup;
...

It seems useless to acquire the slot if it is locally invalidated in
the first place. Won't it be better if after the search we first check
whether the slot is locally invalidated and take appropriate action?

3. After doing the above two, I think it doesn't make sense to have
goto at the remaining places in synchronize_one_slot(). We can simply
release the slot and commit the transaction at other places.

4.

Returning nap time from here appears a bit awkward. I think it is
better if this function returns any_slot_updated and then the caller
decides the adjustment of naptime.

5.
+synchronize_slots(WalReceiverConn *wrconn)
{
...
...
+ /* The syscache access needs a transaction env. */
+ StartTransactionCommand();
+
+ /*
+ * Make result tuples live outside TopTransactionContext to make them
+ * accessible even after transaction is committed.
+ */
+ MemoryContextSwitchTo(oldctx);
+
+ /* Construct query to get slots info from the primary server */
+ initStringInfo(&s);
+ construct_slot_query(&s);
+
+ elog(DEBUG2, "slot sync worker's query:%s \n", s.data);
+
+ /* Execute the query */
+ res = walrcv_exec(wrconn, s.data, SLOTSYNC_COLUMN_COUNT, slotRow);
+ pfree(s.data);
+
+ if (res->status != WALRCV_OK_TUPLES)
+ ereport(ERROR,
+ (errmsg("could not fetch failover logical slots info "
+ "from the primary server: %s", res->err)));
+
+ CommitTransactionCommand();
...
...
}

Where exactly in the above code, there is a syscache access as
mentioned above StartTransactionCommand()?

6.
-          <filename>~/.pgpass</filename> file on the standby server (use
+          <filename>~/.pgpass</filename> file on the standby server. (use
           <literal>replication</literal> as the database name).

Why do we need this change?

7.

In this, I don't understand the relation between the first part of the
line: "Additionally, similar to creating a logical replication slot on
the hot standby ..." with the rest.

8.
However,

A single quote after 'i' is missing.

9.
the slots with state 'r' and 'i' can neither be used for logical
+      decoded nor dropped by the user.

/decoded/decoding

10.
+/*
+ * Allocate and initialize slow sync worker shared memory
+ */

/slow/slot

-- 
With Regards,
Amit Kapila.

---

From: Peter Smith <[email-masked]>
Date: Mon, 11 Dec 2023 11:16:52 +1100

FYI -- the patch 0002 did not apply cleanly for me on top of the 050
test file created by patch 0001.

[postgres@CentOS7-x64 oss_postgres_misc]$ git apply
../patches_misc/v44-0001-Allow-logical-walsenders-to-wait-for-the-physica.patch

[postgres@CentOS7-x64 oss_postgres_misc]$ git apply
../patches_misc/v44-0002-Add-logical-slot-sync-capability-to-the-physical.patch
error: patch failed: src/test/recovery/t/050_standby_failover_slots_sync.pl:289
error: src/test/recovery/t/050_standby_failover_slots_sync.pl: patch
does not apply

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

---

From: "Zhijie Hou (Fujitsu)" <[email-masked]>
Date: Mon, 11 Dec 2023 02:19:43 +0000

On Monday, December 11, 2023 8:17 AM Peter Smith <[email-masked]> wrote:
> 
> FYI -- the patch 0002 did not apply cleanly for me on top of the 050 test file
> created by patch 0001.
> 
> [postgres@CentOS7-x64 oss_postgres_misc]$ git
> apply ../patches_misc/v44-0001-Allow-logical-walsenders-to-wait-for-the-ph
> ysica.patch
> 
> [postgres@CentOS7-x64 oss_postgres_misc]$ git
> apply ../patches_misc/v44-0002-Add-logical-slot-sync-capability-to-the-phy
> sical.patch
> error: src/test/recovery/t/050_standby_failover_slots_sync.pl: patch does not
> apply

Thanks for reporting. Here is the rebased patch set V44_2.
(There are no code changes in this version.)

Best Regards,
Hou zj

---

From: Peter Smith <[email-masked]>
Date: Mon, 11 Dec 2023 18:32:24 +1100

Here are some review comments for v44-0001

======

1. ReplicationSlotCreate

  *     during getting changes, if the two_phase option is enabled it can skip
  *     prepare because by that time start decoding point has been moved. So the
  *     user will only get commit prepared.
+ * failover: Allows the slot to be synced to physical standbys so that logical
+ *     replication can be resumed after failover.
  */
 void
 ReplicationSlotCreate(const char *name, bool db_specific,

~

/Allows the slot.../If enabled, allows the slot.../

======

2. validate_standby_slots

+validate_standby_slots(char **newval)
+{
+ char    *rawname;
+ List    *elemlist;
+ ListCell   *lc;
+ bool ok = true;
+
+ /* Need a modifiable copy of string */
+ rawname = pstrdup(*newval);
+
+ /* Verify syntax and parse string into list of identifiers */
+ if (!(ok = SplitIdentifierString(rawname, ',', &elemlist)))
+ GUC_check_errdetail("List syntax is invalid.");
+
+ /*
+ * If there is a syntax error in the name or if the replication slots'
+ * data is not initialized yet (i.e., we are in the startup process), skip
+ * the slot verification.
+ */
+ if (!ok || !ReplicationSlotCtl)
+ {
+ pfree(rawname);
+ list_free(elemlist);
+ return ok;
+ }

2a.
You don't need to initialize 'ok' during declaration because it is
assigned immediately anyway.

~

2b.
AFAIK assignment within a conditional like this is not a normal PG
coding style unless there is no other way to do it.

~

2c.
/into list/into a list/

SUGGESTION
/* Verify syntax and parse string into a list of identifiers */
ok = SplitIdentifierString(rawname, ',', &elemlist);
if (!ok)
  GUC_check_errdetail("List syntax is invalid.");

~~~

3. assign_standby_slot_names

+ if (!SplitIdentifierString(standby_slot_names_cpy, ',', &standby_slots))
+ {
+ /* This should not happen if GUC checked check_standby_slot_names. */
+ elog(ERROR, "list syntax is invalid");
+ }

This error here and in validate_standby_slots() are different --
"list" versus "List".

======

4. WalSndFilterStandbySlots

+ foreach(lc, standby_slots_cpy)
+ {
+ char    *name = lfirst(lc);
+ XLogRecPtr restart_lsn = InvalidXLogRecPtr;
+ bool invalidated = false;
+ char    *warningfmt = NULL;
+ ReplicationSlot *slot;
+
+ slot = SearchNamedReplicationSlot(name, true);
+
+ if (slot && SlotIsPhysical(slot))
+ {
+ SpinLockAcquire(&slot->mutex);
+ restart_lsn = slot->data.restart_lsn;
+ invalidated = slot->data.invalidated != RS_INVAL_NONE;
+ SpinLockRelease(&slot->mutex);
+ }
+
+ /* Continue if the current slot hasn't caught up. */
+ if (!invalidated && !XLogRecPtrIsInvalid(restart_lsn) &&
+ restart_lsn < wait_for_lsn)
+ {
+ /* Log warning if no active_pid for this physical slot */
+ if (slot->active_pid == 0)
+ ereport(WARNING,
+ errmsg("replication slot \"%s\" specified in parameter \"%s\" does
not have active_pid",
+    name, "standby_slot_names"),
+ errdetail("Logical replication is waiting on the "
+   "standby associated with \"%s\"", name),
+ errhint("Consider starting standby associated with "
+ "\"%s\" or amend standby_slot_names", name));
+
+ continue;
+ }
+ else if (!slot)
+ {
+ /*
+ * It may happen that the slot specified in standby_slot_names GUC
+ * value is dropped, so let's skip over it.
+ */
+ warningfmt = _("replication slot \"%s\" specified in parameter
\"%s\" does not exist, ignoring");
+ }
+ else if (SlotIsLogical(slot))
+ {
+ /*
+ * If a logical slot name is provided in standby_slot_names, issue
+ * a WARNING and skip it. Although logical slots are disallowed in
+ * the GUC check_hook(validate_standby_slots), it is still
+ * possible for a user to drop an existing physical slot and
+ * recreate a logical slot with the same name. Since it is
+ * harmless, a WARNING should be enough, no need to error-out.
+ */
+ warningfmt = _("cannot have logical replication slot \"%s\" in
parameter \"%s\", ignoring");
+ }
+ else if (XLogRecPtrIsInvalid(restart_lsn) || invalidated)
+ {
+ /*
+ * Specified physical slot may have been invalidated, so there is no point
+ * in waiting for it.
+ */
+ warningfmt = _("physical slot \"%s\" specified in parameter \"%s\"
has been invalidated, ignoring");
+ }
+ else
+ {
+ Assert(restart_lsn >= wait_for_lsn);
+ }

This if/else chain seems structured awkwardly. IMO it would be tidier
to eliminate the NULL slot and IsLogicalSlot up-front, which would
also simplify some of the subsequent conditions

SUGGESTION

slot = SearchNamedReplicationSlot(name, true);

if (!slot)
{
...
}
else if (SlotIsLogical(slot))
{
...
}
else
{
  Assert(SlotIsPhysical(slot))

  SpinLockAcquire(&slot->mutex);
  restart_lsn = slot->data.restart_lsn;
  invalidated = slot->data.invalidated != RS_INVAL_NONE;
  SpinLockRelease(&slot->mutex);

  if (XLogRecPtrIsInvalid(restart_lsn) || invalidated)
  {
  ...
  }
  else if (!invalidated && !XLogRecPtrIsInvalid(restart_lsn) &&
restart_lsn < wait_for_lsn)
  {
  ...
  }
  else
  {
    Assert(restart_lsn >= wait_for_lsn);
  }
}

~~~~

5. WalSndWaitForWal

+ else
+ {

/Already/already/

======
src/test/recovery/t/050_standby_failover_slots_sync.pl

6.
+$subscriber1->safe_psql('postgres',
+ "CREATE TABLE tab_int (a int PRIMARY KEY);");
+
+# Create a subscription with failover = true
+$subscriber1->safe_psql('postgres',
+     "CREATE SUBSCRIPTION regress_mysub1 CONNECTION '$publisher_connstr' "
+   . "PUBLICATION regress_mypub WITH (slot_name = lsub1_slot,
failover = true);"
+);

Consider combining these DDL statements.

~~~

7.
+$subscriber2->safe_psql('postgres',
+ "CREATE TABLE tab_int (a int PRIMARY KEY);");
+$subscriber2->safe_psql('postgres',
+     "CREATE SUBSCRIPTION regress_mysub2 CONNECTION '$publisher_connstr' "
+   . "PUBLICATION regress_mypub WITH (slot_name = lsub2_slot);");

Consider combining these DDL statements

~~~

8.
+# Stop the standby associated with specified physical replication slot so that
+# the logical replication slot won't receive changes until the standby slot's
+# restart_lsn is advanced or the slots is removed from the standby_slot_names
+# list
+$publisher->safe_psql('postgres', "TRUNCATE tab_int;");
+$publisher->wait_for_catchup('regress_mysub1');
+$standby1->stop;

/with specified/with the specified/

/or the slots is/or the slot is/

~~~

9.
+# Create some data on primary

/on primary/on the primary/

~~~

10.
+$result =
+  $subscriber1->safe_psql('postgres', "SELECT count(*) = 10 FROM tab_int;");
+is($result, 't',
+ "subscriber1 doesn't get data as the sb1_slot doesn't catch up");

I felt instead of checking for 10 maybe it's more consistent with the
previous code to assign again that $primary_row_count variable to 20;

Then check that those primary rows are not all yet received like:

SELECT count(*) < $primary_row_count FROM tab_int;

~~~

11.
+# Now that the standby lsn has advanced, primary must send the decoded
+# changes to the subscription.
+$publisher->wait_for_catchup('regress_mysub1');
+$result =
+  $subscriber1->safe_psql('postgres', "SELECT count(*) = 20 FROM tab_int;");
+is($result, 't',
+ "subscriber1 gets data from primary after standby1 is removed from
the standby_slot_names list"
+);

/primary must/the primary must/

(continuing the suggestion from the previous review comment)

Now this SQL can use the variable too:

subscriber1->safe_psql('postgres', "SELECT count(*) =
$primary_row_count FROM tab_int;");

~~~

12.
+
+# Create another subscription enabling failover
+$subscriber1->safe_psql('postgres',
+     "CREATE SUBSCRIPTION regress_mysub3 CONNECTION '$publisher_connstr' "
+   . "PUBLICATION regress_mypub WITH (slot_name = lsub3_slot,
copy_data=false, failover = true, create_slot = false);"
+);

Maybe give some more information in that comment:

SUGGESTION
Create another subscription (using the same slot created above) that
enables failover.

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

---

From: "Drouvot, Bertrand" <[email-masked]>
Date: Mon, 11 Dec 2023 08:52:28 +0100

Hi,

On 12/8/23 10:06 AM, Amit Kapila wrote:
> cascading standby.

I've the same feeling here. I think it would probably be expected that
the primary also wait for all the cascading standby.

> I feel even if we want to allow such a behaviour we
> can do it later once the main feature is committed. 

Agree.

> I think it would
> physical standbys represented by GUC 'standby_slot_names'.

That makes sense for me for v1.

> If we agree

I'd prefer to completely prohibit it on standby (to make it very clear it's not
working at all) as long as one can enable it without downtime once the standby
is promoted (which is the case currently).

Regards,

---

From: Dilip Kumar <[email-masked]>
Date: Mon, 11 Dec 2023 13:47:19 +0530

> or at least it should be a no-op even if this GUC should be set on

IMHO, why not keep the behavior consistent across primary and standby?
 I mean if it doesn't require a lot of new code/design addition then
it should be the user's responsibility.  I mean if the user has set
'standby_slot_names' on standby then let standby also wait for
cascading standby to sync their slots?  Is there any issue with that
behavior?

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

---

From: Amit Kapila <[email-masked]>
Date: Mon, 11 Dec 2023 14:05:30 +0530

> This error here and in validate_standby_slots() are different --

Note here elog(ERROR,.. is used and in the other place it is part of
the detail message. I have suggested in my previous review to make
them the same but I overlooked the difference, so I think we should
change the message to "invalid list syntax" as it was there
previously.

>     Assert(restart_lsn >= wait_for_lsn);

+1.

-- 
With Regards,
Amit Kapila.

---

From: shveta malik <[email-masked]>
Date: Mon, 11 Dec 2023 14:20:51 +0530

> cascading standby to sync their slots?  Is there any issue with that

Without waiting for cascading standby on primary, it won't be helpful
to just wait on standby.

Currently logical walsenders on primary waits for physical standbys to
take changes before they update their own logical slots. But they wait
only for their immediate standbys and not for cascading standbys.
Although, on first standby, we do have logic where slot-sync workers
wait for cascading standbys before they update their own slots (synced
ones, see patch3). But this does not guarantee that logical
subscribers on primary will never be ahead of the cascading standbys.
Let us consider this timeline:

t1: logical walsender on primary waiting for standby1 (first standby).
t2: physical walsender on standby1 is stuck and thus there is delay in
sending these changes to standby2 (cascading standby).
t3: standby1 has taken changes and sends confirmation to primary.
t4: logical walsender on primary receives confirmation from standby1
and updates slot, logical subscribers of primary also receives the
changes.
t5: standby2 has not received changes yet as physical walsender on
standby1 is still stuck, slotsync worker still waiting for standby2
(cascading) before it updates its own slots (synced ones).
t6: standby2 is promoted to become primary.

Now we are in a state wherein primary, logical subscriber and first
standby has some changes but cascading standby does not. And logical
slots on primary were updated w/o confirming if cascading standby has
taken changes or not. This is a problem and we do not have a simple
solution for this yet.

thanks
Shveta

---

From: shveta malik <[email-masked]>
Date: Mon, 11 Dec 2023 14:32:45 +0530

> taken changes or not. This is a problem and we do not have a simple

PFA v45, changes in patch002:

--Addressed comments in [1] and [2]
--Added holistic test case for patch02. Thanks Nisha for the test
implementation.

[1]: https://www.postgresql.org/message-id/CAHut%2BPuuqEpDse5msENsVuK3rjTRN-QGS67rRCGVv%2BzcT-f0GA%40mail.gmail.com
[2]: https://www.postgresql.org/message-id/CAA4eK1KbhdjKqui%3Dfr4Ny2TwGAFU9WLWTdypN%2BWG0WEfnBR%3D4w%40mail.gmail.com

thanks
Shveta

---

From: shveta malik <[email-masked]>
Date: Mon, 11 Dec 2023 14:41:26 +0530

> Review comments on v43-0002:

Thanks for the feedback Amit. Addressed these in v45. Please find my
response on a few of these.

> whether the slot is locally invalidated and take appropriate action?

 If we don't acquire the slot first, there could be a race condition
that the local slot could be invalidated just after checking the
invalidated flag. See InvalidatePossiblyObsoleteSlot() where it
invalidates slot directly if the slot is not acquired by other
processes. Thus, I have not removed 'ReplicationSlotAcquire' but I
have re-structured the code a little bit to get rid of duplicate code
in 'if' and 'else' part for invalidation logic.

> mentioned above StartTransactionCommand()?

It is in walrcv_exec (libpqrcv_processTuples). I have changed the
comments to add this info.

> Why do we need this change?

We don't, removed it.

---

From: shveta malik <[email-masked]>
Date: Mon, 11 Dec 2023 15:00:50 +0530

> Here are my review comments for patch v43-0002.

Thanks for the feedback. I have addressed most of these in v45. Please
find my response on a few which are pending or are not needed.

> later review comment about it (in the sgml file)

I have rephrased it, please check now and let me know.

> optional? I thought the GUC validation function mandates these (???).

standby_slot_names setting is not mandatory, it is recommended though.
OTOH hot_standby_feedback setting is mandatory. So I have changed
accordingly.

> have physical replication slot between the primary and the standby"

Removed the duplicate line.

> details, which the user does not need to know about?

 'sync_state' is a way to monitor the state of synchronization and I
feel it is important to tell what happens with 'i' state slots. Also
there was a comment to add this info in doc that disabled
subscriptions are not guaranteed to be usable if enabled after
failover. Thus it was added and rest of the info forms a base for
that. We can trim down or rephrase if needed.

> 'e' (enabled) like the other tri-state attributes are using.

I think it is not a property of a slot where we say enabled/disabled.
It is more like an operation and thus initiated, ready etc sounds
better. These states are similar to the ones maintained for table-sync
operation (SUBREL_STATE_INIT, SUBREL_STATE_READY etc)

> "sync_state as 'r' and 'n' will behave the same" sounds kind of hacky.

I am reviewing your suggestion on 'r' to 'n' conversion on promotion
given later in this email. So give me some more time.

> which I took as implying that it is *not* meaningful from the POV of

In case we planned to retain 'r', it then makes sense to document that
the sync_state on primary can also be 'r' if the primary was promoted
from a standby, because this is a special case which the user may not
be aware of.

> would be: 'standby_sync_state'

sync_state has some value for primary too. It is not null on primary.
Thus the current name seems a better choice.

> on a node that is no longer a standby node?

yes, this is the place. But this needs more thought as it may cause
too much disk activity during promotion. so let me analyze and come
back.

> conspicuous absence of a comment here.

Added some comments, but not very confident on those, so let me know.

> then anyway it should be fixed in patch 0001.

yes, it should go to patch01. Done.

> Should the errdetail give the current state?

I think it is not needed, current info looks good enough. User can
always use pg_replication_slots to monitor sync_state info.

> comment in a couple of places.

I have updated header of file with details and gave reference here and
all such similar places.

> here might be more appropriate than hardwiring 'i'. Why have a

On hold. I could not find quote_* function for a character just like
we have 'quote_literal_cstr' for string. Will review. Let me know if
you know.

> SYNCSLOT_STATE_NONE instead?

No, that will change the flow. It should stay uninitialized if the
slot is not found. I have changed assignment to '\0' for better
clarity.

> code appears to assume it was false already which may or may not be

It is initialized to false in the caller, so we are good here.

> whereas including an Assert will clarify that it must be set.

 I think assert is not needed. Slot-sync worker will never start if
hot_standby_feedback is disabled. If we put assert here, we need to
assert at all other places too where we use other related GUCs like
primary_slot_name, conn_info etc.

> normal value (as I suggested above) instead of the strange assignment

 I feel NULL character ('\0') is better default for local variable
sync_slot as we specifically wanted it to be NULL if not assigned.
Assigning it to 'SYNCSLOT_STATE_NONE' will be misleading. But moved
the 'SYNCSLOT_STATE_NONE' related error though.

> + ReplicationSlotPersist();

yes, it will make code simpler. Changed it.

> WORKER_INACTIVITY_NAPTIME_MS here?

This may happen if walreceiver is temporarily having some issue.
Longer nap is not recommended here. We should check the state again
after a short nap.

> different name would be better like 'remote_slottup' or something

Have changed it to 'tupslot' to keep it short but different from slot.

> OTOH if this is not possible, then the slotsync_reread_config() might

'enable_syncslot' is recently changed to PGC_POSTMASTER from
PGC_SIGHUP. Thus 'slotsync_reread_config' also needs to get rid of
'enable_syncslot'. I have changed that now. Slightly changed the
comment as well.

> (conninfoChanged) so it can be done later in the function.

Once PrimaryConnInfo is changed, we can not get old-dbname. So it is
required to be done before we reach 'conninfoChanged'

> src/backend/replication/slot.c

There is some change in way headers are included. I need to review it
in detail. Keeping it on hold. I tried to explain few points on this
in [1] (see last comment)
[1]: https://www.postgresql.org/message-id/CAJpy0uD6dWUvBgy8MGdugf_Am4pLXTL_vqcwSeHO13v%2BMzc9KA%40mail.gmail.com

> Should the errdetail give the current state?

I feel current info looks good. User can always use
pg_replication_slots to monitor sync_state info.

> #define SYNCSLOT_STATE_ENABLED  'e' /* Sync is enabled and the slot is

responded in comment 11.

> If correct, IMO a better field name might be 'standby_sync_state'

 sync_state has some value for primary too. It is not null on primary.
Thus current name seems a better choice.

---

From: Dilip Kumar <[email-masked]>
Date: Mon, 11 Dec 2023 16:47:50 +0530

> taken changes or not. This is a problem and we do not have a simple

Okay, I think that makes sense.

---

From: shveta malik <[email-masked]>
Date: Mon, 11 Dec 2023 17:13:37 +0530

> is promoted (which is the case currently).

And I think slot-sync worker should exit as well on cascading standby. Thoughts?

If we agree on the above, then we need to look for a way to
distinguish between first and cascading standby. I could not find any
existing way to do so. One possible approach is to connect to the
remote using PrimaryConninfo and run 'pg_is_in_recovery()' there, if
it returns true, then it means we are cascading standby.  Any simpler
way to achieve this?

thanks
Shveta

---

From: Amit Kapila <[email-masked]>
Date: Mon, 11 Dec 2023 19:12:39 +0530

> comments to add this info.

Okay, I see that the patch switches context twice once after starting
the transaction and the second time after committing the transaction,
why is that required? Also, can't we extend the duration of the
transaction till the remote_slot information is constructed? I am
asking this because the context used is TopMemoryContext which should
be used only if we need something specific to be retained at the
process level which doesn't seem to be the case here.

I have noticed a few other minor things:
1.
postgres=# select * from pg_logical_slot_get_changes('log_slot_2', NULL, NULL);
ERROR:  cannot use replication slot "log_slot_2" for logical decoding
DETAIL:  This slot is being synced from the primary server.
...
...
postgres=# select * from pg_drop_replication_slot('log_slot_2');
ERROR:  cannot drop replication slot "log_slot_2"
DETAIL:  This slot is being synced from the primary.

I think the DETAIL message should be the same in the above two cases.

2.
+void
+WalSndWaitForStandbyConfirmation(XLogRecPtr wait_for_lsn)
+{
+ List    *standby_slots;
+
+ Assert(!am_walsender);
+
+ if (!MyReplicationSlot->data.failover)
+ return;
+
+ standby_slots = GetStandbySlotList(true);
+
+ ConditionVariablePrepareToSleep(&WalSndCtl->wal_confirm_rcv_cv);
...
...

Shouldn't we return if the standby slot names list is NIL unless there
is a reason to do ConditionVariablePrepareToSleep() or any of the code
following it?

-- 
With Regards,
Amit Kapila.

---

From: shveta malik <[email-masked]>
Date: Tue, 12 Dec 2023 14:44:28 +0530

On Mon, Dec 11, 2023 at 7:12 PM Amit Kapila <[email-masked]> wrote:
> transaction till the remote_slot information is constructed?

If we extend duration, we have to extend till remote_slot information
is consumed and not only till it is constructed.

> I am

Okay, I understand your concern. But this needs more thoughts on shall
we have all the slots synchronized in one txn or is it better to have
it existing way i.e. each slot being synchronized in its own txn
started in synchronize_one_slot. If we go by the former, can it have
any implications? I need to review this bit more before concluding.
.

---

From: "Zhijie Hou (Fujitsu)" <[email-masked]>
Date: Tue, 12 Dec 2023 09:46:05 +0000

On Monday, December 11, 2023 3:52 PM Drouvot, Bertrand <[email-masked]> wrote:

Hi,

> I've the same feeling here. I think it would probably be expected that the
> primary also wait for all the cascading standby.
> 
> > I feel even if we want to allow such a behaviour we can do it later
> > once the main feature is committed.
> 
> Agree.
> 
> > I think it would

I think we could not check if we are in a standby server in the GUC check_hook,
because the XLogCtl(which is checked in RecoveryInProgress) may have not been
initialized yet. Besides, other GUCs like synchronous_standby_names also don't
work on standby but it will be no-op. So I feel we can also ignore
standby_slot_names on standby. What do you think ?

Here is the V46 patch set which changed the following things:

V46-0001:
* Address Peter[1] and Amit's[2] comments.
* Fix one CFbot failure in meson build.
* Ignore the standby_slot_names on a standby server since we don't support
  syncing slots to cascade standby.

V46-0002:
1) Fix for CFBot make warning.
2) Cascading support removal. Now we do not need to check 'sync_state != 'i''
   in the query while fetching failover slots. This check was needed on the
   cascading standby to fetch failover slots from the first standby.
3) Test correction and optimization.

0003 patch is removed since we agreed not to support syncing slots to cascading
standby.

Thanks Shveta for working on the changes in V46-0002 and thanks Ajin for
working on the test optimization.

--
TODO

There are few pending comments that mentioned in [3][4][5] which are still in
progress.

[1] https://www.postgresql.org/message-id/CAHut%2BPsf9z132WNgy0Gr10ZTnonpNjvTBj74wG8kSxXU4rOD7g%40mail.gmail.com
[2] https://www.postgresql.org/message-id/CAA4eK1%2BCXpfiTLbYRaOoUBP9Z1-xJZdX6QOp14rCdaF5E2gsgQ%40mail.gmail.com
[3] https://www.postgresql.org/message-id/CAJpy0uDaGMNpgmdxie-MgHmMhnD4ET_LDjQNEe76xJ%2BMLqRQ8Q%40mail.gmail.com
[4] https://www.postgresql.org/message-id/CAJpy0uDcOf5Hvk_CdCCAbfx9SY%2Bog%3D%3D%3DtgiuhWKzkYyqebui9g%40mail.gmail.com
[5] https://www.postgresql.org/message-id/CAJpy0uC-8mrn6jakcFjSVmbJiHZs-Okq8YKxGfrMLPD-2%3DwOqQ%40mail.gmail.com

Best Regards,
Hou zj

---

From: "Zhijie Hou (Fujitsu)" <[email-masked]>
Date: Tue, 12 Dec 2023 09:47:49 +0000

On Monday, December 11, 2023 3:32 PM Peter Smith <[email-masked]>

Changed.

> + /* Need a modifiable copy of string */ rawname = pstrdup(*newval);
> +
> + /* Verify syntax and parse string into list of identifiers */ if (!(ok
> + = SplitIdentifierString(rawname, ',', &elemlist)))
> + * data is not initialized yet (i.e., we are in the startup process),
> + skip
> You don't need to initialize 'ok' during declaration because it is assigned
> immediately anyway.
> 
> ~
> 
> 2b.
> AFAIK assignment within a conditional like this is not a normal PG coding style
> unless there is no other way to do it.
> 

Changed.

> ~
> 
> 2c.
> /into list/into a list/
> 
> SUGGESTION
> /* Verify syntax and parse string into a list of identifiers */ ok =
> SplitIdentifierString(rawname, ',', &elemlist); if (!ok)

Changed.

> + elog(ERROR, "list syntax is invalid"); }
> 
> This error here and in validate_standby_slots() are different -- "list" versus
> "List".
> 

The message has been changed to "invalid list syntax" to be consistent with other elog.

> + XLogRecPtr restart_lsn = InvalidXLogRecPtr; bool invalidated = false;
> + SpinLockRelease(&slot->mutex); }
> +
> + /* Continue if the current slot hasn't caught up. */ if (!invalidated
> + && !XLogRecPtrIsInvalid(restart_lsn) && restart_lsn < wait_for_lsn) {
> + (slot->active_pid == 0) ereport(WARNING, errmsg("replication slot
> + \"%s\" specified in parameter \"%s\" does
> not have active_pid",
> +   "standby associated with \"%s\"", name), errhint("Consider starting
> + standby associated with "
> + else if (XLogRecPtrIsInvalid(restart_lsn) || invalidated) {
> + /*
> + * Specified physical slot may have been invalidated, so there is no
> + point
> + * in waiting for it.
> + */
> This if/else chain seems structured awkwardly. IMO it would be tidier to
> eliminate the NULL slot and IsLogicalSlot up-front, which would also simplify
> some of the subsequent conditions
> 
> SUGGESTION
> 
>   else if (!invalidated && !XLogRecPtrIsInvalid(restart_lsn) && restart_lsn <
> wait_for_lsn)
>   {
>   ...
>   }
>   else
>   {

Changed.

Changed.

Changed.

Changed.

> ~~~
> 
> 8.
> +# Stop the standby associated with specified physical replication slot
> +so that # the logical replication slot won't receive changes until the
> +standby slot's # restart_lsn is advanced or the slots is removed from
> +the standby_slot_names # list $publisher->safe_psql('postgres',
> +"TRUNCATE tab_int;"); $publisher->wait_for_catchup('regress_mysub1');
> +$standby1->stop;
> 

Changed.

Changed.

> ~~~
> 
> 10.
> +$result =
> +  $subscriber1->safe_psql('postgres', "SELECT count(*) = 10 FROM
> +tab_int;"); is($result, 't',
> I felt instead of checking for 10 maybe it's more consistent with the previous
> code to assign again that $primary_row_count variable to 20;
> 

I think we'd better check the accurate number here to make sure the number is what we expect.

> +  $subscriber1->safe_psql('postgres', "SELECT count(*) = 20 FROM
> +tab_int;"); is($result, 't',

Changed.

> Create another subscription (using the same slot created above) that enables
> failover.
> 

Added.

Best Regards,
Hou zj

---

From: Nisha Moond <[email-masked]>
Date: Tue, 12 Dec 2023 17:56:43 +0530

A review on v45 patch:

If one creates a logical slot with failover=true as -
select pg_create_logical_replication_slot('logical_slot','pgoutput',
false, true, true);

Then, uses the existing logical slot while creating a subscription -
postgres=#  create subscription sub4 connection 'dbname=postgres
host=localhost port=5433' publication pub1t4 WITH
(slot_name=logical_slot, create_slot=false, failover=true);
NOTICE:  changed the failover state of replication slot "logical_slot"
on publisher to false
CREATE SUBSCRIPTION

Despite configuring logical_slot's failover to true and specifying
failover=true during subscription creation, the NOTICE indicates a
change in the failover state to 'false', without providing any
explanation for this transition.
It can be confusing for users, so IMO, the notice should include the
reason for switching failover to 'false' or should give a hint to use
either refresh=false or copy_data=false to enable failover=true for
the slot as we do in other similar 'alter subscription...' scenarios.

--
Thanks & Regards,
Nisha

---

From: Amit Kapila <[email-masked]>
Date: Wed, 13 Dec 2023 09:01:25 +0530

On Tue, Dec 12, 2023 at 2:44 PM shveta malik <[email-masked]> wrote:
> any implications?

I think the one advantage of syncing each slot in a different
transaction could have been if that helps with the visibility of
updated slot information but that is not the case here as we always
persist it to file. As per my understanding, here we need a
transaction as we may access catalogs while creating/updating slots,
so, a single transaction should be okay unless there are any other
reasons.

-- 
With Regards,
Amit Kapila.

---

From: Amit Kapila <[email-masked]>
Date: Wed, 13 Dec 2023 10:40:24 +0530

> And I think slot-sync worker should exit as well on cascading standby. Thoughts?

I think one has set all the valid parameters for the slot-sync worker
on standby, we should not exit, rather it should be no-op which means
it should not try to sync slots from another standby. One scenario
where this may help is when users promote the standby which has
already synced slots from the primary. In this case, cascading standby
will become non-cascading and should sync slots.

-- 
With Regards,
Amit Kapila.

---

From: shveta malik <[email-masked]>
Date: Wed, 13 Dec 2023 11:39:07 +0530

> the slot as we do in other similar 'alter subscription...' scenarios.

Agree. The NOTICE should be more informative.

thanks
SHveta

---

From: shveta malik <[email-masked]>
Date: Wed, 13 Dec 2023 11:42:42 +0530

> will become non-cascading and should sync slots.

Right, then perhaps we should increase naptime in this no-op case. It
could be even more then current inactivity naptime which is just
10sec. Shall it be say 5min in this case?

thanks
Shveta

---

From: Peter Smith <[email-masked]>
Date: Wed, 13 Dec 2023 21:22:54 +1100

Hi Shveta, here are some review comments for v45-0002.

======

1.
+   <variablelist>
+    <varlistentry>
+     <term><literal>BgWorkerStart_PostmasterStart</literal></term>
+     <listitem>
+      <para>
+       <indexterm><primary>BgWorkerStart_PostmasterStart</primary></indexterm>
+       Start as soon as postgres itself has finished its own initialization;
+       processes requesting this are not eligible for database connections.
+      </para>
+     </listitem>
+    </varlistentry>
+
+    <varlistentry>
+     <term><literal>BgWorkerStart_ConsistentState</literal></term>
+     <listitem>
+      <para>
+       <indexterm><primary>BgWorkerStart_ConsistentState</primary></indexterm>
+       Start as soon as a consistent state has been reached in a hot-standby,
+       allowing processes to connect to databases and run read-only queries.
+      </para>
+     </listitem>
+    </varlistentry>
+
+    <varlistentry>
+     <term><literal>BgWorkerStart_RecoveryFinished</literal></term>
+     <listitem>
+      <para>
+       <indexterm><primary>BgWorkerStart_RecoveryFinished</primary></indexterm>
+       Start as soon as the system has entered normal read-write state. Note
+       that the <literal>BgWorkerStart_ConsistentState</literal> and
+      <literal>BgWorkerStart_RecoveryFinished</literal> are equivalent
+       in a server that's not a hot standby.
+      </para>
+     </listitem>
+    </varlistentry>
+
+    <varlistentry>
+     <term><literal>BgWorkerStart_ConsistentState_HotStandby</literal></term>
+     <listitem>
+      <para>
+       <indexterm><primary>BgWorkerStart_ConsistentState_HotStandby</primary></indexterm>
+       Same meaning as <literal>BgWorkerStart_ConsistentState</literal> but
+       it is more strict in terms of the server i.e. start the worker only
+       if it is hot-standby.
+      </para>
+     </listitem>
+    </varlistentry>
+   </variablelist>

Maybe reorder these slightly, because I felt it is better if the
BgWorkerStart_ConsistentState_HotStandby comes next after
BgWorkerStart_ConsistentState, which it refers to

For example::
1st.BgWorkerStart_PostmasterStart
2nd.BgWorkerStart_ConsistentState
3rd.BgWorkerStart_ConsistentState_HotStandby
4th.BgWorkerStart_RecoveryFinished

======
doc/src/sgml/config.sgml

2.
<varname>enable_syncslot</varname> = true

Not sure, but I thought the "= true" part should be formatted too.

SUGGESTION
<literal>enable_syncslot = true</literal>

======

3.
+    <para>
+     A logical replication slot on the primary can be synchronized to the hot
+     standby by enabling the failover option during slot creation and setting
+     <varname>enable_syncslot</varname> on the standby. For the synchronization
+     to work, it is mandatory to have a physical replication slot between the
+     primary and the standby. It's highly recommended that the said physical
+     replication slot is listed in <varname>standby_slot_names</varname> on
+     the primary to prevent the subscriber from consuming changes faster than
+     the hot standby. Additionally, <varname>hot_standby_feedback</varname>
+     must be enabled on the standby for the slots synchronization to work.
+    </para>

I felt those parts that describe the mandatory GUCs should be kept together.

SUGGESTION
For the synchronization to work, it is mandatory to have a physical
replication slot between the primary and the standby, and
<varname>hot_standby_feedback</varname> must be enabled on the
standby.

It's also highly recommended that the said physical replication slot
is named in <varname>standby_slot_names</varname> list on the primary,
to prevent the subscriber from consuming changes faster than the hot
standby.

~~~

4. (Chapter 49)

By enabling synchronization of slots, logical replication can be
resumed after failover depending upon the
pg_replication_slots.sync_state for the synchronized slots on the
standby at the time of failover. Only slots that were in ready
sync_state ('r') on the standby before failover can be used for
logical replication after failover. However, the slots which were in
initiated sync_state ('i') and not sync-ready ('r') at the time of
failover will be dropped and logical replication for such slots can
not be resumed after failover. This applies to the case where a
logical subscription is disabled before failover and is enabled after
failover. If the synchronized slot due to disabled subscription could
not be made sync-ready ('r') on standby, then the subscription can not
be resumed after failover even when enabled. If the primary is idle,
then the synchronized slots on the standby may take a noticeable time
to reach the ready ('r') sync_state. This can be sped up by calling
the pg_log_standby_snapshot function on the primary.

~

Somehow, I still felt all that was too wordy/repetitive. Below is my
attempt to make it more concise. Thoughts?

SUGGESTION
The ability to resume logical replication after failover depends upon
the pg_replication_slots.sync_state value for the synchronized slots
on the standby at the time of failover. Only slots that have attained
a "ready" sync_state ('r') on the standby before failover can be used
for logical replication after failover. Slots that have not yet
reached 'r' state (they are still 'i') will be dropped, therefore
logical replication for those slots cannot be resumed. For example, if
the synchronized slot could not become sync-ready on standby due to a
disabled subscription, then the subscription cannot be resumed after
failover even when it is enabled.

If the primary is idle, the synchronized slots on the standby may take
a noticeable time to reach the ready ('r') sync_state. This can be
sped up by calling the pg_log_standby_snapshot function on the
primary.

======

5.
+      <para>
+      Defines slot synchronization state. This is meaningful on the physical
+      standby which has configured <varname>enable_syncslot</varname> = true
+      </para>

As mentioned in the previous review comment ([1]#10) I thought it
might be good to include a hyperlink cross-reference to the
'enable_syncslot' GUC.

~~~

6.
+      <para>
+      The hot standby can have any of these sync_state for the slots but on a
+      hot standby, the slots with state 'r' and 'i' can neither be used for
+      logical decoding nor dropped by the user. The primary server will have
+      sync_state as 'n' for all the slots. But if the standby is promoted to
+      become the new primary server, sync_state can be seen 'r' as well. On
+      this new primary server, slots with sync_state as 'r' and 'n' will
+      behave the same.
+      </para></entry>

6a.
/these sync_state for the slots/these sync_state values for the slots/

~

6b
Hm. I still felt (same as previous review [1]#12b) that there seems
too much information here.

IIUC the sync_state is only meaningful on the standby. Sure, it might
have some values line 'n' or 'r' on the primary also, but those either
mean nothing ('n') or are leftover states from a previous failover
from a standby ('r'), which also means nothing. So can't we just say
it more succinctly like that?

SUGGESTION
The sync_state has no meaning on the primary server; the primary
sync_state value is default 'n' for all slots but may (if leftover
from a promoted standby)  also be 'r'.

======

7.
 static void

Still seems to be tampering with indentation that should only be in patch 0001.

======

8. wait_for_primary_slot_catchup

The meaning of the boolean return of this function is still not
described by the function comment.

~~~

9.
+ * If passed, *wait_attempts_exceeded will be set to true only if this
+ * function exits after exhausting its wait attempts. It will be false
+ * in all the other cases like failure, remote-slot invalidation, primary
+ * could catch up.

The above already says when a return false happens, so it seems
overkill to give more information.

SUGGESTION
If passed, *wait_attempts_exceeded will be set to true only if this
function exits due to exhausting its wait attempts. It will be false
in all the other cases.

~~~

10.

+static bool
+wait_for_primary_slot_catchup(WalReceiverConn *wrconn, RemoteSlot *remote_slot,
+   bool *wait_attempts_exceeded)
+{
+#define WAIT_OUTPUT_COLUMN_COUNT 4
+#define WORKER_PRIMARY_CATCHUP_WAIT_ATTEMPTS 5
+

10a
Maybe the long constant name is too long. How about
WAIT_PRIMARY_CATCHUP_ATTEMPTS?

~~~

10b.
IMO it is better to Assert the input value of this kind of side-effect
return parameter, to give a better understanding and to prevent future
accidents.

SUGGESTION
Assert(wait_attempts_exceeded == NULL |} *wait_attempts_exceeded == false);

~~~

11. synchronize_one_slot

+ ReplicationSlot *s;
+ ReplicationSlot *slot;
+ char sync_state = '\0';

11a.
I don't think you need both 's' and 'slot' ReplicationSlot -- it looks
a bit odd. Can't you just reuse the one 'slot' variable?

~

11b.
Also, maybe those assignment like
+ slot = MyReplicationSlot;

can have an explanatory comment like:
/* For convenience, we assign MyReplicationSlot to a shorter variable name. */

~~~

12.
+static void
+synchronize_one_slot(WalReceiverConn *wrconn, RemoteSlot *remote_slot,
+ bool *slot_updated)
+{
+ ReplicationSlot *s;
+ ReplicationSlot *slot;
+ char sync_state = '\0';

In my previous review [1]#33a I thought it was strange to assign the
sync_state (which is essentially an enum) to some meaningless value,
so I suggested it should be set to SYNCSLOT_STATE_NONE in the
declaration. The reply [2] was "No, that will change the flow. It
should stay uninitialized if the slot is not found."

But I am not convinced there is any flow problem. Also,
SYNCSLOT_STATE_NONE seems the naturally correct default for something
with no state. It cannot be found and be SYNCSLOT_STATE_NONE at the
same time (that is reported as an ERROR "skipping sync of slot") so I
see no problem.

The CURRENT code is like this:

/* Slot created by the slot sync worker exists, sync it */
if (sync_state)
{
  Assert(sync_state == SYNCSLOT_STATE_READY || sync_state ==
SYNCSLOT_STATE_INITIATED);
  ...
}
/* Otherwise create the slot first. */
else
{
  ...
}

AFAICT that could easily be changed to like below, with no change to
the logic, and it avoids setting strange values.

SUGGESTION.

if (sync_state == SYNCSLOT_STATE_NONE)
{
  /* Slot not found. Create it. */
  ..
}
else
{
  Assert(sync_state == SYNCSLOT_STATE_READY || sync_state ==
SYNCSLOT_STATE_INITIATED);
  ...
}

~~~

13. synchronize_one_slot

+static void

This *slot_updated parameter looks dubious. It is used in a loop from
the caller to mean that ANY slot was updated -- e.g. maybe it is true
or false on entry to this function.

But, Instead of having some dependency between this function and the
caller, IMO it makes more sense if we would make this just a boolean
function in the first place (e.g. was updated? T/F)

Then the caller can also be written more easily like:

some_slot_updated |= synchronize_one_slot(wrconn, remote_slot);

~~~

14.

Extra curly brackets around the ereport are not needed.

~~~

15.
+ /*
+ * Sanity check: With hot_standby_feedback enabled and
+ * invalidations handled apropriately as above, this should never
+ * happen.
+ */
+ if (remote_slot->restart_lsn < slot->data.restart_lsn)
+ {
+ elog(ERROR,
+ "not synchronizing local slot \"%s\" LSN(%X/%X)"
+ " to remote slot's LSN(%X/%X) as synchronization "
+ " would move it backwards", remote_slot->name,
+ LSN_FORMAT_ARGS(slot->data.restart_lsn),
+ LSN_FORMAT_ARGS(remote_slot->restart_lsn));
+ }

15a.
/apropriately/appropriately/

~

15b.
Extra curly brackets around the elog are not needed.

~~~

16. synchronize_slots

+static bool
+synchronize_slots(WalReceiverConn *wrconn)
+{
+#define SLOTSYNC_COLUMN_COUNT 9
+ Oid slotRow[SLOTSYNC_COLUMN_COUNT] = {TEXTOID, TEXTOID, LSNOID,
+ LSNOID, XIDOID, BOOLOID, BOOLOID, TEXTOID, INT2OID};
+
+ WalRcvExecResult *res;
+ TupleTableSlot *tupslot;
+ StringInfoData s;
+ List    *remote_slot_list = NIL;
+ MemoryContext oldctx = CurrentMemoryContext;
+ ListCell   *lc;
+ bool slot_updated = false;

Suggest renaming 'slot_updated' to 'some_slot_updated' or
'update_occurred' etc because the current name makes it look like it
applies to a single slot, but it doesn't.

~~~

17.
+ return slot_updated;

IMO "return false;" here is more clear than saying "return slot_updated;"

~~~

18.
+ appendStringInfo(&s,

18a.
/and/AND/

~

18b.
In the reply post (see [2]#32) Shveta said "I could not find quote_*
function for a character just like we have 'quote_literal_cstr' for
string". If you still want to use constant substitution instead of
just hardwired 'i' then why do even you need a quote_* function? I
thought the appendStringInfo uses a printf style format-string
internally, so I assumed it is possible to substitute the state char
directly using '%c'.

~~~

19.
+
+
+
+ /* We are done, free remote_slot_list elements */
+ list_free_deep(remote_slot_list);
+
+ walrcv_clear_result(res);
+
+ return slot_updated;
+}

Excessive blank lines.

~~~

20. validate_primary_slot

+ appendStringInfo(&cmd,
+ "SELECT count(*) = 1 from pg_replication_slots "
+ "WHERE slot_type='physical' and slot_name=%s",
+ quote_literal_cstr(PrimarySlotName));

/and/AND/

~~~

21.
+ slot = MakeSingleTupleTableSlot(res->tupledesc, &TTSOpsMinimalTuple);
+ tuple_ok = tuplestore_gettupleslot(res->tuplestore, true, false, slot);
+ Assert(tuple_ok); /* It must return one tuple */

IMO it's better to use all the var names the same across all
functions? So call this 'tupslot' like the other
MakeSingleTupleTableSlot result.

~~~

22. validate_slotsync_parameters

+/*
+ * Checks if GUCs are set appropriately before starting slot sync worker
+ *
+ * The slot sync worker can not start if 'enable_syncslot' is off and
+ * since 'enable_syncslot' is ON, check that the other GUC settings
+ * (primary_slot_name, hot_standby_feedback, wal_level, primary_conninfo)
+ * are compatible with slot synchronization. If not, raise ERROR.
+ */
+static void
+validate_slotsync_parameters(char **dbname)
+{

22a.
The comment is quite verbose. IMO the 2nd para seems just unnecessary
detail of the 1st para.

SUGGESTION
Check that all necessary GUCs for slot synchronization are set
appropriately. If not, raise an ERROR.

~~~

22b.
IMO (and given what was said in the comment about enable_syncslot must
be on)the first statement of this function should be:

/* Sanity check. */
Assert(enable_syncslot);

~~~

23. slotsync_reread_config

+ old_dbname = walrcv_get_dbname_from_conninfo(PrimaryConnInfo);
+ Assert(old_dbname);

(This is same comment as old review [1]#61)

Hmm. I still don't see why this extraction of the dbname cannot be
deferred until later when you know the PrimaryConnInfo has changed,
otherwise, it might be redundant to do this. Shveta replied [2] that
"Once PrimaryConnInfo is changed, we can not get old-dbname.", but I'm
not so sure. Isn't this walrcv_get_dbname_from_conninfo just doing a
string search -- Why can't you defer this until you know
conninfoChanged is true, and then to get the old_dbname, you can just
pass the old_primary_conninfo. E.g. call like
walrcv_get_dbname_from_conninfo(old_primary_conninfo); Maybe I am
mistaken.

~~

24.
+ /*
+ * Since we have initialized this worker with the old dbname, thus
+ * exit if dbname changed. Let it get restarted and connect to the new
+ * dbname specified.
+ */
+ if (conninfoChanged && strcmp(old_dbname, new_dbname) != 0)
+ ereport(ERROR,
+ errmsg("exiting slot sync worker as dbname in "
+    "primary_conninfo changed"));

IIUC when the tablesync has to restart, it emits a LOG message before
it exits; but it's not an ERROR. So, shouldn't this be similar -- IMO
it is not an "error" for the user to wish to change the dbname. Maybe
this should be LOG followed by an explicit exit. If you agree, then it
might be better to encapsulate such logic in some little function:

// pseudo-code
void slotsync_worker_restart(const char *msg)
{
ereport(LOG, msg...
exit(0);
}

~~~

25. ReplSlotSyncWorkerMain

+ for (;;)
+ {
+ int rc;
+ long naptime = WORKER_DEFAULT_NAPTIME_MS;
+ TimestampTz now;
+ bool slot_updated;
+
+ ProcessSlotSyncInterrupts(wrconn);
+
+ slot_updated = synchronize_slots(wrconn);

Here I think the 'slot_updated' should be renamed to the same name as
in #16 above (e.g. 'some_slot_updated' or 'any_slot_updated' or
'update_occurred' etc).

~~~

26. SlotSyncWorkerRegister

+ if (!enable_syncslot)
+ {
+ ereport(LOG,
+ errmsg("skipping slots synchronization because enable_syncslot is "
+    "disabled."));
+ return;
+ }

Instead of saying "because..." in the error message maybe keep the
message more terse and describe the "because" part in the errdetail

SUGGESTION
errmsg("skipping slot synchronization")
errdetail("enable_syncslot is disabled.")

======

27.
+ * sync_state: Defines slot synchronization state. For user created slots, it
+ * is SYNCSLOT_STATE_NONE and for the slots being synchronized on the physical
+ * standby, it is either SYNCSLOT_STATE_INITIATED or SYNCSLOT_STATE_READY
  */
 void
 ReplicationSlotCreate(const char *name, bool db_specific,
    ReplicationSlotPersistency persistency,
-   bool two_phase, bool failover)
+   bool two_phase, bool failover, char sync_state)

27a.
Why is this comment even mentioning SYNCSLOT_STATE_READY? IIUC it
doesn't make sense to ever call ReplicationSlotCreate directly setting
the 'r' state (e.g., bypassing 'i' ???)

~

27b.
Indeed, IMO there should be Assert(sync_state == SYNCSLOT_STATE_NONE
|| syncstate == SYNCSLOT_STATE_INITIATED); to guarantee this.

======

28.
+ /*
+ * Is this a slot created by a sync-slot worker?
+ *
+ * Only relevant for logical slots on the physical standby.
+ */
+ char sync_state;
+

(probably I am repeating a previous thought here)

The comment says the field is only relevant for standby, and that's
how I've been visualizing it, and why I had previously suggested even
renaming it to 'standby_sync_state'. However, replies are saying that
after failover these sync_states also have "some meaning for the
primary server".

That's the part I have trouble understanding. IIUC the server states
are just either all 'n' (means nothing) or 'r' because they are just
leftover from the old standby state. So, does it *truly* have meaning
for the server? Or should those states somehow be removed/ignored on
the new primary? Anyway, the point is that if this field does have
meaning also on the primary (I doubt) then those details should be in
this comment. Otherwise "Only relevant ... on the standby" is too
misleading.

======
.../t/050_standby_failover_slots_sync.pl

29.
+# Create table and publication on primary
+$primary->safe_psql('postgres', "CREATE TABLE tab_mypub3 (a int
PRIMARY KEY);");
+$primary->safe_psql('postgres', "CREATE PUBLICATION mypub3 FOR TABLE
tab_mypub3;");
+

29a.

~

29b.
Consider to combine those DDL

~

29c.
Perhaps for consistency, you should be calling this 'regress_mypub3'.

~~~

30.
+# Create a subscriber node
+my $subscriber3 = PostgreSQL::Test::Cluster->new('subscriber3');
+$subscriber3->init(allows_streaming => 'logical');
+$subscriber3->start;
+$subscriber3->safe_psql('postgres', "CREATE TABLE tab_mypub3 (a int
PRIMARY KEY);");
+
+# Create a subscription with failover = true & wait for sync to complete.
+$subscriber3->safe_psql('postgres',
+ "CREATE SUBSCRIPTION mysub3 CONNECTION '$publisher_connstr' "
+   . "PUBLICATION mypub3 WITH (slot_name = lsub3_slot, failover = true);");
+$subscriber3->wait_for_subscription_sync;

30a
Consider combining those DDLs.

~

30b.
Probably for consistency, you should be calling this 'regress_mysub3'.

~~~

31.
+# Advance lsn on the primary
+$primary->safe_psql('postgres',
+ "SELECT pg_log_standby_snapshot();");
+$primary->safe_psql('postgres',
+ "SELECT pg_log_standby_snapshot();");
+$primary->safe_psql('postgres',
+ "SELECT pg_log_standby_snapshot();");
+

Consider combining all those DDLs.

~~~

32.
+# Truncate table on primary
+$primary->safe_psql('postgres',
+ "TRUNCATE TABLE tab_mypub3;");
+
+# Insert data on the primary
+$primary->safe_psql('postgres',
+ "INSERT INTO tab_mypub3 SELECT generate_series(1, 10);");
+

Consider combining those DDLs.

~~~

33.
+# Confirm that restart_lsn of lsub3_slot slot is synced to the standby
+$result = $standby3->safe_psql('postgres',
+ qq[SELECT '$primary_lsn' >= restart_lsn from pg_replication_slots
WHERE slot_name = 'lsub3_slot';]);
+is($result, 't', 'restart_lsn of slot lsub3_slot synced to standby');

Does "'$primary_lsn' >= restart_lsn" make sense here? NOTE, the sign
was '<=' in v43-0002

~~~

34.
+# Confirm that confirmed_flush_lsn of lsub3_slot slot is synced to the standby
+$result = $standby3->safe_psql('postgres',
+ qq[SELECT '$primary_lsn' >= confirmed_flush_lsn from
pg_replication_slots WHERE slot_name = 'lsub3_slot';]);
+is($result, 't', 'confirmed_flush_lsn of slot lsub3_slot synced to
the standby');

Does "'$primary_lsn' >= confirmed_flush_lsn" make sense here? NOTE,
the sign was '<=' in v43-0002

~~~

35.
+##################################################
+# Test that synchronized slot can neither be docoded nor dropped by the user
+##################################################

35a.
/docoded/decoded/

~

35b.
Please give explanation in the comment *why* those ops are not allowed
(e.g. because the hot_standby_feedback GUC does not have an accepted
value)

~~~

36.
+##################################################
+# Create another slot which stays in sync_state as initiated ('i')
+##################################################
+

Please explain the comment as to *why* it gets stuck in the initiated state.

~~~

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

/'logical_slot'is/'logical_slot' is/ (missing space)

/succesfully/successfully/

~~~

38.
+# Update subscription with new primary's connection info
+$subscriber3->safe_psql('postgres', "ALTER SUBSCRIPTION mysub3 DISABLE;");
+$subscriber3->safe_psql('postgres', "ALTER SUBSCRIPTION mysub3
CONNECTION '$standby3_conninfo';");
+$subscriber3->safe_psql('postgres', "ALTER SUBSCRIPTION mysub3 ENABLE;");

Consider combining all those DDLs.

~~~

39.
+
+# Insert data on the new primary
+$standby3->safe_psql('postgres',
+ "INSERT INTO tab_mypub3 SELECT generate_series(11, 20);");
+
+# Confirm that data in tab_mypub3 replicated on subscriber
+is( $subscriber3->safe_psql('postgres', q{SELECT count(*) FROM tab_mypub3;}),
+ "20",
+ 'data replicated from new primary');

Shouldn't there be some wait_for_subscription_sync logic (or similar)
here just to ensure the subscriber3 had time to receive that data
before you immediately check that it had arrived?

======
[1] My v43-0002 review.
https://www.postgresql.org/message-id/CAHut%2BPuuqEpDse5msENsVuK3rjTRN-QGS67rRCGVv%2BzcT-f0GA%40mail.gmail.com
[2] Replies to v43-0002 review.
https://www.postgresql.org/message-id/CAJpy0uDcOf5Hvk_CdCCAbfx9SY%2Bog%3D%3D%3DtgiuhWKzkYyqebui9g%40mail.gmail.com

Kind Regards,
Peter Smith.
Fujitsu Australia

---

From: shveta malik <[email-masked]>
Date: Wed, 13 Dec 2023 17:07:40 +0530

> 10sec. Shall it be say 5min in this case?

PFA v47 attached, changes are:

patch 001:
1) Addressed comment in [1]. Thanks Hou-san for this change.

patch 002
2) Slot sync worker will be no-op if it is on cascading standby as
suggested in [2]
3) StartTransaction related optimization as suggested in [3]
4) Few other comments' improvement and code-cleanup.

TODO:
--Few pending comments as I stated in [4] (mainly header inclusion in
tablesync.c, and 'r' to 'n' conversion on promotion)
--The comments given today in [5]

[1]: https://www.postgresql.org/message-id/CABdArM4Cow6aOLjGG9qnp6mhg%2B%2BgjK%3DHDO%3DKSU%3D6%3DyT7hLkknQ%40mail.gmail.com
[2]: https://www.postgresql.org/message-id/CAA4eK1Ki1O65SyA6ijh-Mq4zpzeh644fCmkrZXMJcQXHNrAw0Q%40mail.gmail.com
[3]: https://www.postgresql.org/message-id/CAA4eK1L3DiKL_Wq-VdU%2B9wmjmO5%2Bfrf%3DZHK9Lzq-7zOezPP%2BWg%40mail.gmail.com
[4]: https://www.postgresql.org/message-id/CAJpy0uDcOf5Hvk_CdCCAbfx9SY%2Bog%3D%3D%3DtgiuhWKzkYyqebui9g%40mail.gmail.com
[5]: https://www.postgresql.org/message-id/CAHut%2BPtOc7J_n24HJ6f_dFWTuD3X2ApOByQzZf6jZz%2B0wb-ebQ%40mail.gmail.com

thanks
Shveta

---

From: Peter Smith <[email-masked]>
Date: Thu, 14 Dec 2023 12:30:04 +1100

Hi, here are a few more review comments for the patch v47-0002

(plus my review comments of v45-0002 [1] are yet to be addressed)

======
1. General

For consistency and readability, try to use variables of the same
names whenever they have the same purpose, even when they declared are
in different functions. A few like this were already mentioned in the
previous review but there are more I keep noticing.

For example,
'slotnameChanged' in function, VERSUS 'primary_slot_changed' in the caller.

======

2.
+/*
+ *
+ * Validates the primary server info.
+ *
+ * Using the specified primary server connection, it verifies whether
the master
+ * is a standby itself and returns true in that case to convey the caller that
+ * we are on the cascading standby.
+ * But if master is the primary server, it goes ahead and validates
+ * primary_slot_name. It emits error if the physical slot in primary_slot_name
+ * does not exist on the primary server.
+ */
+static bool
+validate_primary_info(WalReceiverConn *wrconn)

2a.
Extra line top of that comment?

~

2b.
IMO it is too tricky to have a function called "validate_xxx", when
actually you gave that return value some special unintuitive meaning
other than just validation. IMO it is always better for the returned
value to properly match the function name so the expectations are very
obvious. So, In this case, I think a better function signature would
be like this:

SUGGESTION

static void
validate_primary_info(WalReceiverConn *wrconn, bool *master_is_standby)

or

static void
validate_primary_info(WalReceiverConn *wrconn, bool *am_cascading_standby)

~~~

3.
+ if (res->status != WALRCV_OK_TUPLES)
+ ereport(ERROR,
+ (errmsg("could not fetch recovery and primary_slot_name \"%s\" info from the "
+ "primary: %s", PrimarySlotName, res->err)));

I'm not sure that including "recovery and" in the error message is
meaningful to the user, is it?

~~~

4. slotsync_reread_config

+/*
+ * Re-read the config file.
+ *
+ * If any of the slot sync GUCs have changed, re-validate them. The
+ * worker will exit if the check fails.
+ *
+ * Returns TRUE if primary_slot_name is changed, let the caller re-verify it.
+ */
+static bool
+slotsync_reread_config(WalReceiverConn *wrconn)

Hm. This is another function where the return value has been butchered
to have a special meaning unrelated the the function name. IMO it
makes the code unnecessarily confusing.

IMO a better function signature here would be:

static void
slotsync_reread_config(WalReceiverConn *wrconn, bool *primary_slot_name_changed)

~~~

5. ProcessSlotSyncInterrupts

+/*
+ * Interrupt handler for main loop of slot sync worker.
+ */
+static bool
+ProcessSlotSyncInterrupts(WalReceiverConn *wrconn, bool
check_cascading_standby)
+{

There is no function comment describing the meaning of the return
value. But actually, IMO this is an example of how conflating the
meanings of validation VERSUS are_we_cascading_standby in the
lower-down function has propagated up to become a big muddle.

The code
+ if (primary_slot_changed || check_cascading_standby)
+ return validate_primary_info(wrconn);

seems unnecessarily hard to understand because,
false -- doesn't mean invalid
true -- doesn't mean valid

Please, consider changing this signature also so the functions return
what you would intuitively expect them to return without surprisingly
different meanings.

SUGGESTION

static void
ProcessSlotSyncInterrupts(WalReceiverConn *wrconn, bool
check_cascading_standby, bool *am_cascading_standby)

~~~

6. ReplSlotSyncWorkerMain

+ int rc;
+ long naptime = WORKER_DEFAULT_NAPTIME_MS;
+ TimestampTz now;
+ bool slot_updated;
+
+ /*
+ * The transaction env is needed by walrcv_exec() in both the slot
+ * sync and primary info validation flow.
+ */
+ StartTransactionCommand();
+
+ if (!am_cascading_standby)
+ {
+ slot_updated = synchronize_slots(wrconn);
+
+ /*
+ * If any of the slots get updated in this sync-cycle, use default
+ * naptime and update 'last_update_time'. But if no activity is
+ * observed in this sync-cycle, then increase naptime provided
+ * inactivity time reaches threshold.
+ */
+ now = GetCurrentTimestamp();
+ if (slot_updated)
+ last_update_time = now;
+ else if (TimestampDifferenceExceeds(last_update_time,
+ now, WORKER_INACTIVITY_THRESHOLD_MS))
+ naptime = WORKER_INACTIVITY_NAPTIME_MS;
+ }
+ else
+ naptime = 6 * WORKER_INACTIVITY_NAPTIME_MS; /* 60 sec */
+
+ rc = WaitLatch(MyLatch,
+    WL_LATCH_SET | WL_TIMEOUT | WL_EXIT_ON_PM_DEATH,
+    naptime,
+    WAIT_EVENT_REPL_SLOTSYNC_MAIN);
+
+ if (rc & WL_LATCH_SET)
+ ResetLatch(MyLatch);
+
+ am_cascading_standby =
+ ProcessSlotSyncInterrupts(wrconn, am_cascading_standby);
+
+ CommitTransactionCommand();

IMO it is more natural to avoid negative conditions, so just reverse
these. Also, some comment is needed to explain why the longer naptime
is needed in this special case.

SUGGESTION
if (am_cascading_standby)
{
  /* comment the reason .... */
  naptime = 6 * WORKER_INACTIVITY_NAPTIME_MS; /* 60 sec */
}
else
{
  /* Normal standby */
  ...
}

======
[1] review of v45-0002.
https://www.postgresql.org/message-id/CAHut%2BPtOc7J_n24HJ6f_dFWTuD3X2ApOByQzZf6jZz%2B0wb-ebQ%40mail.gmail.com

Kind Regards,
Peter Smith.
Fujitsu Australia

---

From: Peter Smith <[email-masked]>
Date: Thu, 14 Dec 2023 15:45:21 +1100

A review comment for v47-0001

======

1.  GetStandbySlotList

+static void
+WalSndRereadConfigAndReInitSlotList(List **standby_slots)
+{
+ char    *pre_standby_slot_names;
+
+ ProcessConfigFile(PGC_SIGHUP);
+
+ /*
+ * If we are running on a standby, there is no need to reload
+ * standby_slot_names since we do not support syncing slots to cascading
+ * standbys.
+ */
+ if (RecoveryInProgress())
+ return;

Should the RecoveryInProgress() check be first -- even before the
ProcessConfigFile call?

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

---

From: Amit Kapila <[email-masked]>
Date: Thu, 14 Dec 2023 11:08:22 +0530

> SYNCSLOT_STATE_INITIATED);

I think instead of creating syncslot based on syncstate, it would be
better to create it when we don't find it via
SearchNamedReplicationSlot(). That will avoid the need to initialize
the syncstate and I think it would make code in this area look better.

+1.

> walrcv_get_dbname_from_conninfo(old_primary_conninfo); Maybe I am

I think we should just restart if any one of the information is
changed with a message like: "slotsync worker will restart because of
a parameter change". This would be similar to what we do apply worker
in maybe_reread_subscription().

> this comment. Otherwise "Only relevant ... on the standby" is too

I think this deserves more comments.

-- 
With Regards,
Amit Kapila.

---

From: "Zhijie Hou (Fujitsu)" <[email-masked]>
Date: Thu, 14 Dec 2023 05:53:02 +0000

> A review comment for v47-0001

Thanks for the comment.

> +WalSndRereadConfigAndReInitSlotList(List **standby_slots) {
> + * standby_slot_names since we do not support syncing slots to
> + cascading
> + * standbys.
> + */

ProcessConfigFile is necessary here, it is used not only for standby_slot_names
but also all other GUCs that could be used in the caller.

Best Regards,
Hou zj

---

From: Amit Kapila <[email-masked]>
Date: Thu, 14 Dec 2023 16:40:34 +0530

> validate_primary_info(WalReceiverConn *wrconn, bool *am_cascading_standby)

The terminology master_is_standby is a bit indirect for this usage, so
I would prefer the second one. Shall we name this function as
check_primary_info()? Additionally, can we rewrite the following
comment: "Using the specified primary server connection, check whether
we are cascading standby. It also validates primary_slot_info for
non-cascading-standbys.".

-- 
With Regards,
Amit Kapila.

---

From: shveta malik <[email-masked]>
Date: Fri, 15 Dec 2023 11:02:23 +0530

> non-cascading-standbys.".

PFA v48. Changes are:

1) Addressed comments by Peter for v45-002 and v47-002 given in [1]
and [2] respectively
2) Addressed comments by Amit for v47-002 given in [3], [4]
3) Addressed an old comment (#74 in [5]) of getting rid of header
inclusion from tablesync.c when there was no code change in that file.
Thanks Hou-san for working on this change.

TODO:
--Address the test comments in [1] for 050_standby_failover_slots_sync.pl
--Review the feasibility of addressing one pending comment (comment 13
in [5]) of 'r'->'n' conversion.

[1]: https://www.postgresql.org/message-id/CAHut%2BPtOc7J_n24HJ6f_dFWTuD3X2ApOByQzZf6jZz%2B0wb-ebQ%40mail.gmail.com
[2]: https://www.postgresql.org/message-id/CAHut%2BPsvxs-%3Dj3aCpPVs3e4w78HndCdO-F4bLPzAX70%2BdgWUuQ%40mail.gmail.com
[3]: https://www.postgresql.org/message-id/CAA4eK1L2ts%3DgfiF4aw7-DH8HWj29s08hVRq-Ff8%3DmjfdUXx8CA%40mail.gmail.com
[4]: https://www.postgresql.org/message-id/CAA4eK1%2Bw9yv%2B4UZXhiDHZpGDfbeRHYDBu23FwsniS8sYUZeu1w%40mail.gmail.com
[5]: https://www.postgresql.org/message-id/CAJpy0uDcOf5Hvk_CdCCAbfx9SY%2Bog%3D%3D%3DtgiuhWKzkYyqebui9g%40mail.gmail.com

thanks
Shveta

---

From: shveta malik <[email-masked]>
Date: Fri, 15 Dec 2023 11:03:33 +0530

> PFA v48. Changes are:
Sorry, I missed attaching the patch. PFA v48.

---

From: shveta malik <[email-masked]>
Date: Fri, 15 Dec 2023 11:09:12 +0530

> Hi Shveta, here are some review comments for v45-0002.

Thanks for the feedback. Addressed these in v48. Please find my
comments on some.

> /* For convenience, we assign MyReplicationSlot to a shorter variable name. */

I have changed it to slightly simpler one, if that is okay?

> SYNCSLOT_STATE_INITIATED);

I have restructured the entire code here and thus initialization of
sync_state is no longer needed. Please review now and let me know.

> internally, so I assumed it is possible to substitute the state char

Since we have removed cascading standby support, this condition
(sync_state != 'i') is no longer needed in the query.

> walrcv_get_dbname_from_conninfo(old_primary_conninfo); Maybe I am

Sorry missed your point earlier that we can use old_primary_conninfo
to extract dbname later.
I have removed this re-validation now as we will restart the worker in
case of a parameter change similar to the case of logical apply
worker. So these changes are no longer needed.

> void slotsync_worker_restart(const char *msg)

we can not do proc_exit(0), as then postmaster will not restart it on
clean-exit. I agree with your logic, but will have another argument in
this function to accept 'exit code' from the caller.

> this comment. Otherwise "Only relevant ... on the standby" is too

I have modified it currently, but I will give another thought on your
suggestions here (and in earlier emails) and will let you know.

> .../t/050_standby_failover_slots_sync.pl

We are working on CFbot failure fixes in this file and restructing the
tests here. Thus I am keeping these test comments on hold and will
address in next version.

---

From: shveta malik <[email-masked]>
Date: Fri, 15 Dec 2023 11:12:53 +0530

> A review comment for v47-0001

Thanks for reviewing. I have addressed these in v48. There is some
design change around the code part where we were checking cascading
and were revalidating new GUC values on conf-reload. So code has
changed entirely around that part where some of these comments were.
Please review now and let me know.

---

From: Nisha Moond <[email-masked]>
Date: Fri, 15 Dec 2023 17:55:09 +0530

Review for v47 patch -

(1)
When we try to create a subscription on standby using a synced slot
that is in 'r' sync_state, the subscription will be created at the
subscriber, and on standby, two actions will take place -
    (i)  As copy_data is true by default, it will switch the failover
state of the synced-slot to 'false'.
    (ii) As we don't allow to use synced-slots, it will start giving
the expected error in the log file -
        ERROR:  cannot use replication slot "logical_slot" for logical decoding
        DETAIL:  This slot is being synced from the primary server.
        HINT:  Specify another replication slot.

The first one seems an issue,  it toggles the failover to false and
then it remains false after that. I think it should be fixed.

(2)
With the patch, the 'CREATE SUBSCRIPTION' command with a 'slot_name'
of an 'active' logical slot fails and errors out -
    ERROR:  could not alter replication slot "logical_slot" on
publisher: ERROR:  replication slot "logical_slot1" is active for PID
xxxx

Without the patch, the create subscription with an 'active' slot_name
succeeds and the log file shows the error "could not start WAL
streaming: ERROR:  replication slot "logical_slot" is active for PID
xxxx".

Given that the specified active slot_name has failover set to false
and the create subscription command also specifies failover=false, the
expected behavior of the "with-patch" case is anticipated to be the
same as that of the "without-patch" scenario.

---

From: Amit Kapila <[email-masked]>
Date: Sun, 17 Dec 2023 08:21:40 +0530

> then it remains false after that. I think it should be fixed.

+1. If we don't allow the slot to be used, we shouldn't allow its
state to be changed as well.

> same as that of the "without-patch" scenario.

Currently, we first acquire the slot to change its state but I guess
if we want the behavior as you mentioned we first need to check the
slot's 'failover' state without acquiring the slot. I am not sure if
that is any better because anyway we are going to fail in the very
next step as the slot is busy.

-- 
With Regards,
Amit Kapila.

---

From: Amit Kapila <[email-masked]>
Date: Mon, 18 Dec 2023 16:22:28 +0530

> Sorry, I missed attaching the patch. PFA v48.

Few comments on v48_0002
========================
1.
+static void
+slotsync_reread_config(WalReceiverConn *wrconn)
{
...
+ pfree(old_primary_conninfo);
+ pfree(old_primary_slotname);
+
+ if (restart)
+ {
+ char    *msg = "slot sync worker will restart because of a parameter change";
+
+ /*
+ * The exit code 1 will make postmaster restart the slot sync worker.
+ */
+ slotsync_worker_exit(msg, 1 /* proc_exit code */ );
+ }
...

I don't see the need to explicitly pfree in case we are already
exiting the process because anyway the memory will be released. We can
avoid using the 'restart' variable for this. Also, probably, directly
exiting here makes sense and at another place where this function is
used. I see that in maybe_reread_subscription(), we exit with a 0 code
and still apply worker restarts, so why use a different exit code
here?

2.
+static void
+check_primary_info(WalReceiverConn *wrconn, bool *am_cascading_standby)
{
...
+ remote_in_recovery = DatumGetBool(slot_getattr(tupslot, 1, &isnull));
+ Assert(!isnull);
+
+ /* No need to check further, return that we are cascading standby */
+ if (remote_in_recovery)
+ {
+ *am_cascading_standby = true;
+ CommitTransactionCommand();
+ return;
...
}

Don't we need to clear the result and tuple in case of early return?

3. It would be a good idea to mention about requirements like a
physical slot on primary, hot_standby_feedback, etc. in the commit
message.

4.
+static bool
+wait_for_primary_slot_catchup(WalReceiverConn *wrconn, RemoteSlot *remote_slot,
+   bool *wait_attempts_exceeded)
{
...
+ tupslot = MakeSingleTupleTableSlot(res->tupledesc, &TTSOpsMinimalTuple);
+ if (!tuplestore_gettupleslot(res->tuplestore, true, false, tupslot))
+ {
+ ereport(WARNING,
+ (errmsg("slot \"%s\" creation aborted", remote_slot->name),
+ errdetail("This slot was not found on the primary server")));
...
+ /*
+ * It is possible to get null values for LSN and Xmin if slot is
+ * invalidated on the primary server, so handle accordingly.
+ */
+ new_invalidated = DatumGetBool(slot_getattr(tupslot, 1, &isnull));
+ Assert(!isnull);
+
+ new_restart_lsn = DatumGetLSN(slot_getattr(tupslot, 2, &isnull));
+ if (new_invalidated || isnull)
+ {
+ ereport(WARNING,
+ (errmsg("slot \"%s\" creation aborted", remote_slot->name),
+ errdetail("This slot was invalidated on the primary server")));
...
}

a. The errdetail message should end with a full stop. Please check all
other errdetail messages in the patch to follow the same guideline.
b. I think saying slot creation aborted is not completely correct
because we would have created the slot especially when it is in 'i'
state. Can we change it to something like: "aborting initial sync for
slot \"%s\""?
c. Also, if the remote_slot is invalidated, ideally, we can even drop
the local slot but it seems that the patch will drop the same before
the next-sync cycle with any other slot that needs to be dropped. If
so, can we add the comment to indicate the same?

5.
+static void
+local_slot_update(RemoteSlot *remote_slot)
+{
+ Assert(MyReplicationSlot->data.invalidated == RS_INVAL_NONE);
+
+ LogicalConfirmReceivedLocation(remote_slot->confirmed_lsn);
+ LogicalIncreaseXminForSlot(remote_slot->confirmed_lsn,
+    remote_slot->catalog_xmin);
+ LogicalIncreaseRestartDecodingForSlot(remote_slot->confirmed_lsn,
+   remote_slot->restart_lsn);
+
+ SpinLockAcquire(&MyReplicationSlot->mutex);
+ MyReplicationSlot->data.invalidated = remote_slot->invalidated;
+ SpinLockRelease(&MyReplicationSlot->mutex);
...
...

If required, the invalidated flag is updated in the caller as well, so
why do we need to update it here as well?

-- 
With Regards,
Amit Kapila.

---

From: "Zhijie Hou (Fujitsu)" <[email-masked]>
Date: Mon, 18 Dec 2023 12:11:22 +0000

> too much disk activity during promotion. so let me analyze and come

Per off-list discussion with Amit.

I think it's fine to keep both READY and NONE on a primary. Because even if we
update the sync_state from READY to NONE on promotion, it doesn't reduce the
complexity for the handling of READY and NONE state. And it's not
straightforward to choose the right place to update sync_state, here is the
analysis:

(related steps on promotion)
1 (patch) shutdown slotsync worker
2 (patch) drop 'i' state slots.
3 remove standby.signal and recovery.signal
4 switch to a new timeline and write the timeline history file 
5 set SharedRecoveryState = RECOVERY_STATE_DONE which means RecoveryInProgress() will return false.

We could not update the sync_state before step 3 because if the update fails after
updating some of slots' state, then the server will be shutdown leaving some
'NONE' state slots. After restarting, the server is still a standby so the slot
sync worker will fail to sync these 'NONE' state slots.

We also could not update it after step 3 and before step 4. Because if any ERROR
when updating, then after restarting the server, although the server will
become a master(as standby.signal is removed), but it can still be made as a
active standby by creating a standby.signal file because the timeline has not
been switched. And in this case, the slot sync worker will also fail to sync
these 'NONE' state slots.

Updating the sync_state after step 4 and before step 5 is OK, but still
It doesn't simplify the handling for both READY and NONE state slots.
Therefore, I think we can retain the READY state slots after promotion as they
can provide information about the slot's origin. I added some comments around
slotsync cleanup codes (in FinishWalRecovery) to mentioned the reason.

Best Regards,
Hou zj

---

From: Peter Smith <[email-masked]>
Date: Tue, 19 Dec 2023 10:20:43 +1100

Here are some review comments for v48-0002

======
doc/src/sgml/config.sgml

1.
+          If slot synchronization is enabled then it is also necessary to
+          specify <literal>dbname</literal> in the
+          <varname>primary_conninfo</varname> string. This will only
be used for
+          slot synchronization. It is ignored for streaming.

I felt the "If slot synchronization is enabled" part should also
include an xref to the enable_slotsync GUC, otherwise there is no
information here about how to enable it.

SUGGESTION
If slot synchronization is enabled (see XXX) ....

======

2.
+    <para>
+     The ability to resume logical replication after failover depends upon the
+     <link linkend="view-pg-replication-slots">pg_replication_slots</link>.<structfield>sync_state</structfield>
+     value for the synchronized slots on the standby at the time of failover.
+     Only slots that have attained "ready" sync_state ('r') on the standby
+     before failover can be used for logical replication after failover. Slots
+     that have not yet reached 'r' state (they are still 'i') will be dropped,
+     therefore logical replication for those slots cannot be resumed. For
+     example, if the synchronized slot could not become sync-ready on standby
+     due to a disabled subscription, then the subscription cannot be resumed
+     after failover even when it is enabled.
+     If the primary is idle, then the synchronized slots on the standby may
+     take a noticeable time to reach the ready ('r') sync_state. This can
+     be sped up by calling the
+     <function>pg_log_standby_snapshot</function> function on the primary.
+    </para>

2a.
/sync-ready on standby/sync-ready on the standby/

~

2b.
Should "If the primary is idle" be in a new paragraph?

======

3.
+      <para>
+      The hot standby can have any of these sync_state values for the slots but
+      on a hot standby, the slots with state 'r' and 'i' can neither be used
+      for logical decoding nor dropped by the user.
+      The sync_state has no meaning on the primary server; the primary
+      sync_state value is default 'n' for all slots but may (if leftover
+      from a promoted standby)  also be 'r'.
+      </para></entry>

I still feel we are exposing too much useless information about the
primary server values.

Isn't it sufficient to just say "The sync_state values have no meaning
on a primary server.", and not bother to mention what those
meaningless values might be -- e.g. if they are meaningless then who
cares what they are or how they got there?

======

4. synchronize_one_slot

+ /* Slot ready for sync, so sync it. */
+ if (sync_state == SYNCSLOT_STATE_READY)
+ {
+ /*
+ * Sanity check: With hot_standby_feedback enabled and
+ * invalidations handled appropriately as above, this should never
+ * happen.
+ */
+ if (remote_slot->restart_lsn < slot->data.restart_lsn)
+ elog(ERROR,
+ "not synchronizing local slot \"%s\" LSN(%X/%X)"
+ " to remote slot's LSN(%X/%X) as synchronization "
+ " would move it backwards", remote_slot->name,
+ LSN_FORMAT_ARGS(slot->data.restart_lsn),
+ LSN_FORMAT_ARGS(remote_slot->restart_lsn));
+
+ if (remote_slot->confirmed_lsn != slot->data.confirmed_flush ||
+ remote_slot->restart_lsn != slot->data.restart_lsn ||
+ remote_slot->catalog_xmin != slot->data.catalog_xmin)
+ {
+ /* Update LSN of slot to remote slot's current position */
+ local_slot_update(remote_slot);
+ ReplicationSlotSave();
+ slot_updated = true;
+ }
+ }
+ /* Slot not ready yet, let's attempt to make it sync-ready now. */
+ else if (sync_state == SYNCSLOT_STATE_INITIATED)
+ {
+ /*
+ * Wait for the primary server to catch-up. Refer to the comment
+ * atop the file for details on this wait.
+ */
+ if (remote_slot->restart_lsn < slot->data.restart_lsn ||
+ TransactionIdPrecedes(remote_slot->catalog_xmin,
+   slot->data.catalog_xmin))
+ {
+ if (!wait_for_primary_slot_catchup(wrconn, remote_slot, NULL))
+ {
+ ReplicationSlotRelease();
+ return false;
+ }
+ }
+
+ /*
+ * Wait for primary is over, update the lsns and mark the slot as
+ * READY for further syncs.
+ */
+ local_slot_update(remote_slot);
+ SpinLockAcquire(&slot->mutex);
+ slot->data.sync_state = SYNCSLOT_STATE_READY;
+ SpinLockRelease(&slot->mutex);
+
+ /* Save the changes */
+ ReplicationSlotMarkDirty();
+ ReplicationSlotSave();
+ slot_updated = true;
+
+ ereport(LOG,
+ errmsg("newly locally created slot \"%s\" is sync-ready now",
+    remote_slot->name));
+ }

4a.
It would be more natural in the code if you do the
SYNCSLOT_STATE_INITIATED logic before the SYNCSLOT_STATE_READY because
that is the order those states come in.

~

4b.
I'm not sure if it is worth it, but I was thinking that some duplicate
code can be avoided by doing if/if instead of if/else

if (sync_state == SYNCSLOT_STATE_INITIATED)
{
..
}
if (sync_state == SYNCSLOT_STATE_READY)
{
}

By arranging it this way maybe the SYNCSLOT_STATE_INITIATED code block
doesn't need to do anything except update the sync_state =
SYNCSLOT_STATE_READY; Then it can just fall through to the
SYNCSLOT_STATE_READY logic to do all the
local_slot_update(remote_slot); etc in just one place.

~~~

5. check_primary_info

+ * Checks the primary server info.
+ *
+ * Using the specified primary server connection, check whether we
are cascading
+ * standby. It also validates primary_slot_name for non-cascading-standbys.
+ */
+static void
+check_primary_info(WalReceiverConn *wrconn, bool *am_cascading_standby)

5a.
/we are cascading/we are a cascading/

5b.
/non-cascading-standbys./non-cascading standbys./

~~~

6.
+ CommitTransactionCommand();
+
+ *am_cascading_standby = false;

Maybe it's simpler just to set this default false up-front, replacing
the current assert.

BEFORE:
+ Assert(am_cascading_standby != NULL);

AFTER:
*am_cascading_standby = false; /* maybe overwrite later */

~~~

7.
+/*
+ * Exit the slot sync worker with given exit-code.
+ */
+static void
+slotsync_worker_exit(const char *msg, int code)
+{
+ ereport(LOG, errmsg("%s", msg));
+ proc_exit(code);
+}

This could be written differently (don't pass the exit code, instead
pass a bool) like:

static void
slotsync_worker_exit(const char *msg, bool restart_worker)

By doing it this way, you can keep the special exit code values (0,1)
within this function where you can comment all about them instead of
having scattered comments about exit codes in the callers.

SUGGESTION
ereport(LOG, errmsg("%s", msg));
/* <some big comment here about how the code causes the worker to
restart or not> */
proc_exit(restart_worker ? 1 : 0);

~~~

8. slotsync_reread_config

+ if (restart)
+ {

Shouldn't that message be written as _(), so that it will get translated?

SUGGESTION
slotsync_worker_exit(_("slot sync worker will restart because of a
parameter change"), true /* restart worker */ );

~~~

9. ProcessSlotSyncInterrupts

+ CHECK_FOR_INTERRUPTS();
+
+ if (ShutdownRequestPending)
+ {
+ char    *msg = "replication slot sync worker is shutting down on
receiving SIGINT";
+
+ walrcv_disconnect(wrconn);
+
+ /*
+ * The exit code 0 means slot sync worker will not be restarted by
+ * postmaster.
+ */
+ slotsync_worker_exit(msg, 0 /* proc_exit code */ );
+ }

Shouldn't that message be written as _(), so that it will be translated?

SUGGESTION
slotsync_worker_exit(_("replication slot sync worker is shutting down
on receiving SIGINT"), false /* don't restart worker */ );

~~~

10.
+/*
+ * Cleanup function for logical replication launcher.
+ *
+ * Called on logical replication launcher exit.
+ */
+static void
+slotsync_worker_onexit(int code, Datum arg)
+{
+ SpinLockAcquire(&SlotSyncWorker->mutex);
+ SlotSyncWorker->pid = InvalidPid;
+ SpinLockRelease(&SlotSyncWorker->mutex);
+}

IMO it would make sense for this function to be defined adjacent to
the slotsync_worker_exit() function.

~~~

11. ReplSlotSyncWorkerMain

+ /*
+ * Using the specified primary server connection, check whether we are
+ * cascading standby and validates primary_slot_name for
+ * non-cascading-standbys.
+ */
+ check_primary_info(wrconn, &am_cascading_standby);
...
+ /* Recheck if it is still a cascading standby */
+ if (am_cascading_standby)
+ check_primary_info(wrconn, &am_cascading_standby);

Those 2 above calls could be combined if you want. By defaulting the
am_cascading_standby = true when declared, then you could put this
code at the top of the loop instead of having the same code in 2
places:

+ if (am_cascading_standby)
+ check_primary_info(wrconn, &am_cascading_standby);

======
src/include/commands/subscriptioncmds.h

12.
 #include "parser/parse_node.h"
+#include "replication/walreceiver.h"

There is #include, but no other code change. Is this needed?

======

13.
+ /*
+ * Synchronization state for a logical slot.
+ *
+ * The standby can have any value among the possible values of 'i','r' and
+ * 'n'. For primary, the default is 'n' for all slots but may also be 'r'
+ * if leftover from a promoted standby.
+ */
+ char sync_state;
+

All that is OK now, but I keep circling back to my original thought
that since this state has no meaning for the primary server then

a) why do we even care what potential values it might have there, and
b) isn't it better to call this field 'standby_sync_state' to
emphasize it only has meaning for the standby?

e.g.
SUGGESTION
/*
 * Synchronization state for a logical slot.
 *
 * The standby can have any value among the possible values of 'i','r' and
 * 'n'. For the primary, this field value has no meaning.
 */
char standby_sync_state;

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

---

From: Amit Kapila <[email-masked]>
Date: Tue, 19 Dec 2023 11:36:51 +0530

> cares what they are or how they got there?

I feel it would be good to mention somewhere that primary can have
slots in 'r' state, if not here, some other place.

> proc_exit(restart_worker ? 1 : 0);

Hmm, I don't see the need for this function in the first place. We can
use proc_exit in the two callers directly.

-- 
With Regards,
Amit Kapila.

---

From: shveta malik <[email-masked]>
Date: Tue, 19 Dec 2023 17:10:57 +0530

> use proc_exit in the two callers directly.

PFA v50 patch-set which addresses comments for  v48-0002 and v49-0002
given in [1], [2] and [3].

TODO:
--Fix CFBot failure.
--Work on correctness of test to merge patch003 to patch002

[1]: https://www.postgresql.org/message-id/CAA4eK1Ko-EBBDkea2R8V8PeveGg10PBswCF7JQdnRu%2BMJP%2BYBQ%40mail.gmail.com
[2]: https://www.postgresql.org/message-id/CAHut%2BPsyZQZ1A4XcKw-D%3DvcTg16pN9Dw0PzE8W_X7Yz_bv00rQ%40mail.gmail.com
[3]: https://www.postgresql.org/message-id/CAHut%2BPv86wBZiyOLHxycd8Yj9%3Dk5kzVa1x7Gbp%2B%3Dc1VGT9TG2w%40mail.gmail.com

thanks
Shveta

---

From: shveta malik <[email-masked]>
Date: Tue, 19 Dec 2023 17:17:15 +0530

> > Sorry, I missed attaching the patch. PFA v48.

Thanks for reviewing. These are addressed in v50. Please find my
comments inline for some of these.

> avoid using the 'restart' variable for this.

I have moved pfree to the end where we do not exit the worker. Removed
restart variable.

>Also, probably, directly

Logical rep worker is started by logical rep launcher and it has
different logic of restarting it. OTOH, slot-sync worker is started by
the postmaster and the postmaster starts any of its bgworkers only if
the worker had an abnormal exit and restart_time is given during
registration of the worker. Thus we need exit_code here. I have
removed the new function added though.

> Don't we need to clear the result and tuple in case of early return?

Yes, it was needed. Modified.

> so, can we add the comment to indicate the same?

I have added comments. Basically, it will be dropped in caller only if
it is 'RS_EPHEMERAL' state else if it is already persisted, then will
be maintained as is but marked as invalidated in caller and its sync
will be skipped next time onwards.

> why do we need to update it here as well?

It was needed by the part where the slot is not existing and we need
to create a new slot. I have now moved the invalidation check in
caller; we do not create the slot itself if remote_slot is found as
invalidated one in the beginning. And if it is invalidated in between
of the wait logic, then it will be dropped by ReplicationSlotRelease.

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Tue, 19 Dec 2023 13:05:17 +0000

Dear Shveta,

I resumed to review the patch. I will play more about it, but I can post some
cosmetic comments.

====
walsender.c

01. WalSndWaitForStandbyConfirmation

```
+        sleeptime = WalSndComputeSleeptime(GetCurrentTimestamp());
```

It works well, but I'm not sure whether we should use WalSndComputeSleeptime()
because the function won't be called by walsender.

02.WalSndWaitForStandbyConfirmation

```
+        ConditionVariableTimedSleep(&WalSndCtl->wal_confirm_rcv_cv, sleeptime,
+                                    WAIT_EVENT_WAL_SENDER_WAIT_FOR_STANDBY_CONFIRMATION)
```

Hmm, is it OK to use the same event as WalSndWaitForWal()? IIUC it should be avoided.

03. WalSndShmemInit()

```
+
+        ConditionVariableInit(&WalSndCtl->wal_confirm_rcv_cv);
```

Unnecessary blank?

~~~~~
050_standby_failover_slots_sync.pl

04. General

My pgperltidy modified your test. Please check.

05.

```
# Create publication on the primary
```

Missing "a" before publication?

06.

```
$subscriber1->init(allows_streaming => 'logical');
...
$subscriber2->init(allows_streaming => 'logical');
```

IIUC, these settings are not needed.

07.

```
my $primary_insert_time = time();
```

The variable is not used.

08.

```
# Stop the standby associated with the specified physical replication slot so
# that the logical replication slot won't receive changes until the standby
# slot's restart_lsn is advanced or the slot is removed from the
# standby_slot_names list
```

Missing comma?

09.

```
$back_q->query_until(qr//,
	"SELECT pg_logical_slot_get_changes('test_slot', NULL, NULL);\n");
```

Not sure, should we have to close the back_q connection?

10.

```
# Remove the standby from the standby_slot_names list and reload the
# configuration
$primary->adjust_conf('postgresql.conf', 'standby_slot_names', "''");
$primary->psql('postgres', "SELECT pg_reload_conf()");
```

a.
Missing comma?

b.
I counted and reload function in perl (e.g., `$primary->reload;`) is more often to
be used. Do you have a reason to use pg_reload_conf()?

11.

```
# Now that the standby lsn has advanced, the primary must send the decoded
# changes to the subscription.
$publisher->wait_for_catchup('regress_mysub1');
```

Is the comment correct? I think primary sends data because the GUC is modified.

12.

```
# Put the standby back on the primary_slot_name for the rest of the tests
$primary->adjust_conf('postgresql.conf', 'standby_slot_names', 'sb1_slot');
$primary->restart();
```

Just to confirm - you used restart() here because we must ensure the GUC change is
propagated to all backends, right?

~~~~~
wait_event_names.txt

13.

```
+WAL_SENDER_WAIT_FOR_STANDBY_CONFIRMATION    "Waiting for the WAL to be received by physical standby in WAL sender process."
```

But there is a possibility that backend processes may wait with the event, right?

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

---

From: Amit Kapila <[email-masked]>
Date: Tue, 19 Dec 2023 18:49:03 +0530

On Tue, Dec 19, 2023 at 5:17 PM shveta malik <[email-masked]> wrote:
> Thanks for reviewing. These are addressed in v50.
>

I was still reviewing the v48 version and have a few comments as
below. If some of these are already addressed or not relevant, feel
free to ignore them.
1.
+ /*
+ * Slot sync worker can be stopped at any time.
+ * Use exit status 1 so the background worker is restarted.

We don't need to start the second line of comment in a separate line.

2.
+ * The assumption is that these dropped local invalidated slots will get
+ * recreated in next sync-cycle and it is okay to drop and recreate such slots

In the above line '.. local invalidated ..' sounds redundant. Shall we
remove it?

3.
+ if (remote_slot->confirmed_lsn > WalRcv->latestWalEnd)
+ {
+ SpinLockRelease(&WalRcv->mutex);
+ elog(ERROR, "skipping sync of slot \"%s\" as the received slot sync "

This error message looks odd to me. At least, it should be exiting
instead of skipping because we won't continue after this.

4.

Same problem as above. The skipping in error message doesn't seem to
be suitable for the purpose. Additionally, errdetail message should
end with a full stop.

5.

The start of the error message sounds odd. Shall we say 'cannot
synchronize ...'?

6. All except one of the callers of local_slot_update() marks the slot
dirty and the same is required as well. I think the remaining caller
should also mark it dirty and we should move
ReplicationSlotMarkDirty() in the caller space.

7.
+synchronize_one_slot(WalReceiverConn *wrconn, RemoteSlot *remote_slot)
{
...
+ /*
+ * Copy the invalidation cause from remote only if local slot is not
+ * invalidated locally, we don't want to overwrite existing one.
+ */
+ if (slot->data.invalidated == RS_INVAL_NONE)
+ {
+ SpinLockAcquire(&slot->mutex);
+ slot->data.invalidated = remote_slot->invalidated;
+ SpinLockRelease(&slot->mutex);
+ }
...

It doesn't seem that after changing the invalidated flag, we always
mark the slot dirty. Am, I missing something?

8.
+ /*
+ * Drop local slots that no longer need to be synced. Do it before
+ * synchronize_one_slot to allow dropping of slots before actual sync
+ * which are invalidated locally while still valid on the primary server.
+ */
+ drop_obsolete_slots(remote_slot_list);

The second part of the above comment seems redundant as that is obvious.

9.
+static WalReceiverConn *
+remote_connect(void)
+{
+ WalReceiverConn *wrconn = NULL;
+ char    *err;
+
+ wrconn = walrcv_connect(PrimaryConnInfo, true, false,
+ cluster_name[0] ? cluster_name : "slotsyncworker",
+ &err);
+ if (wrconn == NULL)
+ ereport(ERROR,
+ (errcode(ERRCODE_CONNECTION_FAILURE),
+ errmsg("could not connect to the primary server: %s", err)));
+ return wrconn;
+}

Do we need a function for this? It appears to be called from just one
place, so not sure if it is helpful to have a function for this.

-- 
With Regards,
Amit Kapila.

---

From: Amit Kapila <[email-masked]>
Date: Wed, 20 Dec 2023 11:22:47 +0530

> because the function won't be called by walsender.

I don't think it is correct to use this function because it is
walsender specific, for example, it uses 'last_reply_timestamp' which
won't be even initialized in the backend environment. We need to
probably use a different logic for sleep here or need to use a
hard-coded value. I think we should change the name of functions like
WalSndWaitForStandbyConfirmation() as they are no longer used by
walsender. IIRC, earlier, we had a common logic to wait from both
walsender and SQL APIs which led to this naming but that is no longer
true with the latest patch.

> Hmm, is it OK to use the same event as WalSndWaitForWal()? IIUC it should be avoided.

Agreed. So, how about using WAIT_EVENT_WAIT_FOR_STANDBY_CONFIRMATION
so that we can use it both from the backend and walsender?

-- 
With Regards,
Amit Kapila.

---

From: Peter Smith <[email-masked]>
Date: Wed, 20 Dec 2023 17:31:38 +1100

Here are some comments for the patch v50-0002.

======
GENERAL

(I made a short study of all the ereports in this patch -- here are
some findings)

~~~

0.1 Don't need the parentheses.

Checking all the ereports I see that half of them have the redundant
parentheses and half of them do not; You might as well make them all
use the new style where the extra parentheses are not needed.

e.g.
+ ereport(LOG,
+ (errmsg("skipping slot synchronization"),
+ errdetail("enable_syncslot is disabled.")));

e.g.
+ ereport(ERROR,
+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("cannot drop replication slot \"%s\"", name),
+ errdetail("This slot is being synced from the primary server.")));

and many more like this. Search for all the ereports.

~~~

0.2
+ ereport(LOG,
+ (errmsg("dropped replication slot \"%s\" of dbid %d as it "
+ "was not sync-ready", NameStr(s->data.name),
+ s->data.database)));

I felt maybe that could be:

errmsg("dropped replication slot \"%s\" of dbid %d", ...
errdetail("It was not sync-ready.")

(now this shares the same errmsg with another ereport)

~~~

0.3.
+ ereport(ERROR,
+    " the same name already exists on the standby.")));

This seemed too wordy. Can't it be shortened (maybe like below)
without losing any of the vital information?

errmsg("skipping sync of slot \"%s\"", ...)
errdetail("A user-created slot with the same name already exists on
the standby.")

~~~

0.4
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("exiting from slot synchronization due to bad configuration"),
+ /* translator: second %s is a GUC variable name */
+ errdetail("The primary slot \"%s\" specified by %s is not valid.",
+    PrimarySlotName, "primary_slot_name")));

/The primary slot/The primary server slot/

~~~

0.5
+ ereport(ERROR,
+ (errmsg("could not fetch primary_slot_name \"%s\" info from the "
+ "primary: %s", PrimarySlotName, res->err)));

/primary:/primary server:/

~~~

0.6
The continuations for long lines are inconsistent. Sometimes there are
trailing spaces and sometimes there are leading spaces. And sometimes
there are both at the same time which would cause double-spacing in
the message! Please make them all the same. I think using leading
spaces is easier but YMMV.

e.g.
+ elog(ERROR,

======

1. check_primary_info

+ /* No need to check further, return that we are cascading standby */
+ if (remote_in_recovery)
+ {
+ *am_cascading_standby = true;
+ ExecClearTuple(tupslot);
+ walrcv_clear_result(res);
+ CommitTransactionCommand();
+ return;
+ }
+
+ valid = DatumGetBool(slot_getattr(tupslot, 2, &isnull));
+ Assert(!isnull);
+
+ if (!valid)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("exiting from slot synchronization due to bad configuration"),
+ /* translator: second %s is a GUC variable name */
+ errdetail("The primary slot \"%s\" specified by %s is not valid.",
+    PrimarySlotName, "primary_slot_name")));
+ ExecClearTuple(tupslot);
+ walrcv_clear_result(res);
+ CommitTransactionCommand();
+}

Now that there is a common cleanup/return code this function be
reduced further like below:

SUGGESTION

if (remote_in_recovery)
{
  /* No need to check further, return that we are cascading standby */
  *am_cascading_standby = true;
}
else
{
  /* We are a normal standby. */

  valid = DatumGetBool(slot_getattr(tupslot, 2, &isnull));
  Assert(!isnull);

  if (!valid)
    ...
}

ExecClearTuple(tupslot);
walrcv_clear_result(res);
CommitTransactionCommand();
}

~~~

2. ReplSlotSyncWorkerMain

+ /*
+ * One can promote the standby and we can no longer be a cascading
+ * standby. So recheck here.
+ */
+ if (am_cascading_standby)
+ check_primary_info(wrconn, &am_cascading_standby);

Minor rewording of that new comment.

SUGGESTION
If the standby was promoted then what was previously a cascading
standby might no longer be one, so recheck each time.

======

3.
+##################################################
+# Test that a synchronized slot can not be decoded, altered and
dropped by the user
+##################################################

/and dropped/or dropped/

~~~

4.
+
+($result, $stdout, $stderr) = $standby1->psql(
+    'postgres',
+    qq[ALTER_REPLICATION_SLOT lsub1_slot (failover);],
+    replication => 'database');
+ok($stderr =~ /ERROR:  cannot alter replication slot "lsub1_slot"/,
+ "synced slot on standby cannot be altered");
+

Add a comment for this test  part

SUGGESTION
Attempting to alter a synced slot should result in an error

~~~

5.
IMO it would be better if the tests were done in the same order
mentioned in the comment. So either change the tests or change the
comment.

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

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Wed, 20 Dec 2023 08:03:05 +0000

Dear Amit, Shveta,

> hard-coded value.

Oh, you are right. I haven't look until the func.

> I think we should change the name of functions like

How about "WaitForStandbyConfirmation", which is simpler? There are some
functions like "WaitForParallelWorkersToFinish", "WaitForProcSignalBarrier" and so on.

> WAIT_EVENT_WAL_SENDER_WAIT_FOR_STANDBY_CONFIRMATION)
> > ```
> >

Seems right. Note again that a description of .txt file must be also fixed.

Anyway, further comments on v50-0001.

~~~~~
protocol.sgml

01. create_replication_slot

```
+       <varlistentry>
+        <term><literal>FAILOVER { 'true' | 'false' }</literal></term>
+        <listitem>
+         <para>
+          If true, the slot is enabled to be synced to the physical
+          standbys so that logical replication can be resumed after failover.
+         </para>
+        </listitem>
+       </varlistentry>
```

IIUC, the true/false is optional. libpqwalreceiver does not add the boolean.
Also you can follow the notation of `TWO_PHASE`.

02. alter_replication_slot

```
+      <variablelist>
+       <varlistentry>
+        <term><literal>FAILOVER { 'true' | 'false' }</literal></term>
+        <listitem>
+         <para>
+          If true, the slot is enabled to be synced to the physical
+          standbys so that logical replication can be resumed after failover.
+         </para>
+        </listitem>
+       </varlistentry>
+      </variablelist>
```

Apart from above, this boolean is mandatory, right?
But you can follow other notation.

~~~~~~~
slot.c

03. validate_standby_slots

```
+    /* Need a modifiable copy of string. */
...
+    /* Verify syntax and parse string into a list of identifiers. */
```

Unnecessary comma?

04. validate_standby_slots

```

It may be more efficient to exit earlier when ReplicationSlotCtl is NULL.

~~~~~~~
walsender.c

05. PhysicalWakeupLogicalWalSnd

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

The function can be called from backend processes, but you said "the current walsender"
in the comment.

06. WalSndRereadConfigAndReInitSlotList

```
+    pre_standby_slot_names = pstrdup(standby_slot_names);
```

I felt that we must preserve pre_standby_slot_names before calling ProcessConfigFile().

07. WalSndFilterStandbySlots

I felt the prefix "WalSnd" may not be needed because both backend processes and
walsender will call the function.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

---

From: "Zhijie Hou (Fujitsu)" <[email-masked]>
Date: Wed, 20 Dec 2023 12:42:28 +0000

On Wednesday, December 20, 2023 4:03 PM Kuroda, Hayato/黒田 隼人 <[email-masked]> wrote:

Hi,

> > > +        sleeptime =
> WalSndComputeSleeptime(GetCurrentTimestamp());
> > > ```
> > >

Thanks for the comments. I think WaitForStandbyConfirmation is OK.
And I removed the WalSnd prefix for these functions and move them to
slot.c where the standby_slot_names is declared.

> > > Hmm, is it OK to use the same event as WalSndWaitForWal()? IIUC it
> > > should be
> > avoided.
> > >
> >

Changed.

Changed.

> But you can follow other notation.

Right, changed it to optional to be consistent with others.

> +    /* Verify syntax and parse string into a list of identifiers. */

You mean comma or period ? I think the current style is OK.

> It may be more efficient to exit earlier when ReplicationSlotCtl is NULL.

I think even if ReplicationSlotCtl is NULL, we still need to check the syntax
of the slot names.

> + * Wake up the logical walsender processes with failover-enabled slots
> +if the
> + * physical slot of the current walsender is specified in
> +standby_slot_names
> + * GUC.
> + */
> +void

Changed the words.

> cascading

Good catch. Fixed.

> I felt the prefix "WalSnd" may not be needed because both backend processes
> and walsender will call the function.

Right, renamed.

Attach the V51 patch set which addressed Kuroda-san's comments.
I also tried to improve the test in 0003 to make it stable.

Best Regards,
Hou zj

---

From: "Zhijie Hou (Fujitsu)" <[email-masked]>
Date: Wed, 20 Dec 2023 12:43:55 +0000

Thanks for the comments.

> because the function won't be called by walsender.

Changed to a hard-coded value.

> Hmm, is it OK to use the same event as WalSndWaitForWal()? IIUC it should be

As discussed, I change the event name to a more common one,
so that it makes sense to use it in both places.

Removed.

> My pgperltidy modified your test. Please check.

Will run this in next version.

Changed.

> IIUC, these settings are not needed.

Yeah, removed.

Removed.

Added.

> Not sure, should we have to close the back_q connection?

Added the quit.

> be used. Do you have a reason to use pg_reload_conf()?

I think it was copied from other places, changed to ->reload.

> Is the comment correct? I think primary sends data because the GUC is

Fixed.

> propagated to all backends, right?

Yes, but I think restart is not necessary, so I changed it to reload.

> 
> ~~~~~
> wait_event_names.txt
> 
> 13.
> 
> ```
> +WAL_SENDER_WAIT_FOR_STANDBY_CONFIRMATION    "Waiting for the
> WAL to be received by physical standby in WAL sender process."
> ```
> 

Adjusted.

Best Regards,
Hou zj

---

From: "Zhijie Hou (Fujitsu)" <[email-masked]>
Date: Thu, 21 Dec 2023 02:23:12 +0000

> I also tried to improve the test in 0003 to make it stable.

The patches conflict with a recent commit dc21234.
Here is the rebased V51_2 version, there is no code changes in this version.

Best Regards,
Hou zj

---

From: Peter Smith <[email-masked]>
Date: Thu, 21 Dec 2023 15:24:31 +1100

Here is a minor comment for v51-0001

======

1.
+void
+RereadConfigAndReInitSlotList(List **standby_slots)
+{
+ char    *pre_standby_slot_names;
+
+ /*
+ * If we are running on a standby, there is no need to reload
+ * standby_slot_names since we do not support syncing slots to cascading
+ * standbys.
+ */
+ if (RecoveryInProgress())
+ {
+ ProcessConfigFile(PGC_SIGHUP);
+ return;
+ }
+
+ pre_standby_slot_names = pstrdup(standby_slot_names);
+
+ ProcessConfigFile(PGC_SIGHUP);
+
+ if (strcmp(pre_standby_slot_names, standby_slot_names) != 0)
+ {
+ list_free(*standby_slots);
+ *standby_slots = GetStandbySlotList(true);
+ }
+
+ pfree(pre_standby_slot_names);
+}

Consider below, which seems a simpler way to do that but with just one
return point and without duplicating the ProcessConfigFile calls:

SUGGESTION
{
char *pre_standby_slot_names = pstrdup(standby_slot_names);

ProcessConfigFile(PGC_SIGHUP);

if (!RecoveryInProgress())
{
  if (strcmp(pre_standby_slot_names, standby_slot_names) != 0)
  {
    list_free(*standby_slots);
    *standby_slots = GetStandbySlotList(true);
  }
}

pfree(pre_standby_slot_names);
}

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

---

From: shveta malik <[email-masked]>
Date: Thu, 21 Dec 2023 15:06:06 +0530

> Here are some comments for the patch v50-0002.

Thank You for the feedback. I have addressed these in v52.

> errdetail("A user-created slot with the same name already exists on

I have modified it a little bit more. Please see now. I wanted to add
the info that slot-sync worker is exiting instead of skipping a slot
and that the concerned slot is a failover slot on primary. These were
the other comments around the same.