Hi, On Thu, Feb 15, 2024 at 06:13:38PM +0530, Amit Kapila wrote: > On Thu, Feb 15, 2024 at 12:07 PM Zhijie Hou (Fujitsu) > <houzj.f...@fujitsu.com> wrote: > > > > Since the slotsync function is committed, I rebased remaining patches. > > And here is the V88 patch set. > >
Thanks! > > Please find the improvements in some of the comments in v88_0001* > attached. Kindly include these in next version, if you are okay with > it. Looking at v88_0001, random comments: 1 === Commit message "Be enabling slot synchronization" Typo? s:Be/By 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]) 3 === +#define SlotSyncWorkerAllowed() \ + (sync_replication_slots && pmState == PM_HOT_STANDBY && \ + SlotSyncWorkerCanRestart()) Maybe add a comment above the macro explaining the logic? 4 === +#include "replication/walreceiver.h" #include "replication/slotsync.h" should be reverse order? 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? 6 === +static void +slotsync_reread_config(bool restart) +{ worth to add test(s) in 040_standby_failover_slots_sync.pl for it? [1]: https://www.postgresql.org/message-id/CAA4eK1JcBG6TJ3o5iUd4z0BuTbciLV3dK4aKgb7OgrNGoLcfSQ%40mail.gmail.com Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com