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

On Fri, Sep 5, 2025 at 8:50 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> 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.

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?

Regards,

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