Re: Use SIGTERM instead of SIGUSR1 for slotsync worker to exit during promotion? - Mailing list pgsql-hackers

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()".

+ 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.

+ * 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.


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

Regards,

--
Fujii Masao