Re: promotion related handling in pg_sync_replication_slots()

Hi,

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(dot)kapila16(at)gmail(dot)com> wrote:
> >
> > On Fri, Apr 12, 2024 at 5:25 PM shveta malik <shveta(dot)malik(at)gmail(dot)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?

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com