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

On Fri, Nov 14, 2025 at 5:10 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Fri, Nov 14, 2025 at 3:12 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Fri, Nov 14, 2025 at 4:15 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > >
> > > 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:
> > > >
> > > > 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.
> > >
> >
> > If the startup process decides to disable decoding, this means there
> > doesn't exist any logical slot and wal_level is 'replica', right? If
> > so, then when we create the first slot before decoding, we should try
> > to first enable xlog_logical_info, if not already enabled, wait for
> > all backends to reflect that state. So, that should be sufficient.
>
> Right. But I think its (cascaded) standby could have logical slots and
> decode non-logical WAL records.
>

Okay, so it is possible because cascaded standby could decode
non-logical WAL records written on promoted standby after we disable
xlog_logical_info and before we could disable logical_decoding and
write WAL for it. This is not possible in the current patch because it
disables xlog_logical_info, logical_decoding, and writes WAL for the
same before marking recovery state as recovery_done. So before any
non-logical WAL in the system could be replicated to a cascaded
standby node, the WAL for disabling the logical_decoding would be
replicated, the replay of which will disable logical_decoding on
cascaded standby node.

I have a few questions about this, is it okay to write a new WAL
record (by startup process) before marking the recovery state as
recovery_done? Are we doing that even without this patch at any other
place?

In v26-0002-FIXUP-remove-status_change_allowed-flag, by using
status_change_inprogress, we ensure that no backend is allowed to
toggle the logical_wal/decoding status till startup process marks the
recovery state as recovery_done. I am trying to think what problem
this part of design prevents. I have considered the following
scenarios:

Scenario-1:
1. Startup process enables logical_wal and logical_decoding. Writes
WAL record for it
2. Backend disables logical_decoding, writes WAL for it, and disables
logical_wal.
3. Startup process sets recovery_done and allows wal_writes

Say, instead of using status_change_inprogress to prevent doing
step-2, if we had used recovery_in_progress kind of flag then how is
it possible for backends to create any problem for the current node or
cascaded standbys? I think the only way a problem can happen is if we
write the WAL to disable_logical decoding after any backend could have
written a non-logical WAL information record. Can that happen if we
use the recovery_in_progress flag to prevent disable of logical_wal?
If so, how?

Scenario-2:
1. Startup process disables logical_wal and logical_decoding. Writes
WAL record for it
2. Backend enables logical_wal, wait for other backends to reflect
this state, enable logical_decoding and writes WAL for it.
3. Startup process sets recovery_done and allows wal_writes

Here, I see that the new patch is already using recovery_in_progress.
So, not sure if the latest patch has used status_change_inprogress
during recovery to cover this scenario. Am I missing something?

--
With Regards,
Amit Kapila.