Re: promotion related handling in pg_sync_replication_slots()

On Tue, Apr 23, 2024 at 9:07 AM 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:

Thanks for the 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(). Do you have any other/better idea for this?

I have currently implemented it this way in v11.

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

Yes, this is a problem. Thanks for catching it.

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

Modified the logic to remove only synced temporary slots during
SQL-function exit.

Please find v11 with above changes.

thanks
Shveta