Re: promotion related handling in pg_sync_replication_slots()

On Tue, Apr 16, 2024 at 9:27 AM Zhijie Hou (Fujitsu)
<houzj(dot)fnst(at)fujitsu(dot)com> wrote:
>
> On Monday, April 15, 2024 6:09 PM shveta malik <shveta(dot)malik(at)gmail(dot)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