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

On Tue, Sep 23, 2025 at 10:29 AM shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
>
> 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.
>

Tested the patch, few more suggestions

5)
Currently the error message is:

postgres=# SELECT pg_sync_replication_slots();
ERROR: cannot continue slot synchronization due to parameter changes
DETAIL: Critical replication parameters (primary_conninfo,
primary_slot_name, or hot_standby_feedback) have changed since
pg_sync_replication_slots() started.
HINT: Retry pg_sync_replication_slots() to use the updated configuration.

a)
To be consistent with other error-messages, can we change ERROR msg
to: 'cannot continue replication slots synchronization due to
parameter changes'

b)
There is double space in DETAIL msg: "have changed since"

Will it be better to shorten the DETAIL as: 'One or more of
primary_conninfo, primary_slot_name, or hot_standby_feedback were
modified.'

6)
postgres=# SELECT pg_sync_replication_slots();
ERROR: exiting from slot synchronization as promotion is triggered

Shall we rephrase it similar to the previous message: 'cannot continue
replication slots synchronization as standby promotion is triggered'

thanks
Shveta