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

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.

2.
We don't need to
+ * wait for running transactions to finish as we don't accept any writes
+ * yet. We need the wait even if we've not updated the status above as the
+ * status have been turned on and off during recovery, having running
+ * processes have different status on their local caches.

In the first sentence, the comment says, we don't need to wait, and in
the second, it says we need the wait... Isn't it confusing? What is
the difference between both waits, if any? Can we improve this
comment?

IIUC, this comment means that we should wait for all other backends to
reflect the new state for xlog_logical_info and
logical_decoding_enabled. But the way it is written makes it
confusing.

--
With Regards,
Amit Kapila.