Re: Synchronizing slots from primary to standby

On Thu, Feb 15, 2024 at 10:48 PM Bertrand Drouvot
<bertranddrouvot(dot)pg(at)gmail(dot)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