Re: Synchronizing slots from primary to standby

On Thu, Feb 15, 2024 at 10:48 PM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:
>
> Looking at v88_0001, random comments:

Thanks for the feedback.

>
> 1 ===
>
> Commit message "Be enabling slot synchronization"
>
> Typo? s:Be/By

Modified.

> 2 ===
>
> +        It enables a physical standby to synchronize logical failover slots
> +        from the primary server so that logical subscribers are not blocked
> +        after failover.
>
> Not sure "not blocked" is the right wording.
> "can be resumed from the new primary" maybe? (was discussed in [1])

Modified.

> 3 ===
>
> +#define SlotSyncWorkerAllowed()        \
> +       (sync_replication_slots && pmState == PM_HOT_STANDBY && \
> +        SlotSyncWorkerCanRestart())
>
> Maybe add a comment above the macro explaining the logic?

Done.

> 4 ===
>
> +#include "replication/walreceiver.h"
>  #include "replication/slotsync.h"
>
> should be reverse order?

Removed walreceiver.h inclusion as it was not needed.

> 5 ===
>
> +       if (SlotSyncWorker->syncing)
>         {
> -               SpinLockRelease(&SlotSyncCtx->mutex);
> +               SpinLockRelease(&SlotSyncWorker->mutex);
>                 ereport(ERROR,
>                                 errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
>                                 errmsg("cannot synchronize replication slots concurrently"));
>         }
>
> worth to add a test in 040_standby_failover_slots_sync.pl for it?

It will be very difficult to stabilize this test as we have to make
sure that the concurrent users (SQL function(s) and/or worker(s)) are
in that target function at the same time to hit it.

>
> 6 ===
>
> +static void
> +slotsync_reread_config(bool restart)
> +{
>
> worth to add test(s) in 040_standby_failover_slots_sync.pl for it?

Added test.

Please find v89  patch set. The other changes are:

patch001:
1) Addressed some comments by Amit and Ajin given off-list.
2) Removed redundant header inclusions from slotsync.c.
3) Corrected the value returned by validate_remote_info().
4) Restructured code around validate_remote_info.
5) Improved comments and commit msg.

patch002:
Rebased it.

thanks
Shveta