Re: Synchronizing slots from primary to standby

Peter Smith <smithpb2250@gmail.com>

From: Peter Smith <smithpb2250@gmail.com>
To: shveta malik <shveta.malik@gmail.com>
Cc: Masahiko Sawada <sawada.mshk@gmail.com>, Amit Kapila <amit.kapila16@gmail.com>, "Zhijie Hou (Fujitsu)" <houzj.fnst@fujitsu.com>, Dilip Kumar <dilipbalaut@gmail.com>, Bertrand Drouvot <bertranddrouvot.pg@gmail.com>, Nisha Moond <nisha.moond412@gmail.com>, "Hayato Kuroda (Fujitsu)" <kuroda.hayato@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: 2024-01-17T04:37:14Z
Lists: pgsql-hackers
On Tue, Jan 16, 2024 at 10:57 PM shveta malik <shveta.malik@gmail.com> wrote:
>
...
> v62-006:
> Separated the failover-ready validation steps into this separate
> doc-patch (which were earlier present in v61-002 and v61-003). Also
> addressed some of the doc comments by Peter in [1].
> Thanks Hou-San for providing this patch.
>
> [1]: https://www.postgresql.org/message-id/CAHut%2BPteZVNx1jQ6Hs3mEdoC%3DDNALVpJJ2mZDYim7sU-04tiaw%40mail.gmail.com
>

Thanks for addressing my previous review in the new patch 0006. I
checked it again and below are a few more comments

======
1. GENERAL

I was wondering if some other documentation (like somewhere from
chapter 27, or maybe the pgctl promote docs?) should be referring back
to this new information about how to decide if the standby is ready
for promotion.

======
doc/src/sgml/logical-replication.sgml

2.
+
+  <para>
+   Because the slot synchronization logic copies asynchronously, it is
+   necessary to confirm that replication slots have been synced to the standby
+   server before the failover happens. Furthermore, to ensure a successful
+   failover, the standby server must not be lagging behind the subscriber. It
+   is highly recommended to use <varname>standby_slot_names</varname> to
+   prevent the subscriber from consuming changes faster than the hot standby.
+   To confirm that the standby server is indeed ready for failover, follow
+   these 2 steps:
+  </para>

For easier navigation, perhaps that standby_slot_names should include
a link back to where the standby_slot_names GUC is described.

~~~

3.
+    <substeps>
+     <step performance="required">
+      <para>
+       Firstly, on the subscriber node, use the following SQL to identify the
+       slot names that should be synced to the standby that we plan to promote.

Minor change to wording.

SUGGESTION
Firstly, on the subscriber node, use the following SQL to identify
which slots should be synced to the standby that we plan to promote.

~~~

4.
+<programlisting>
+test_sub=# SELECT
+               array_agg(slotname) AS slots
+           FROM
+           ((
+               SELECT r.srsubid AS subid, CONCAT('pg_' || srsubid ||
'_sync_' || srrelid || '_' || ctl.system_identifier) AS slotname
+               FROM pg_control_system() ctl, pg_subscription_rel r,
pg_subscription s
+               WHERE r.srsubstate = 'f' AND s.oid = r.srsubid AND s.subfailover
+           ) UNION (
+               SELECT s.oid AS subid, s.subslotname as slotname
+               FROM pg_subscription s
+               WHERE s.subfailover
+           ));
+ slots
+-------
+ {sub}
+(1 row)
+</programlisting></para>
+     </step>

I think the example might be better if the result shows > 1 slot.  e.g.
{sub1,sub2,sub3}

This would also make the next step 1.b. more clear.

~~~

5.
+</programlisting></para>
+     </step>
+     <step performance="required">
+      <para>
+       Next, check that the logical replication slots identified above exist on
+       the standby server. This step can be skipped if
+       <varname>standby_slot_names</varname> has been correctly configured.
+<programlisting>
+test_standby=# SELECT bool_and(synced AND NOT temporary AND
conflict_reason IS NULL) AS failover_ready
+               FROM pg_replication_slots
+               WHERE slot_name in ('sub');
+ failover_ready
+----------------
+ t
+(1 row)
+</programlisting></para>

5a.

(uppercase SQL keyword)

/in/IN/

~

5b.
I felt this might be easier to understand if the SQL gives a
two-column result instead of one all-of-nothing T/F where you might no
be sure which slot was the one giving a problem. e.g.

failover_ready | slot
---------------------
t              | sub1
t              | sub2
f              | sub3
...

~~~

6.
+       <para>
+        Firstly, on the subscriber node check the last replayed WAL. If the
+        query result is NULL, it indicates that the subscriber has not yet
+        replayed any WAL. Therefore, the next step can be skipped, as the
+        standby server must be ahead of the subscriber.

IMO all of that part "If the query result is NULL" does not really
belong here because it describes skipping the *next* step. So, it
would be better to say this in the next step. Something like:

SUGGESTION (for step 2b)
Next, on the standby server check that the last-received WAL location
is ahead of the replayed WAL location on the subscriber identified
above. If the above SQL result was NULL, it means the subscriber has
not yet replayed any WAL, so the standby server must be ahead of the
subscriber, and this step can be skipped.

~~~

7.
+<programlisting>
+test_sub=# SELECT
+               MAX(remote_lsn) AS remote_lsn_on_subscriber
+           FROM
+           ((
+               SELECT (CASE WHEN r.srsubstate = 'f' THEN
pg_replication_origin_progress(CONCAT('pg_' || r.srsubid || '_' ||
r.srrelid), false)
+                           WHEN r.srsubstate IN ('s', 'r') THEN
r.srsublsn END) as remote_lsn
+               FROM pg_subscription_rel r, pg_subscription s
+               WHERE r.srsubstate IN ('f', 's', 'r') AND s.oid =
r.srsubid AND s.subfailover
+           ) UNION (
+               SELECT pg_replication_origin_progress(CONCAT('pg_' ||
s.oid), false) AS remote_lsn
+               FROM pg_subscription s
+               WHERE subfailover
+           ));
+ remote_lsn_on_subscriber
+--------------------------
+ 0/3000388
+</programlisting></para>

7a.
(uppercase SQL keyword)

/as/AS/

~

7b.
missing table alias

/WHERE subfailover/WHERE s.subfailover/

~~~

8.
+    </step>
+    <step performance="required">
+     <para>
+      Next, on the standby server check that the last-received WAL location
+      is ahead of the replayed WAL location on the subscriber identified above.


See the review comment above (#6) which suggested adding some more info here.

======
Kind Regards,
Peter Smith.
Fujitsu Australia



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