Horton

Re: Synchronizing slots from primary to standby

Masahiko Sawada <sawada.mshk@gmail.com>

View thread View on postgr.es
From: Masahiko Sawada <sawada.mshk@gmail.com>
To: "Zhijie Hou (Fujitsu)" <houzj.fnst@fujitsu.com>
Cc: Amit Kapila <amit.kapila16@gmail.com>, shveta malik <shveta.malik@gmail.com>, Ajin Cherian <itsajin@gmail.com>, Dilip Kumar <dilipbalaut@gmail.com>, Nisha Moond <nisha.moond412@gmail.com>, "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>, Peter Smith <smithpb2250@gmail.com>, Bertrand Drouvot <bertranddrouvot.pg@gmail.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>, Alvaro Herrera <alvherre@alvh.no-ip.org>
Date: 2024-03-01T06:10:57Z
Lists: pgsql-hackers 
On Fri, Mar 1, 2024 at 12:42 PM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:
>
> On Friday, March 1, 2024 10:17 AM Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com> wrote:
> >
> >
> > Attach the V102 patch set which addressed Amit and Shveta's comments.
> > Thanks Shveta for helping addressing the comments off-list.
>
> The cfbot reported a compile warning, here is the new version patch which fixed it,
> Also removed some outdate comments in this version.
>

Thank you for updating the patch!

I've reviewed the v102-0001 patch. Here are some comments:

---
I got a compiler warning:

walsender.c:1829:6: warning: variable 'wait_event' is used
uninitialized whenever '&&' condition is false
[-Wsometimes-uninitialized]
        if (!XLogRecPtrIsInvalid(RecentFlushPtr) &&
            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
walsender.c:1871:7: note: uninitialized use occurs here
                if (wait_event == WAIT_EVENT_WAL_SENDER_WAIT_FOR_WAL)
                    ^~~~~~~~~~
walsender.c:1829:6: note: remove the '&&' if its condition is always true
        if (!XLogRecPtrIsInvalid(RecentFlushPtr) &&
            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
walsender.c:1818:20: note: initialize the variable 'wait_event' to
silence this warning
        uint32          wait_event;
                                  ^
                                   = 0
1 warning generated.

---
+void
+assign_standby_slot_names(const char *newval, void *extra)
+{
+        List      *standby_slots;
+        MemoryContext oldcxt;
+        char      *standby_slot_names_cpy = extra;
+

Given that the newval and extra have the same data (standby_slot_names
value), why do we not use newval instead? I think that if we use
newval, we don't need to guc_strdup() in check_standby_slot_names(),
we might need to do list_copy_deep() instead, though. It's not clear
to me as there is no comment.

---
+
+        standby_slot_oldest_flush_lsn = min_restart_lsn;
+

IIUC we expect that standby_slot_oldest_flush_lsn never moves
backward. If so, I think it's better to have an assertion here.

---
Resetting standby_slot_names doesn't work:

=# alter system set standby_slot_names to '';
ERROR:  invalid value for parameter "standby_slot_names": """"
DETAIL:  replication slot "" does not exist

---
+        /*
+         * Switch to the memory context under which GUC variables are allocated
+         * (GUCMemoryContext).
+         */
+        oldcxt =
MemoryContextSwitchTo(GetMemoryChunkContext(standby_slot_names_cpy));
+        standby_slot_names_list = list_copy(standby_slots);
+        MemoryContextSwitchTo(oldcxt);

Why do we not explicitly switch to GUCMemoryContext?

---
+        if (!standby_slot_names_list)
+                return true;
+

Probably "standby_slot_names_list == NIL" is more consistent with
other places. The same can be applied in WaitForStandbyConfirmation().

---
+        /*
+         * Return true if all the standbys have already caught up to
the passed in
+         * WAL localtion.
+         */
+

s/localtion/location/

---
I was a bit surprised by the fact that standby_slot_names value is
handled in a different way than a similar parameter
synchronous_standby_names. For example, the following command doesn't
work unless there is a replication slot 'slot1, slot2':

=# alter system set standby_slot_names to 'slot1, slot2';
ERROR:  invalid value for parameter "standby_slot_names": ""slot1, slot2""
DETAIL:  replication slot "slot1, slot2" does not exist

Whereas "alter system set synchronous_standby_names to stb1, stb2;"
can correctly separate the string into 'stb1' and 'stb2'.

Probably it would be okay since this behavior of standby_slot_names is
the same as other GUC parameters that accept a comma-separated string.
But I was confused a bit the first time I used it.

---
+        /*
+         * "*" 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("\"*\" is not accepted for
standby_slot_names");
+                return false;
+        }

Why only '*' is checked aside from validate_standby_slots()? I think
that the doc doesn't mention anything about '*' and '*' cannot be used
as a replication slot name. So even if we don't have this check, it
might be no problem.

Regards,

-- 
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

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).