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

On Fri, Sep 19, 2025 at 10:49 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Fri, Sep 19, 2025 at 7:45 AM Ashutosh Bapat
> <ashutosh.bapat.oss@gmail.com> wrote:
> >
> > On Fri, Sep 12, 2025 at 2:26 PM Ashutosh Bapat
> > <ashutosh.bapat.oss@gmail.com> wrote:
> > >
> > > On Fri, Sep 12, 2025 at 9:38 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > > >
> > > >
> > > > One thing related to this which needs a discussion is after this
> > > > change, it is possible that part of the transaction contains
> > > > additional logical_wal_info. I couldn't think of a problem due to this
> > > > but users using pg_waldump or other WAL reading utilities could
> > > > question this. One possibility is that we always start including
> > > > logical_wal_info for the next new transaction but not sure if that is
> > > > required. It would be good if other people involved in the discussion
> > > > or otherwise could share their opinion on this point.
> > > >
> > >
> > > AFAIR, logical info is a separate section in a WAL record, and there
> > > is not marker which says "WAL will contain logical info henceforth".
> > > So the utilities should be checking for the existence of such info
> > > before reading it. So I guess it should be ok. Some extra sensitive
> > > utilities may expect that once a WAL record has logical info, all the
> > > succeeding WAL records will have it. They may find it troublesome that
> > > WAL records with and without logical info are interleaved. Generally,
> > > I would prefer that presence/absence of logical info changes at
> > > transaction boundaries, but we will still have interleaving WAL
> > > records. So I doubt how much that matters.
> > >
> > > Sorry for jumping late in the discussion. I have a few comments,
> > > mostly superficial ones. I am yet to take a deeper look at the
> > > synchronization logic.
> >
> > I started looking at the synchronization logic but stumbled at
> >
> > @@ -5100,6 +5139,7 @@ BootStrapXLOG(uint32 data_checksum_version)
> > checkPoint.ThisTimeLineID = BootstrapTimeLineID;
> > checkPoint.PrevTimeLineID = BootstrapTimeLineID;
> > checkPoint.fullPageWrites = fullPageWrites;
> > + checkPoint.logicalDecodingEnabled = IsLogicalDecodingEnabled();
> >
> > At the time of bootstrapping, logical decoding is solely dependent on
> > the boot_val of wal_level as there will not be any logical slots.
> > Above code however does not make this clear. If we were to change the
> > boot value of wal_level to logical this leads to a misleading
> > CHECKPOINT_SHUTDOWN record being added at the time of bootstrap like
> > below.
> > rmgr: XLOG len (rec/tot): 122/ 122, tx: 0, lsn: 0/01000028, prev
> > 0/00000000, desc: CHECKPOINT_SHUTDOWN redo 0/01000028; tli 1; prev tli
> > 1; fpw true; wal_level logical; logical decoding false; xid 0:3; oid
> > 10000; multi 1; offset 0; oldest xid 3 in DB 1; oldest multi 1 in DB
> > 1; oldest/newest commit timestamp xid: 0/0; oldest running xid 0;
> > shutdown
> >
> > This soon gets corrected by the following WAL record
> > rmgr: XLOG len (rec/tot): 27/ 27, tx: 0, lsn: 0/010000A8, prev
> > 0/01000028, desc: LOGICAL_DECODING_STATUS_CHANGE true
> >
> > So beyond misleading a code reader or someone who is reading the WAL,
> > this does not have any functional impact. But maybe we should consider
> > making this a bit more clear by setting
> > checkPoint.logicalDecodingEnabled based on wal_level in
> > BootStrapXLOG(). Whether we change the code or not, I think we should
> > add a comment to explain this code.
>
> I agree that calling IsLogicalDecodingEnabled() in BootStrapXLOG()
> could be quite confusing. I think we can directly set false there and
> add some comments for those who try to change the default wal_level
> value.

Or just set the value based on the wal_level.

--
Best Wishes,
Ashutosh Bapat