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

On Thu, Sep 4, 2025 at 1:24 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Tue, Sep 2, 2025 at 4:35 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Fri, Aug 29, 2025 at 9:38 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > >
> > > I've attached the updated patch.
> > >
> >
> > Few comments:
>
> Thank you for the comments!
>
> > =============
> > 1.
> > + * When XLogLogicalInfoActive() is true, guarantee that a subtransaction's
> > + * xid can only be seen in the WAL stream if its toplevel xid has been
> > + * logged before. If necessary we log an xact_assignment record with fewer
> > + * than PGPROC_MAX_CACHED_SUBXIDS. Note that it is fine if didLogXid isn't
> > + * set for a transaction even though it appears in a WAL record, we just
> > + * might superfluously log something. That can happen when an xid is
> > + * included somewhere inside a wal record, but not in XLogRecord->xl_xid,
> > + * like in xl_standby_locks.
> >   */
> >   if (isSubXact && XLogLogicalInfoActive() &&
> >   !TopTransactionStateData.didLogXid)
> >
> > Instead of writing XLogLogicalInfoActive() is true in comments, can we
> > say When effective wal_level is logical and then also point to some
> > place if required where the patch has explained about effective
> > wal_level? Otherwise, it sounds like we are writing what is apparent
> > from code and may not be very clear.
>
> Agreed.
>
> >
> > 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.

--
With Regards,
Amit Kapila.