Re: Synchronizing slots from primary to standby

On Fri, Dec 1, 2023 at 9:40 AM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:
>
> On Wednesday, November 29, 2023 5:12 PM Zhijie Hou (Fujitsu) <houzj.fnst@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