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

On Fri, Dec 5, 2025 at 12:23 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Thu, Dec 4, 2025 at 1:30 AM shveta malik <shveta.malik@gmail.com> wrote:
> >
> > On Thu, Dec 4, 2025 at 1:54 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > >
> > >
> > > I've attached the updated patch that incorporated the review comments
> > > and is rebased to the current HEAD.
> > >
> >
> > Thanks, a few things:
> >
> >
> > 1)
> > +        The system automatically increases the
> > +        effective WAL level to <literal>logical</literal> when
> > creating the first
> > +        logical replication slot, and decreases it back to
> > <literal>replica</literal>
> > +        when dropping the last logical replication slot. The current
> > effective WAL
> > +        level can be monitored through <xref
> > linkend="guc-effective-wal-level"/>
> > +        parameter.
> >
> > Shouldn't we mention about invalidation of slot along with dropping of
> > slot? I have suggested this earlier but I think it got missed to be
> > addressed.
>
> Fixed. Sorry I missed it.
>
> >
> > 2)
> > + else if (sync_replication_slots)
> > + {
> > + /*
> > + * Signal the postmaster to launch the slotsync worker.
> > + *
> > + * XXX: For simplicity, we keep the slotsync worker running
> > + * even after logical decoding is disabled. A future
> > + * improvement can consider starting and stopping the worker
> > + * based on logical decoding status change.
> > + */
> > + kill(PostmasterPid, SIGUSR1);
> > + }
> > + }
> > +
> > + /* Update the status on shared memory */
> > + UpdateLogicalDecodingStatus(logical_decoding, true);
> >
> > I see that we have moved 'Update' post slotsync's start attempt. This
> > leaves a possibility that that slot-sync is started sooner than last
> > 'Update' call and thus slotsync may exit with:
> > replication slot synchronization requires "effective_wal_level" >=
> > "logical" on the primary
> >
> > I see that update was prior to the slotsync step in earlier patches.
> > Why have we moved it to a later stage?
>
> You're right. I've reconsidered the operation order and reverted the
> previous change. On further thought, I think it should not happen to
> decode the STATUS_CHANGE record because when applying the
> STATUS_CHANGE record, we disable logical decoding first and then
> invalidate the slots. There is no window where a logical slot can
> creep in on the standby, and on the primary it cannot restart after
> changing wal_level < replica if there is a pre-existing slot. If my
> understanding is right, we should put a sanity check in xlog_decode().
>

I agree. But can we please update comments to bring more clarity:

Current:
+ * Logical decoding is disabled and then existing logical
+ * slots on the standby get invalidated when this WAL record
+ * is replayed. No logical slot can creep in while the logical
+ * decoding status is being disabled. This cannot occur even
+ * on the primary either, as the primary would not allow to
+ * restart after changing wal_level < replica if there is
+ * pre-existing logical slot.

Suggestion:
Logical decoding is disabled, and existing logical slots on the
standby are invalidated when this WAL record is replayed. No logical
decoder can process this WAL record until replay completes, and by
then the slots are already
invalidated. Furthermore, no new logical slots can be created while
logical decoding is disabled. This cannot occur even on primary
either, since it will not restart with wal_level < replica if any
logical slots exist.

thanks
Shveta