On Tue, Apr 23, 2024 at 9:07 AM Amit Kapila <amit.kapil...@gmail.com> wrote: > > On Mon, Apr 22, 2024 at 7:04 PM Masahiko Sawada <sawada.m...@gmail.com> wrote: > > > > On Mon, Apr 22, 2024 at 9:02 PM shveta malik <shveta.ma...@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
v11-0001-Handle-stopSignaled-during-sync-function-call.patch
Description: Binary data