Re: Use SIGTERM instead of SIGUSR1 for slotsync worker to exit during promotion?

On Wed, Apr 1, 2026 at 3:19 PM Fujii Masao <masao.fujii@gmail.com> wrote:
>
> On Wed, Apr 1, 2026 at 2:34 PM Nisha Moond <nisha.moond412@gmail.com> wrote:
> >
> > On Tue, Mar 31, 2026 at 3:56 PM Zhijie Hou (Fujitsu)
> > <houzj.fnst@fujitsu.com> wrote:
> > >
> > > On Tuesday, March 31, 2026 2:02 PM Nisha Moond <nisha.moond412@gmail.com> wrote:
> > > >
> > > >
> > > > Please find the updated patch (v6) attached.
> > >
> > > Thanks for updating the patch. One minor comment:
> > >
> > > I think we could avoid interrupting and reporting an ERROR when
> > > IsSyncingReplicationSlots() returns false to avoid reporting ERROR unnecessarily
> > > when the slotsync has already finished.
> > >
> >
> > Thanks for the review. Fixed above in v7.
>
> Thanks for updating the patch! It looks good to me, with just a few minor points
> . If those are addressed, I'd like to push it.
>
> + * a new worker (or a new API call) that starts after the old worker was
>
> "API" feels a bit vague. It might be clearer to explicitly say
> "pg_sync_replication_slots()".
>

Done.

> + PROCSIG_SLOTSYNC_MESSAGE, /* ask slotsync worker/API to stop */
>
> "API" here also feels a bit vague. So I'd like to use "ask slot synchronization
> to stop" as the comment, instead.

Done.

> + * We cannot rely solely on 'stopSignaled' here because:
> + * 1) It resides in shared memory and is visible to all processes, so checking
> + *    it directly in ProcessInterrupts() would require additional checks to
> + *    ensure only the synchronizing process acts on it.
> + * 2) It has different lifetime semantics and cannot be reset after handling,
> + *    as it also guards against postmaster and promotion race conditions.
> + * 3) Accessing it requires acquiring a spinlock, which can be too expensive
> + *    or undesirable for every ProcessInterrupts() call.
>
> Now that PROCSIG_SLOTSYNC_MESSAGE is in place, using SlotSyncShutdownPending
> is intuitive. So it seems more useful to explain why stopSignaled is still
> needed rather than why SlotSyncShutdownPending is used (i.e., why stopSignaled
> is not sufficient). Since that rationale is already covered in the SlotSyncCtx
> comments, I'd suggest removing this comment block.

Okay, done.

>
> As for backpatching, this looks like it should go back to v17, where slotsync
> was introduced. Thought?

Right, the issue exists in v17 as well.

Attached the updated patch.

--
Thanks,
Nisha