Re: promotion related handling in pg_sync_replication_slots()

On Tue, Apr 23, 2024 at 12:37 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Mon, Apr 22, 2024 at 7:04 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > On Mon, Apr 22, 2024 at 9:02 PM shveta malik <shveta.malik@gmail.com> wrote:
> > >
> > > Thanks for the patch, the changes look good Amit. Please find the merged patch.
> > >
> >
> > I've reviewed the patch and have some comments:
> >
> > ---
> >     /*
> > -    * Early initialization.
> > +    * Register slotsync_worker_onexit() before we register
> > +    * ReplicationSlotShmemExit() in BaseInit(), to ensure that during the
> > +    * exit of the slot sync worker, ReplicationSlotShmemExit() is called
> > +    * first, followed by slotsync_worker_onexit(). The startup process during
> > +    * promotion invokes ShutDownSlotSync() which waits for slot sync to
> > +    * finish and it does that by checking the 'syncing' flag. Thus worker
> > +    * must be done with the slots' release and cleanup before it marks itself
> > +    * as finished syncing.
> >      */
> >
> > I'm slightly worried that we register the slotsync_worker_onexit()
> > callback before BaseInit(), because it could be a blocker when we want
> > to add more work in the callback, for example sending the stats.
> >
>
> The other possibility is that we do slot release/clean up in the
> slotsync_worker_onexit() call itself and then we can do it after
> BaseInit().

This approach sounds clearer and safer to me. The current approach
relies on the callback registration order of
ReplicationSlotShmemExit(). If it changes in the future, we will
silently have the same problem. Every slot sync related work should be
done before allowing someone to touch synced slots by clearing the
'syncing' flag.

>
> > ---
> >         synchronize_slots(wrconn);
> > +
> > +       /* Cleanup the temporary slots */
> > +       ReplicationSlotCleanup();
> > +
> > +       /* We are done with sync, so reset sync flag */
> > +       reset_syncing_flag();
> >
> > I think it ends up removing other temp slots that are created by the
> > same backend process using
> > pg_create_{physical,logical_replication_slots() function, which could
> > be a large side effect of this function for users.
> >
>
> True, I think here we should either remove only temporary and synced
> marked slots. The other possibility is to create slots as RS_EPHEMERAL
> initially when called from the SQL function but that doesn't sound
> like a neat approach.
>
> >
>  Also, if users want
> > to have a process periodically calling pg_sync_replication_slots()
> > instead of the slotsync worker, it doesn't support a case where we
> > create a temp not-ready slot and turn it into a persistent slot if
> > it's ready for sync.
> >
>
> True, but eventually the API should be able to directly create the
> persistent slots and anyway this can happen only for the first time
> (till the slots are created and marked persistent) and one who wants
> to use this function periodically should be able to see regular syncs.

I agree that we remove temp-and-synced slots created via the API at
the end of the API . We end up creating and dropping slots in every
API call but since the pg_sync_replication_slots() function is a kind
of debug-purpose function and it will not be common to call this
function regularly instead of using the slot sync worker, we can live
with such overhead.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com