Re: promotion related handling in pg_sync_replication_slots()

On Tue, Apr 23, 2024 at 9:07 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Mon, Apr 22, 2024 at 7:04 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> >
> > On Mon, Apr 22, 2024 at 9:02 PM shveta malik <shveta(dot)malik(at)gmail(dot)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