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

On Fri, Nov 14, 2025 at 1:38 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Fri, Nov 14, 2025 at 5:01 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > On Thu, Nov 13, 2025 at 1:56 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > On Wed, Nov 12, 2025 at 3:36 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > > >
> > > > I've attached the updated version patch. I addressed all comments I
> > > > got so far, and made some cosmetic changes.
> > > >
> > >
> > > Few comments:
> > > 1. BTW, to accomplish what we do in
> > > UpdateLogicalDecodingStatusEndOfRecovery(), can't we follow a logic
> > > similar to what we do in EnsureLogicalDecodingEnabled()?
> > >
> > > Basically, enable xlog_logical_info and mark SharedRecoveryState =
> > > RECOVERY_STATE_DONE under one spinlock. After the spinlock is
> > > released, wait for other backends to reflect the xlog_logical_info via
> > > WaitForProcSignalBarrier(EmitProcSignalBarrier(PROCSIGNAL_BARRIER_UPDATE_XLOG_LOGICAL_INFO))
> > > and then enable logical decoding by setting logical_decoding_enabled
> > > and writing a new WAL record.
> > >
> > > Can't this avoid the special handling required in the patch for the
> > > status_change_allowed flag? This flag seems to be primarily required
> > > because recovery_done action and the change of wal_level action are
> > > being performed separately during promotion. If it is feasible to
> > > perform them together, then we don't need an additional state that
> > > says logical decoding status_change_allowed.
> > >
> > > The reason I am trying to explore an alternative to the current way to
> > > handle promotion is that it can eliminate one of the complex parts of
> > > the patch. But, if that is not feasible then we can live with the
> > > current approach.
> >
> > Thank you for the suggestion! How should it work in cases where we
> > need to disable logical decoding at the end of recovery? We accept WAL
> > writes as soon as we mark SharedRecoveryState = RECOVERY_STATE_DONE,
> > so we need to disable logical decoding with a STATUS_CHANGE WAL write
> > beforehand. And then we disable xlog_logical_info and update
> > SharedRecoveryState under one spinlock. Probably we need to do these
> > operations while status_change_inprogress being true to prevent
> > concurrent logical decoding status updates, and clear the flag after
> > recovery fully completes. While it seems to work, we need to verify
> > the side-effects due to moving xlog_logical_info to XLogCtlData since
> > every process needs to retrieve the flag at process startup time.
> >
> > After thinking of this idea more, I think we can simplify the idea
> > more; we do all end-of-recovery operations (updating both
> > xlog_logical_info and logical_decoding_eanbled, WAL writes, and
> > synchronization) while status_change_inprogress being true, and after
> > recovery fully completes we clear the inprogress flag. It's probably
> > okay to perform these operations in any order since WAL writes are
> > still not permitted, and we can prevent concurrent logical decoding
> > status updates until we clear the inprogress flag, which happens after
> > marking SharedRecoveryState = RECOVERY_STATE_DONE. I've drafted this
> > idea. Please see the 0002 patch.
> >
>
> I can look at the patch but this inprogress flag is another part which
> I wanted to avoid if possible again due to its additional complexity.
> So, I came up with an alternative locking scheme to enable/disable
> decoding. You can compare both the ideas and share your thoughts:
>
> During promotion code path:
> 1. Acquire LogicalDecodingControlLock, then check and remember whether
> we need to enable or disable the xlog_logical_info and
> logical_decoding_enabled. Release LogicalDecodingControlLock.
> 2. Set xlog_logical_info and mark SharedRecoveryState =
> RECOVERY_STATE_DONE under one spinlock.
> 3. After the spinlock and controlfile lock are released, wait for
> other backends to reflect the xlog_logical_info via
> WaitForProcSignalBarrier(EmitProcSignalBarrier(PROCSIGNAL_BARRIER_UPDATE_XLOG_LOGICAL_INFO)),
> 4. Acquire LogicalDecodingControlLock in X mode, then there are two
> cases to deal with:
>   (a) As part of step-1, the decision was to enable logical decoding.
> So, we first check if some backend has already enabled it by checking
> logical_decoding_enabled, if so, then we don't need to do anything.
> Otherwise, once again count_slots to ensure that the concurrent
> backend hasn't removed them, and if there still exist any, then set
> logical_decoding_enabled, write a new WAL record, and release the
> LogicalDecodingControlLock.
>   (b) As part of step-1, the decision was to disable logical decoding.
> So, we first check if some backend has already enabled it by checking
> logical_decoding_enabled, if so, then we don't need to do anything.
> Otherwise, set logical_decoding_enabled to false, write WAL record,
> and release the LogicalDecodingControlLock.

I'm not sure it works in cases where we need to disable logical
decoding at the end-of-recovery. Suppose that the decision made in
step-1 was to disable logical decoding, it's possible that non-logical
WAL records are written as soon as step-3 finishes while the logical
decoding is still enabled. This is because the backend processes who
started after step-3 see xlog_logical_info = false. This ends up with
logical decoding decoding non-logical WAL records.

And there is a small window between step-2 and step-4(a) where
wal_level shows 'logical' but logical decoding is not enabled.

Regards,

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