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

On Fri, Sep 19, 2025 at 7:45 AM Ashutosh Bapat
<ashutosh(dot)bapat(dot)oss(at)gmail(dot)com> wrote:
>
> On Fri, Sep 12, 2025 at 2:26 PM Ashutosh Bapat
> <ashutosh(dot)bapat(dot)oss(at)gmail(dot)com> wrote:
> >
> > On Fri, Sep 12, 2025 at 9:38 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)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.

Regards,

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