Subject: Re: promotion related handling in pg_sync_replication_slots()

From: shveta malik <[email-masked]>
Date: Fri, 5 Apr 2024 10:31:15 +0530

On Thu, Apr 4, 2024 at 5:17 PM Amit Kapila <[email-masked]> wrote:
>
> On Thu, Apr 4, 2024 at 5:05 PM shveta malik <[email-masked]> wrote:
> >
> > Hello hackers,
> >
> > Currently, promotion related handling is missing in the slot sync SQL
> > function pg_sync_replication_slots().   Here is the background on how
> > it is done in slot sync worker:
> > During promotion, the startup process in order to shut down the
> > slot-sync worker, sets the 'stopSignaled' flag, sends the shut-down
> > signal, and waits for slot sync worker to exit. Meanwhile if the
> > postmaster has not noticed the promotion yet, it may end up restarting
> > slot sync worker. In such a case, the worker exits if 'stopSignaled'
> > is set.
> >
> > Since there is a chance that the user (or any of his scripts/tools)
> > may execute SQL function pg_sync_replication_slots() in parallel to
> > promotion, such handling is needed in this SQL function as well, The
> > attached patch attempts to implement the same.
> >
>
> Thanks for the report and patch. I'll look into it.
>

Please find v2. Changes are:
1) Rebased the patch as there was conflict due to recent commit 6f132ed.
2) Added an Assert in update_synced_slots_inactive_since() to ensure
that the slot does not have active_pid.
3) Improved commit msg and comments.

thanks
Shveta

---

From: Amit Kapila <[email-masked]>
Date: Sat, 6 Apr 2024 11:49:19 +0530

> 3) Improved commit msg and comments.

