On Fri, Dec 1, 2023 at 9:40 AM Zhijie Hou (Fujitsu) <houzj.f...@fujitsu.com> wrote: > > On Wednesday, November 29, 2023 5:12 PM Zhijie Hou (Fujitsu) > <houzj.f...@fujitsu.com> wrote: > > I was reviewing slotsync worker design and here > are few comments on 0002 patch:
Thanks for reviewing the patch. > > 1. > > + if (!WalRcv || > + (WalRcv->slotname[0] == '\0') || > + XLogRecPtrIsInvalid(WalRcv->latestWalEnd)) > > I think we'd better take spinlock when accessing these shared memory fields. > > 2. > > /* > * The slot sync feature itself is disabled, exit. > */ > if (!enable_syncslot) > { > ereport(LOG, > errmsg("exiting slot sync worker as > enable_syncslot is disabled.")); > > Can we check the GUC when registering the worker(SlotSyncWorkerRegister), > so that the worker won't be started if enable_syncslot is false. > > 3. In synchronize_one_slot, do we need to skip the slot sync and drop if the > local slot is a physical one ? > IMO, if a local slot exists which is a physical one, it will be a user created slot and in that case worker will error out on finding existing slot with same name. And the case where local slot is physical one but not user-created is not possible on standby (assuming we have correct check on primary disallowing setting 'failover' property for physical slot). Do you have some other scenario in mind, which I am missing here? > 4. > > *locally_invalidated = > (remote_slot->invalidated == RS_INVAL_NONE) && > (local_slot->data.invalidated != > RS_INVAL_NONE); > > When reading the invalidated flag of local slot, I think we'd better take > spinlock. > > Best Regards, > Hou zj