Re: POC: enable logical decoding when wal_level = 'replica' without a server restart - Mailing list pgsql-hackers

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:
> > >
> > > On Sat, Oct 25, 2025 at 4:06 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > > >
> > > > On Fri, Oct 24, 2025 at 4:48 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > > > >
> > > > > 5.
> > > > > +bool
> > > > > +CheckLogicalSlotExists(void)
> > > > > {
> > > > > …
> > > > > + /* NB: counting invalidated slots */
> > > > > +
> > > > > + if (SlotIsLogical(s))
> > > > >
> > > > > Why can't we avoid counting invalid slots? I think this needs more
> > > > > comments. BTW, shouldn't this patch consider changing
> > > > > effective_wal_level when the last logical slot is invalidated?
> > > > > Ideally, when logical decoding is not possible in the system, then we
> > > > > can reduce the wal_level back to replica, no?
> > > >
> > > > Hmm, good point. I've considered this idea before but I didn't
> > > > implement it probably because it makes the code more complex. But
> > > > thinking of this idare carefully, it doesn't seem too complex. I've
> > > > implemented this part as a separate patch to make reviews easy. I'll
> > > > merge them if it looks good.
> > > >
> > >
> > > 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.

> >
> > > Another related point is that, say we decide to disable decoding
> > > because the last logical slot got invalidated and
> > > RequestDisableLogicalDecoding()->LogicalDecodingStatusChangeAllowed()
> > > returns false, then how the disabling will happen?
> > >
> >
> > If LogicalDecodingStatusChangeAllowed() returns false, we do not
> > disable logical decoding because that essentially means
> > recovery-in-progress and if that is the case, we do not allow
> > status-change. LogicalDecodingStatusChangeAllowed() returns false
> > always on standby until it is promoted and recovery ends.
>
> Exactly.
>
> Regards,
>
> --
> Masahiko Sawada
> Amazon Web Services: https://aws.amazon.com