On Mon, Apr 15, 2024 at 6:21 PM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:
>
> On Mon, Apr 15, 2024 at 03:38:53PM +0530, shveta malik wrote:
> > On Mon, Apr 15, 2024 at 2:29 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > On Fri, Apr 12, 2024 at 5:25 PM shveta malik <shveta.malik@gmail.com> wrote:
> > > >
> > > > Please find v3 addressing race-condition and one other comment.
> > > >
> > > > Up-thread it was suggested that, probably, checking
> > > > SlotSyncCtx->syncing should be sufficient in ShutDownSlotSync(). On
> > > > re-thinking, it might not be. Slot sync worker sets and resets
> > > > 'syncing' with each sync-cycle, and thus we need to rely on worker's
> > > > pid in ShutDownSlotSync(), as there could be a window where promotion
> > > > is triggered and 'syncing' is not set for worker, while the worker is
> > > > still running. This implementation of setting and resetting syncing
> > > > with each sync-cycle looks better as compared to setting syncing
> > > > during the entire life-cycle of the worker. So, I did not change it.
> > > >
> > >
> > > To retain this we need to have different handling for 'syncing' for
> > > workers and function which seems like more maintenance burden than the
> > > value it provides. Moreover, in SyncReplicationSlots(), we are calling
> > > a function after acquiring spinlock which is not our usual coding
> > > practice.
> >
> > Okay. Changed it to consistent handling.
>
> Thanks for the patch!
>
> > Now both worker and SQL
> > function set 'syncing' when they start and reset it when they exit.
>
> It means that it's not possible anymore to trigger a manual sync if
> sync_replication_slots is on. Indeed that would trigger:
>
> postgres=# select pg_sync_replication_slots();
> ERROR: cannot synchronize replication slots concurrently
>
> That looks like an issue to me, thoughts?
>
This is intentional as of now for the sake of keeping
implementation/code simple. It is not difficult to allow them but I am
not sure whether we want to add another set of conditions allowing
them in parallel. And that too in an unpredictable way as the API will
work only for the time slot sync worker is not performing the sync.
--
With Regards,
Amit Kapila.
