RE: Synchronizing slots from primary to standby

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