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

On Thu, Jul 17, 2025 at 9:34 AM shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
>
> On Wed, Jul 16, 2025 at 3:47 PM Ajin Cherian <itsajin(at)gmail(dot)com> wrote:
> >
> > > I am not able to apply the patch to the latest head or even to a week
> > > back version. Can you please check and rebase?
> > >
> > > thanks
> > > Shveta
> >
> > Rebased.
> >
>
> Thanks. Please find a few comments:
>
>
> 1)
> /* Any slot with NULL in these fields should not have made it this far */
>
> It is good to get rid of the case where we had checks for NULL
> confirmed_lsn and catalog_xmin (i.e. when slot was in RS_EPHEMERAL
> state), as that has already been checked by synchronize_slots() and
> such a slot will not even reach wait_for_primary_slot_catchup(). But a
> slot can still be invalidated on primary anytime, and thus during this
> wait, we should check for primary's invalidation as we were doing in
> v1.
>
> 2)
> + * If in SQL API synchronization, and we've been promoted, then no point
>
> extra space before promoted.
>
> 3)
>
> + if (!AmLogicalSlotSyncWorkerProcess() && PromoteIsTriggered())
>
> We don't need 'AmLogicalSlotSyncWorkerProcess' as that is already
> checked at the beginning of this function.
>
> 4)
> + ereport(WARNING,
> + errmsg("aborting sync for slot \"%s\"",
> + remote_slot->name),
> + errdetail("Promotion occurred before this slot was fully"
> + " synchronized."));
> + pfree(cmd.data);
> +
> + return false;
>
> a) Please add an error-code.
>
> b) Shall we change msg to
>
> errmsg("aborting sync for slot \"%s\"",
> remote_slot->name),
> errhint("%s cannot be executed once promotion is
> triggered.",
>
> "pg_sync_replication_slots()")));
>
>
>
> 5)
> Instead of using PromoteIsTriggered, shall we rely on
> 'SlotSyncCtx->stopSignaled' as we do when we start this API.
>
> 6)
> In logicaldecoding.sgml, we can get rid of "Additionally, enabling
> sync_replication_slots on the standby is required" to make it same as
> what we had prior to the patch I pointed earlier.
>
> Or better we can refine it to below. Thoughts?
>
> The logical replication slots on the primary can be enabled for
> synchronization to the hot standby by using the failover parameter of
> pg_create_logical_replication_slot, or by using the failover option of
> CREATE SUBSCRIPTION during slot creation. After that, synchronization
> can be performed either manually by calling pg_sync_replication_slots
> on the standby, or automatically by enabling sync_replication_slots on
> the standby. When sync_replication_slots is enabled, the failover
> slots are periodically synchronized by the slot sync worker. For the
> synchronization to work, .....

I am wondering if we should provide an optional parameter to
pg_sync_replication_slots(), to control whether to wait for the slot
to be synced or just return with ERROR as it is doing now, default can
be wait.

--
Regards,
Dilip Kumar
Google