Re: POC: enable logical decoding when wal_level = 'replica' without a server restart

On Tue, Nov 11, 2025 at 6:05 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
> On Mon, Nov 10, 2025 at 8:05 PM shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
> >
> > On Thu, Nov 6, 2025 at 4:32 AM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> > >
> > >
> > > I've updated and rebased the patch.
> > >
> >
> > Thanks for the patch. Please find a few comments:
> >
> >
> > 1)
> > ReplicationSlotsDropDBSlots:
> >
> > + SpinLockAcquire(&s->mutex);
> > + invalidated = s->data.invalidated == RS_INVAL_NONE;
> > + SpinLockRelease(&s->mutex);
> > +
> > + /*
> > + * Count slots on other databases too so we can disable logical
> > + * decoding only if no slots in the cluster.
> > + */
> > + if (invalidated)
> > + n_valid_logicalslots++;
> >
> >
> > This seems confusing to me. Can we instead do:
> >
> > SpinLockAcquire(&s->mutex);
> > if (s->data.invalidated == RS_INVAL_NONE)
> > n_valid_logicalslots++;
> > SpinLockRelease(&s->mutex);
> >
> > 2)
> > InvalidateObsoleteReplicationSlots:
> >
> > + bool islogical = SlotIsLogical(s);
> >
> > /* Prevent invalidation of logical slots during binary upgrade */
> > if (SlotIsLogical(s) && IsBinaryUpgrade)
> > + {
> > + SpinLockAcquire(&s->mutex);
> > + if (s->data.invalidated == RS_INVAL_NONE)
> > + n_valid_logicalslots++;
> > + SpinLockRelease(&s->mutex);
> > +
> > continue;
> > + }
> >
> > We should use 'islogical' instead of SlotIsLogical here.
> >
> > 3)
> > InvalidateObsoleteReplicationSlots() is more robust now as we are
> > using both 'invalidated' and 'released_lock' flags but still nowhere
> > we guarantee that invalidated=true implies released_lock=true. Since
> > we jump to 'restart' label only if released_lock is true, it becomes
> > important to have an ASSERT which says invalidated=true implicitly
> > means released_lock=true or vice versa. Because at the end we go by
> > 'invalidated_logical' rather than 'released_lock' to decide about
> > logical-decoding disabling.
> >
> > In this logic:
> >
> > + if (InvalidatePossiblyObsoleteSlot(possible_causes, s, oldestLSN,
> > + dboid, snapshotConflictHorizon,
> > + &released_lock))
> > {
> > - /* if the lock was released, start from scratch */
> > - goto restart;
> > + /* Remember we have invalidated a physical or logical slot */
> > + invalidated = true;
> > +
> > + /*
> > + * Additionally, remember we have invalidated a logical slot too
> > + * as we can request disabling logical decoding later.
> > + */
> > + if (islogical)
> > + invalidated_logical = true;
> > }
> >
> > Shall we have an Assert(released_lock) if
> > InvalidatePossiblyObsoleteSlot returns true. Or any better way?
> >
> > 4)
> > + SpinLockAcquire(&s->mutex);
> > + if (s->data.invalidated == RS_INVAL_NONE)
> > + n_valid_logicalslots++;
> >
> > In the same function, isn't the above code problematic: Don't we need
> > 'islogical' check before incrementing 'n_valid_logicalslots',
> > otherwise it may wrongly count valid physical slots as well.
>
> Agreed with all the above points. Will fix and update the updated version.
>

I've attached the updated version patch. I addressed all comments I
got so far, and made some cosmetic changes.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com