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

On Tue, Oct 28, 2025 at 3:59 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Mon, Oct 27, 2025 at 8:38 PM shveta malik <shveta.malik@gmail.com> wrote:
> >
> > On Mon, Oct 27, 2025 at 6:12 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> >
> > > Another related point is that, say we decide to disable decoding
> > > because the last logical slot got invalidated and
> > > RequestDisableLogicalDecoding()->LogicalDecodingStatusChangeAllowed()
> > > returns false, then how the disabling will happen?
> > >
> >
> > If LogicalDecodingStatusChangeAllowed() returns false, we do not
> > disable logical decoding because that essentially means
> > recovery-in-progress and if that is the case, we do not allow
> > status-change. LogicalDecodingStatusChangeAllowed() returns false
> > always on standby until it is promoted and recovery ends.
> >
>
> But the request will be lost and we won't be able to disable decoding
> after promotion.
>

Okay, I noticed that even if this request is lost the promotion path
will take care of disabling the decoding via
UpdateLogicalDecodingStatusEndOfRecovery().

I have noticed a few minor points:
1.
@@ -377,6 +378,12 @@ CheckpointerMain(const void *startup_data, size_t
startup_data_len)
  */
  AbsorbSyncRequests();

+ /*
+ * Disable logical decoding if someone requested it. See comments atop
+ * logicalctl.c.
+ */
+ DisableLogicalDecodingIfNecessary();
+

The patch calls DisableLogicalDecodingIfNecessary() in the beginning
of checkpointer loop, so if there is any delay in disabling there is
more chance that scheduled checkpoint can delay. I think it is better
to do this before wait like we do for switching wal files via a call
to CheckArchiveTimeout().

2.
@@ -7054,6 +7097,7 @@ CreateCheckPoint(int flags)

  checkPoint.fullPageWrites = Insert->fullPageWrites;
  checkPoint.wal_level = wal_level;
+ checkPoint.logicalDecodingEnabled = IsLogicalDecodingEnabled();

This is the place where we have blocked all concurrent WAL writes via
WALInsertLockAcquireExclusive(), so why to set it here when it can be
done after releasing the lock with other parameters in checkpoint
record aka after 'Get the other info we need for the checkpoint
record.'. I guess you have done to set logicalDecodingEnabled along
with wal_level but I think it is better to set it after releasing the
lock along with other parameters unless there is a reason to do it
under WALInsertLockAcquireExclusive.

--
With Regards,
Amit Kapila.