Horton

Re: Synchronizing slots from primary to standby

Amit Kapila <amit.kapila16@gmail.com>

View thread View on postgr.es
From: Amit Kapila <amit.kapila16@gmail.com>
To: shveta malik <shveta.malik@gmail.com>
Cc: Masahiko Sawada <sawada.mshk@gmail.com>, "Zhijie Hou (Fujitsu)" <houzj.fnst@fujitsu.com>, Peter Smith <smithpb2250@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>, 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>, Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Date: 2024-02-21T11:48:48Z
Lists: pgsql-hackers 
On Wed, Feb 21, 2024 at 12:15 PM shveta malik <shveta.malik@gmail.com> wrote:
>
>
> Thanks for the feedback. I have addressed it in v93.
>

A few minor comments:
=================
1.
+/*
+ * Is stopSignaled set in SlotSyncCtx?
+ */
+bool
+IsStopSignaledSet(void)
+{
+ bool signaled;
+
+ SpinLockAcquire(&SlotSyncCtx->mutex);
+ signaled = SlotSyncCtx->stopSignaled;
+ SpinLockRelease(&SlotSyncCtx->mutex);
+
+ return signaled;
+}
+
+/*
+ * Reset stopSignaled in SlotSyncCtx.
+ */
+void
+ResetStopSignaled(void)
+{
+ SpinLockAcquire(&SlotSyncCtx->mutex);
+ SlotSyncCtx->stopSignaled = false;
+ SpinLockRelease(&SlotSyncCtx->mutex);
+}

I think these newly introduced functions don't need spinlock to be
acquired as these are just one-byte read-and-write. Additionally, when
IsStopSignaledSet() is invoked, there shouldn't be any concurrent
process to update that value. What do you think?

2.
+REPL_SLOTSYNC_MAIN "Waiting in main loop of slot sync worker."
+REPL_SLOTSYNC_SHUTDOWN "Waiting for slot sync worker to shut down."

Let's use REPLICATION instead of REPL. I see other wait events using
REPLICATION in their names.

3.
- * In standalone mode and in autovacuum worker processes, we use a fixed
- * ID, otherwise we figure it out from the authenticated user name.
+ * In standalone mode, autovacuum worker processes and slot sync worker
+ * process, we use a fixed ID, otherwise we figure it out from the
+ * authenticated user name.
*/
- if (bootstrap || IsAutoVacuumWorkerProcess())
+ if (bootstrap || IsAutoVacuumWorkerProcess() || IsLogicalSlotSyncWorker())
{
InitializeSessionUserIdStandalone();
am_superuser = true;

IIRC, we discussed this previously and it is safe to make the local
connection as superuser as we don't consult any user tables, so we can
probably add a comment where we invoke InitPostgres in slotsync.c

4.
$publisher->safe_psql('postgres',
- "CREATE PUBLICATION regress_mypub FOR ALL TABLES;");
+ "CREATE PUBLICATION regress_mypub FOR ALL TABLES;"
+);

Why this change is required in the patch?

5.
+# Confirm that restart_lsn and of confirmed_flush_lsn lsub1_slot slot
are synced
+# to the standby

/and of/; looks like a typo

6.
+# Confirm that restart_lsn and of confirmed_flush_lsn lsub1_slot slot
are synced
+# to the standby
+ok( $standby1->poll_query_until(
+ 'postgres',
+ "SELECT '$primary_restart_lsn' = restart_lsn AND
'$primary_flush_lsn' = confirmed_flush_lsn from pg_replication_slots
WHERE slot_name = 'lsub1_slot';"),
+ 'restart_lsn and confirmed_flush_lsn of slot lsub1_slot synced to standby');
+
...
...
+# Confirm the synced slot 'lsub1_slot' is retained on the new primary
+is($standby1->safe_psql('postgres',
+ q{SELECT slot_name FROM pg_replication_slots WHERE slot_name =
'lsub1_slot';}),
+ 'lsub1_slot',
+ 'synced slot retained on the new primary');

In both these checks, we should additionally check the 'synced' and
'temporary' flags to ensure that they are marked appropriately.

-- 
With Regards,
Amit Kapila.

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