Horton

RE: Synchronizing slots from primary to standby

Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>

View thread View on postgr.es
From: "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>
To: 'Amit Kapila' <amit.kapila16@gmail.com>, shveta malik <shveta.malik@gmail.com>
Cc: Peter Smith <smithpb2250@gmail.com>, "Drouvot, Bertrand" <bertranddrouvot.pg@gmail.com>, Nisha Moond <nisha.moond412@gmail.com>, "Zhijie Hou (Fujitsu)" <houzj.fnst@fujitsu.com>, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>, Peter Eisentraut <peter.eisentraut@enterprisedb.com>, Bruce Momjian <bruce@momjian.us>, Ashutosh Sharma <ashu.coek88@gmail.com>, Andres Freund <andres@anarazel.de>, pgsql-hackers <pgsql-hackers@postgresql.org>, Ajin Cherian <itsajin@gmail.com>, Alvaro Herrera <alvherre@alvh.no-ip.org>
Date: 2023-12-20T08:03:05Z
Lists: pgsql-hackers 
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

Commits

  1. Doc: Add the new section "Logical Replication Failover".

  2. Fix the review comments and a bug in the slot sync code.

  3. Fix a test in failover slots regression test.

  4. Fix the intermittent buildfarm failures in 040_standby_failover_slots_sync.

  5. Split XLogCtl->LogwrtResult into separate struct members

  6. Ensure that the sync slots reach a consistent state after promotion without losing data.

  7. Introduce a new GUC 'standby_slot_names'.

  8. Fix BF failure introduced by commit b3f6b14cf4.

  9. Fixups for commit 93db6cbda0.

  10. Fix BF failure in commit 93db6cbda0.

  11. Add a new slot sync worker to synchronize logical slots.

  12. Improve ERROR/LOG messages added by commits ddd5f4f54a and 7a424ece48.

  13. Disable autovacuum on primary in 040_standby_failover_slots_sync test.

  14. Fix the incorrect format specifier used in commit 7a424ece48.

  15. Change the LOG level in 040_standby_failover_slots_sync.pl to DEBUG2.

  16. Add more LOG and DEBUG messages for slot synchronization.

  17. Another try to fix BF failure introduced in commit ddd5f4f54a.

  18. Fix BF introduced in commit ddd5f4f54a.

  19. Add a slot synchronization function.

  20. Clean-ups for 776621a5e4 and 7329240437.

  21. Enhance libpqrcv APIs to support slot synchronization.

  22. Add a failover option to subscriptions.

  23. Allow setting failover property in the replication command.

  24. Allow to enable failover property for replication slots via SQL API.

  25. Add macros for looping through a List without a ListCell.

  26. Track conflict_reason in pg_replication_slots.

  27. Allow upgrades to preserve the full subscription's state.

  28. Add support for incremental backup.

  29. Simplify some logic in CreateReplicationSlot()

  30. Fix uninitialized access to InitialRunningXacts during decoding after ERROR.

  31. Flexible options for CREATE_REPLICATION_SLOT.

  32. Empty search_path in logical replication apply worker and walsender.

  33. Fix oldest xmin and LSN computation across repslots after advancing

  34. Require C99 (and thus MSCV 2013 upwards).