Re: Using failover slots for PG-non_PG logical replication

On Mon, Jul 7, 2025 at 7:00 PM Ashutosh Bapat
<ashutosh(dot)bapat(dot)oss(at)gmail(dot)com> wrote:
>
> On Mon, Jul 7, 2025 at 9:53 AM shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
> >
> > Thanks for the patch.
> >
> > > I couldn't figure out whether the query on primary to fetch all the
> > > slots to be synchronized should filter based on invalidation_reason
> > > and conflicting or not. According to synchronize_slots(), it seems
> > > that we retain invalidated slots on standby when failover = true and
> > > they would remain with synced = true on standby. Is that right?
> > >
> >
> > Yes, that’s correct. We sync the invalidation status of replication
> > slots from the primary to the standby and then stop synchronizing any
> > slots that have been marked as invalidated, retaining synced flag as
> > true. IMO, there's no need to filter out conflicting slots on the
> > primary, because instead of excluding them there and showing
> > everything as failover-ready on the standby, the correct approach is
> > to reflect the actual state on standby.This means conflicting slots
> > will appear as non-failover-ready on the standby. That’s why Step 3
> > also considers conflicting flag in its evaluation.
>
> Thanks for the explanation. WFM.
>
> If a slot is invalidated because RS_INVAL_WAL_REMOVED, conflicting =
> false, but the slot is not useful standby. But then it's not useful on
> primary as well. Is that why we are not including (invalidation_reason
> IS NOT NULL) condition along in (synced AND NOT temporary AND NOT
> conflicting)?

Thanks for bringing it up. Even if the slot is not useful on the
primary node as well, we shall still show failover-ready as false on
standby. We should modify the query of step3 to check
'invalidation_reason IS NULL' instead of 'NOT conflicting'. That will
cover all the cases where the slot is invalidated and thus not
failover ready.

> >
> > 1)
> > +/* primary # */ SELECT array_agg(quote_literal(r.slot_name)) AS slots
> > + FROM pg_replication_slots r
> > + WHERE r.failover AND NOT r.temporary;
> >
> > On primary, there is no need to check temporary-status. We do not
> > allow setting failover as true for temporary slots.
>
> Why does synchronize_slots() has it in its query?
>

It is not needed but no harm in maintaining it.
If we want documents to be in sync with code, we can have 'not
temporary' check in doc as well.

> >
> > 2)
> > Although not directly related to the concerns addressed in the given
> > patch, I think it would be helpful to add a note in the original doc
> > stating that Steps 1 and 2 should be executed on each subscriber node
> > that will be served by the standby after failover.
>
> There's a bit of semantic repeatition here since an earlier paragraph
> mentions a given subscriber. But I think overall it's better to be
> more clear than being less clear.
> >
> > I have attached a top-up patch with the above changes and a few more
> > trivial changes. Please include it if you find it okay.
>
> Thanks. Included. I have made a few edits and included them in the
> attached patch.
>

Thanks. The existing changes (originally targeted in this patch) look
good to me.

I have attached a top-up patch for step-3 correction. Please include
if you find it okay to be fixed in the same patch, otherwise we can
handle it separately.

thanks
Shveta