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

On Fri, Nov 21, 2025 at 8:10 PM shveta malik <shveta.malik@gmail.com> wrote:
>
> On Fri, Nov 21, 2025 at 9:14 AM Ajin Cherian <itsajin@gmail.com> wrote:
> >
> >
> > Attaching patch v24, addressing the above comments.
> >
>
> Thanks for the patch. Please find a few comments:
>
>
> 1)
> Instead of passing an argument to slotsync_reread_config and
> ProcessSlotSyncInterrupts, we can use 'AmLogicalSlotSyncWorkerProcess'
> to distinguish the worker and API.
>

Changed as such.

> 2)
> Also, since we are not using a separate memory context, we don't need
> to make structure 'SlotSyncApiFailureParams' for slot_names failure.
> slot_names will be freed with the memory-context itself when
> exec_simple_query finishes.
>

Removed.

> 3)
> - if (old_sync_replication_slots != sync_replication_slots)
> + /* Worker-specific check for sync_replication_slots change */
> + if (!from_api && old_sync_replication_slots != sync_replication_slots)
>   {
>   ereport(LOG,
> - /* translator: %s is a GUC variable name */
> - errmsg("replication slot synchronization worker will shut down
> because \"%s\" is disabled", "sync_replication_slots"));
> + /* translator: %s is a GUC variable name */
> + errmsg("replication slot synchronization worker will shut down
> because \"%s\" is disabled",
> +    "sync_replication_slots"));
>   proc_exit(0);
>   }
>
> Here, we need not to have different flow for api and worker. Both can
> quit sync when this parameter is changed. The idea is if someone
> enables 'sync_replication_slots' when API is working, that means we
> need to start slot-sync worker, so it is okay if the API notices this
> and exits too.
>

changed but used a different error message.

> 4)
> + if (from_api)
> + elevel = ERROR;
> + else
> + elevel = LOG;
>
> - proc_exit(0);
> + ereport(elevel,
> + errmsg("replication slot synchronization will stop because of a
> parameter change"));
> +
>
> We can do:
> ereport(AmLogicalSlotSyncWorkerProcess() ? LOG : ERROR, ...);
>

changed as such.

> 5)
>  SlotSyncCtx->syncing = true;
>
> + /* The worker pid must not be already assigned in SlotSyncCtx */
> + Assert(SlotSyncCtx->pid == InvalidPid);
> +
>
> We can shift Assert before we set the shared-memory flag 'SlotSyncCtx->syncing'.
>

Done.

Attaching patch v25 addressing the above comments.

regards,
Ajin Cherian
Fujitsu Australia