On Mon, Oct 27, 2025 at 8:08 AM shveta malik <[email protected]> wrote: > > On Mon, Oct 27, 2025 at 6:12 PM Amit Kapila <[email protected]> wrote: > > > > On Sat, Oct 25, 2025 at 4:06 AM Masahiko Sawada <[email protected]> > > wrote: > > > > > > On Fri, Oct 24, 2025 at 4:48 AM Amit Kapila <[email protected]> > > > 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. > > > 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
