Re: promotion related handling in pg_sync_replication_slots()

On Mon, Apr 22, 2024 at 9:02 PM shveta malik <shveta.malik@gmail.com> wrote:
>
> On Mon, Apr 22, 2024 at 5:10 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Fri, Apr 19, 2024 at 1:52 PM shveta malik <shveta.malik@gmail.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