On Tue, Sep 23, 2025 at 2:59 PM shveta malik <shveta.malik@gmail.com> wrote:
>
> On Mon, Sep 22, 2025 at 4:21 PM Ajin Cherian <itsajin@gmail.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.
>
Changed.
> 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.
>
Changed.
> 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()
>
Changed.
> 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.
>
Changed.
> I have made some comment changes, attached the patch. Please include
> it if you find it okay.
Incorporated.
On Tue, Sep 23, 2025 at 4:42 PM shveta malik <shveta.malik@gmail.com> wrote:
>
>
> 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'
>
Changed.
> 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.'
>
Changed.
> 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'
>
Changed.
Attaching patch v14 incorporating the above changes.
regards,
Ajin Cherian
Fujitsu Australia
