Re: Improve pg_sync_replication_slots() to wait for primary to advance - Mailing list pgsql-hackers

On Wed, Sep 3, 2025 at 11:58 AM Ajin Cherian <itsajin@gmail.com> wrote:
>
>
> On Fri, Aug 29, 2025 at 6:50 PM Ashutosh Bapat
> <ashutosh.bapat.oss@gmail.com> wrote:
> >
> > On Fri, Aug 29, 2025 at 11:42 AM Ajin Cherian <itsajin@gmail.com> wrote:
>
> > +/*
> > + * Flag used by pg_sync_replication_slots()
> > + * to do retries if the slot did not persist while syncing.
> > + */
> > +static bool slot_persistence_pending = false;
> >
> > I don't think we need to keep a global variable for this. The variable
> > is used only inside SyncReplicationSlots() and the call depth is not
> > more than a few calls. From synchronize_slots(), before which the
> > variable is reset and after which the variable is checked, to
> > update_and_persist_local_synced_slot() which sets the variable, all
> > the functions return bool. All of them can be made to return an
> > integer status instead indicating the result of the operation. If we
> > do so we could check the return value of synchronize_slots() to decide
> > whether to retry or not, isntead of maintaining a global variable
> > which has a much wider scope than required. It's difficult to keep it
> > updated over the time.
> >
>
> The problem is that all those calls synchronize_slots() and
> update_and_persist_local_synced_slot() are shared with the slotsync
> worker logic and API. Hence, changing this will affect slotsync_worker
> logic as well. While the API needs to spefically retry only if the
> initial sync fails, the slotsync worker will always be retrying. I
> feel using a global variable is a more convenient way of doing this.

AFAICS, it's a matter of expanding the scope of what's returned by
those functions. The worker may not want to use the whole expanded
scope but the API will use it. That shouldn't change the functionality
of the worker, but it will help avoid the global variable - which have
much wider scope and their maintenance can be prone to bugs.

>
> > @@ -1276,7 +1331,7 @@ wait_for_slot_activity(bool some_slot_updated)
> >
> > The function is too cute to be useful. The code should be part of
> > ReplSlotSyncWorkerMain() just like other worker's main functions.
> >
>
> But this wouldn't be part of this feature.
>
> > void
> > SyncReplicationSlots(WalReceiverConn *wrconn)
> > {
> > PG_ENSURE_ERROR_CLEANUP(slotsync_failure_callback, PointerGetDatum(wrconn));
> > {
> >
> > Shouldn't this function call CheckForInterrupts() somewhere in the
> > loop since it could be potentially an infinite loop?
>
> I've tested this and I see that interrupts are being handled by
> sending SIGQUIT and SIGINT to the backend process.

Can you please point me to the code (the call to
CHECK_FOR_INTERRUPTS()) which processes these interrupts while
pg_sync_replication_slots() is executing, especially when the function
is waiting while syncing a slot.

--
Best Wishes,
Ashutosh Bapat