On Tue, Oct 28, 2025 at 1:55 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Tue, Oct 28, 2025 at 8:59 AM shveta malik <shveta.malik@gmail.com> wrote:
> >
> > On Mon, Oct 27, 2025 at 11:56 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > >
> > > On Mon, Oct 27, 2025 at 8:08 AM shveta malik <shveta.malik@gmail.com> wrote:
> > > >
> > > > On Mon, Oct 27, 2025 at 6:12 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > > > >
> > > > >
> > > > > Thanks for looking into it. I didn't get a chance to review the entire
> > > > > 0002 but I looked at InvalidateObsoleteReplicationSlots() and have a
> > > > > few questions related to that.
> > > > >
> > > > > In InvalidateObsoleteReplicationSlots(), the patch increments
> > > > > n_valid_logicalslots before trying to invalidate the slot. Say, if
> > > > > there is just one logical slot which got invalidated, then because we
> > > > > have first incremented n_valid_logicalslots, how will it request to
> > > > > disable logical_decoding after invalidating the last logical slot?
> > > > >
> > > >
> > > > My initial understand on this:
> > > >
> > > > Whenever it invalidates a slot, the code jumps to the restart label,
> > > > which in turn sets n_valid_logicalslots to 0. If it does not
> > > > invalidate the slot but a logical slot exists, then
> > > > n_valid_logicalslots remains greater than 0. Therefore, by the end of
> > > > the function:
> > > >
> > > > --If valid logical slots were found and all were invalidated,
> > > > n_valid_logicalslots must be 0.
> > > > --If a valid logical slot was found but was not invalidated,
> > > > n_valid_logicalslots must be greater than 0.
> > > >
> > > > But on looking again, I found that the code jumps to restart-label if
> > > > lock-was released in interim. So can it happen that 'invalidated' is
> > > > true but lock was not released by InvalidatePossiblyObsoleteSlot()
> > > > causing n_valid_logicalslots to be greater than 0 even when the slot
> > > > was actually invalidated?
> > >
> > > IIUC it cannot happen. When we invalidate one logical slot we release
> > > the lwlock to avoid holding a lwlock while ReplicationSlotSave(). It's
> > > not necessarily true that if we release the lwlock we invalidate the
> > > slot but the opposite is always true.
> > >
> >
> > Okay. Thanks for confirming. After looking more into the flow, I agree on this.
> >
>
> But, is it a good idea to rely on such a flag? I feel there should be
> some other bullet-proof way to detect whether slot is invalidated so
> that even if this internal API changes in future, we won't go wrong.
> In any case, if we still want to go the same way at least a comment
> explaining this rationale would be required.
Good point. I'll refactor these functions to make it more bullet-proof.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
