Re: promotion related handling in pg_sync_replication_slots()

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:
>
> ---
>     /*
> -    * 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?

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

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.

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

True, but eventually the API should be able to directly create the
persistent slots and anyway this can happen only for the first time
(till the slots are created and marked persistent) and one who wants
to use this function periodically should be able to see regular syncs.
OTOH, leaving temp slots created via this API could remain as-is after
promotion and we need to document for users to remove such slots. Now,
we can do that if we want but I think it is better to clean up such
slots rather than putting the onus on users to remove them after
promotion.

--
With Regards,
Amit Kapila.