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