Re: Improve pg_sync_replication_slots() to wait for primary to advance

On Mon, Sep 22, 2025 at 4:21 PM Ajin Cherian <itsajin(at)gmail(dot)com> wrote:
>
>
> Created a patch v13 with these changes.
>

Please find a few comments:

1)
+ /* update the failure structure so that it can be freed on error */
+ fparams.slot_names = slot_names;
+

Since slot_names is assigned only once, we can make the above
assignment as well only once, inside the if-block where we initialize
slot_names.

2)
extract_slot_names():

+ foreach(lc, remote_slots)
+ {
+ RemoteSlot *remote_slot = (RemoteSlot *) lfirst(lc);
+ char *slot_name;
+
+ /* Switch to long-lived TopMemoryContext to store slot names */
+ oldcontext = MemoryContextSwitchTo(TopMemoryContext);
+
+ slot_name = pstrdup(remote_slot->name);
+ slot_names = lappend(slot_names, slot_name);
+
+ MemoryContextSwitchTo(oldcontext);
+ }

It will be better to move 'MemoryContextSwitchTo' calls outside of the
loop. No need to switch the context for each slot.

3)
ProcessSlotSyncAPIChanges() gives a feeling that it is actually
processing API changes where instead it is processing interrupts or
config changes. Can we please rename to ProcessSlotSyncAPIInterrupts()

4)
I prefer version 11's slotsync_api_reread_config() over current
slotsync_api_config_changed(). There, even error was taken care of
inside the function, which to me looked better and similar to how
slotsync worker deals with it.

I have made some comment changes, attached the patch. Please include
it if you find it okay.

thanks
Shveta