Dear Amit, Shveta,
> > 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.
> >
>
> 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.
Oh, you are right. I haven't look until the func.
> 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.
How about "WaitForStandbyConfirmation", which is simpler? There are some
functions like "WaitForParallelWorkersToFinish", "WaitForProcSignalBarrier" and
so on.
> > 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.
> >
>
> Agreed. So, how about using
> WAIT_EVENT_WAIT_FOR_STANDBY_CONFIRMATION
> so that we can use it both from the backend and walsender?
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
```
+ if (!ok || !ReplicationSlotCtl)
+ {
+ pfree(rawname);
+ list_free(elemlist);
+ return ok;
+ }
```
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
```
+ 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;
+
+ 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
