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

On Mon, Sep 8, 2025 at 11:34 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Mon, Sep 8, 2025 at 11:14 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> >
> > On Fri, Sep 5, 2025 at 8:50 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > >
> > > On Thu, Sep 4, 2025 at 1:24 AM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> > > > >
> > > > > 2.
> > > > > - /*
> > > > > - * Invalidate logical slots if we are in hot standby and the primary
> > > > > - * does not have a WAL level sufficient for logical decoding. No need
> > > > > - * to search for potentially conflicting logically slots if standby is
> > > > > - * running with wal_level lower than logical, because in that case, we
> > > > > - * would have either disallowed creation of logical slots or
> > > > > - * invalidated existing ones.
> > > > > - */
> > > > > - if (InRecovery && InHotStandby &&
> > > > > - xlrec.wal_level < WAL_LEVEL_LOGICAL &&
> > > > > - wal_level >= WAL_LEVEL_LOGICAL)
> > > > > - InvalidateObsoleteReplicationSlots(RS_INVAL_WAL_LEVEL,
> > > > > - 0, InvalidOid,
> > > > > - InvalidTransactionId);
> > > > > -
> > > > > LWLockAcquire(ControlFileLock, LW_EXCLUSIVE);
> > > > > ControlFile->MaxConnections = xlrec.MaxConnections;
> > > > > ControlFile->max_worker_processes = xlrec.max_worker_processes;
> > > > > @@ -8605,6 +8643,50 @@ xlog_redo(XLogReaderState *record)
> > > > > {
> > > > > /* nothing to do here, just for informational purposes */
> > > > > }
> > > > > + else if (info == XLOG_LOGICAL_DECODING_STATUS_CHANGE)
> > > > > + {
> > > > > + bool logical_decoding;
> > > > > +
> > > > > + /* Update the status on shared memory */
> > > > > + memcpy(&logical_decoding, XLogRecGetData(record), sizeof(bool));
> > > > > + UpdateLogicalDecodingStatus(logical_decoding, true);
> > > > > +
> > > > > + if (InRecovery && InHotStandby)
> > > > > + {
> > > > > + if (!logical_decoding)
> > > > >
> > > > > Like previously, shouldn't we have a check for standby's wal_level as
> > > > > well due to the reasons mentioned in the removed comments?
> > > >
> > > > IIUC we need to replay the STATUS_CHANGE record when wal_level is set
> > > > to 'replica' or 'logical'. If we want to add a check for standby's
> > > > wal_level, the check would be "wal_level >= WAL_LEVEL_REPLICA" but it
> > > > would be redundant as we already checked "InRecovery && InHotStandby".
> > > >
> > >
> > > If we want to mimic the current implementation, won't
> > > effective_wal_level be 'logical' even on standby? Otherwise, there
> > > shouldn't be any logical slots which can be invalidated.
> >
> > Yes, effective_wal_level should be logical on the standby in this
> > case. But when replaying STATUS_CHANGE with logical_decoding=false
> > (i.e., !logical_decoding), it's obvious that the previous
> > effective_wal_level was logical, no?
> >
>
> Isn't it possible that when we are replaying STATUS_CHANGE with
> logical_decoding=false, the standby already has effective_wal_level
> lesser than 'logical'? If so, then in that case, we don't need to
> attempt invalidating the slots.

Since we write the STATUS_CHANGE with logical_decoding=false only when
logical decoding is enabled, I've not seen the case. But it's harmless
to check if logical decoding was enabled before trying to invalidate
logical slots when replaying STATUS_CHANGE with
logical_decoding=false. So I'll add it.

Regards,

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