Re: promotion related handling in pg_sync_replication_slots()

On Tue, Apr 16, 2024 at 9:27 AM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:
>
> On Monday, April 15, 2024 6:09 PM shveta malik <shveta.malik@gmail.com> wrote:
> >
> > Please find v4 addressing the above comments.
>
> Thanks for the patch.
>
> Here are few comments:

Thanks for reviewing the patch.

>
> 1.
>
> +                       ereport(ERROR,
> +                                       errmsg("promotion in progress, can not synchronize replication slots"));
> +               }
>
> I think an errcode is needed.
>
> The style of the error message seems a bit unnatural to me. I suggest:
> "cannot synchronize replication slots when standby promotion is ongoing"

Modified.

>
> 2.
>
> +       if (worker_pid != InvalidPid)
> +               Assert(SlotSyncCtx->pid == InvalidPid);
>
> We could merge the checks into one Assert().
> Assert(SlotSyncCtx->pid == InvalidPid || worker_pid == InvalidPid);

Modified.

>
> 3.
>
> -       pqsignal(SIGINT, SignalHandlerForShutdownRequest);
>
> I realized that we should register this before setting SlotSyncCtx->pid,
> otherwise if the standby is promoted after setting pid and before registering
> signal handle function, the slotsync worker could miss to handle SIGINT sent by
> startup process(ShutDownSlotSync). This is an existing issue for slotsync
> worker, but maybe we could fix it together with the patch.

Yes, it seems like a problem. Fixed it. Also to be consistent, moved
other signal handlers' registration as well before we set pid.

Please find v5 addressing above comments.

thanks
Shveta