Few comments:
==============
1.
 void
 SyncReplicationSlots(WalReceiverConn *wrconn)
 {
+ /*
+ * Startup process signaled the slot sync to stop, so if meanwhile user
+ * has invoked slot sync SQL function, simply return.
+ */
+ SpinLockAcquire(&SlotSyncCtx->mutex);
+ if (SlotSyncCtx->stopSignaled)
+ {
+ ereport(LOG,
+ errmsg("skipping slot synchronization as slot sync shutdown is
signaled during promotion"));
+
+ SpinLockRelease(&SlotSyncCtx->mutex);
+ return;
+ }
+ SpinLockRelease(&SlotSyncCtx->mutex);

There is a race condition with this code. Say during promotion
ShutDownSlotSync() is just before setting this flag and the user has
invoked pg_sync_replication_slots() and passed this check but still
didn't set the SlotSyncCtx->syncing flag. So, now, the promotion would
recognize that there is slot sync going on in parallel, and slot sync
wouldn't know that the promotion is in progress.

The current coding for slot sync worker ensures there is no such race
by checking the stopSignaled and setting the pid together under
spinlock. I think we need to move the setting of the syncing flag
similarly. Once we do that probably checking SlotSyncCtx->syncing
should be sufficient in ShutDownSlotSync(). If we change the location
of setting the 'syncing' flag then please ensure its cleanup as we
currently do in slotsync_failure_callback().

2.
@@ -1395,6 +1395,7 @@ update_synced_slots_inactive_since(void)
  if (s->in_use && s->data.synced)
  {
  Assert(SlotIsLogical(s));
+ Assert(s->active_pid == 0);

We can add a comment like: "The slot must not be acquired by any process"

-- 
With Regards,
Amit Kapila.

---

From: Bharath Rupireddy <[email-masked]>
Date: Sat, 6 Apr 2024 12:25:26 +0530

> Please find v2. Changes are:

Thanks for the patch. Here are some comments.

1. Can we have a clear saying in the shmem variable who's syncing at
the moment? Is it a slot sync worker or a backend via SQL function?
Perhaps turn "bool syncing;" to "SlotSyncSource sync_source;"

typedef enum SlotSyncSource
{
    SLOT_SYNC_NONE,
    SLOT_SYNC_WORKER,
    SLOT_SYNC_BACKEND,
} SlotSyncSource;

Then, the check in ShutDownSlotSync can be:

+       /*
+        * Return if neither the slot sync worker is running nor the function
+        * pg_sync_replication_slots() is executing.
+        */
+       if ((SlotSyncCtx->pid == InvalidPid) &&
SlotSyncCtx->sync_source != SLOT_SYNC_BACKEND)
        {

2.
SyncReplicationSlots(WalReceiverConn *wrconn)
 {
+    /*
+     * Startup process signaled the slot sync to stop, so if meanwhile user
+     * has invoked slot sync SQL function, simply return.
+     */
+    SpinLockAcquire(&SlotSyncCtx->mutex);
+    if (SlotSyncCtx->stopSignaled)
+    {
+        ereport(LOG,
+                errmsg("skipping slot synchronization as slot sync
shutdown is signaled during promotion"));
+

Unless I'm missing something, I think this can't detect if the backend
via SQL function is already half-way through syncing in
synchronize_one_slot. So, better move this check to (or also have it
there) slot sync loop that calls synchronize_one_slot. To avoid
spinlock acquisitions, we can perhaps do this check in when we acquire
the spinlock for synced flag.

    /* Search for the named slot */
    if ((slot = SearchNamedReplicationSlot(remote_slot->name, true)))
    {
        bool        synced;

        SpinLockAcquire(&slot->mutex);
        synced = slot->data.synced;
        << get SlotSyncCtx->stopSignaled here >>
        SpinLockRelease(&slot->mutex);

        << do the slot sync skip check here if (stopSignaled) >>

3. Can we have a test or steps at least to check the consequences
manually one can get if slot syncing via SQL function is happening
during the promotion?

IIUC, we need to ensure there is no backend acquiring it and
performing sync while the slot sync worker is shutting down/standby
promotion is occuring. Otherwise, some of the slots can get resynced
and some are not while we are shutting down the slot sync worker as
part of the standby promotion which might leave the slots in an
inconsistent state.

-- 
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

---

From: shveta malik <[email-masked]>
Date: Fri, 12 Apr 2024 07:47:39 +0530

> > 3) Improved commit msg and comments.

Did you mean that now, the promotion *would not* recognize...

I see, I will fix this.

> currently do in slotsync_failure_callback().

Sure, let me review.

---

From: shveta malik <[email-masked]>
Date: Fri, 12 Apr 2024 07:57:39 +0530

> Thanks for the patch. Here are some comments.

Thanks for reviewing.
> the spinlock for synced flag.

If the sync via SQL function is already half-way, then promotion
should wait for it to finish. I don't think it is a good idea to move
the check to synchronize_one_slot().  The sync-call should either not
start (if it noticed the promotion) or finish the sync and then let
promotion proceed. But I would like to know others' opinion on this.

> part of the standby promotion which might leave the slots in an

I do not think that we can reach a state (exception is some error
scenario) where some of the slots are synced while the rest are not
during a *particular* sync-cycle only because promotion is going in
parallel. (And yes we need to fix the race-condition stated by Amit
up-thread for this statement to be true.)

thanks
Shveta

---

From: Amit Kapila <[email-masked]>
Date: Fri, 12 Apr 2024 16:07:06 +0530

> Did you mean that now, the promotion *would not* recognize...

Right.

> I see, I will fix this.

Thanks.

-- 
With Regards,
Amit Kapila.

---

From: Amit Kapila <[email-masked]>
Date: Fri, 12 Apr 2024 16:17:53 +0530

> > SlotSyncCtx->sync_source != SLOT_SYNC_BACKEND)

I don't know if this will be help, especially after fixing the race
condition I mentioned. But otherwise, also, at this stage it doesn't
seem helpful to add the source of sync explicitly.

> promotion proceed. But I would like to know others' opinion on this.

Agreed.

-- 
With Regards,
Amit Kapila.