Re: promotion related handling in pg_sync_replication_slots()

On Mon, Apr 22, 2024 at 9:02 PM shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
>
> On Mon, Apr 22, 2024 at 5:10 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >
> > On Fri, Apr 19, 2024 at 1:52 PM shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
> > >
> > > Please find v9 with the above comments addressed.
> > >
> >
> > I have made minor modifications in the comments and a function name.
> > Please see the attached top-up patch. Apart from this, the patch looks
> > good to me.
>
> 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.

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

Regards,

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