Re: Synchronizing slots from primary to standby

On Wed, Feb 21, 2024 at 12:15 PM shveta malik <shveta(dot)malik(at)gmail(dot)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.