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