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

On Fri, Oct 24, 2025 at 3:23 AM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
> On Thu, Oct 23, 2025 at 11:52 AM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
> I've addressed the above comments and made some cosmetic changes.
>

Below are few comments/questions:

1.
+ <para>
+ When <varname>wal_level</varname> is set to <literal>replica</literal>,
+ dropping the last logical slot may disable logical decoding on
the primary,

In this sentence, using 'may' sounds wrong because we will disable
logical decoding on primary. Is there any case where the same won't
happen?

2.
* don't include the new tuple in the WAL record in that case. Also
- * disable if wal_level='logical', as logical decoding needs to be able to
+ * disable if logical decoding is enabled and the relation requires WAL to
+ * be logged for logical decoding, as logical decoding needs to be able to
* read the new tuple in whole from the WAL record alone.

Using logical decoding in the above sentence thrice sounds redundant.
Can we avoid its second usage?

3.
+ /*
+ * During the recovery, effective_wal_level inherits on the primary's

'on' sounds redundant in the above sentence.

4.

+/*
+ * Initialize the logical decoding status on shmem at server startup.
…
…
+/*
+ * Enable or disable both status of logical info WAL logging and
logical decoding
+ * on shared memory.
+

In both the above comments, instead of on shmem, it seems in shmem
should be used.
/both status/both the status

5.
+bool
+CheckLogicalSlotExists(void)
{
…
+ /* NB: counting invalidated slots */
+
+ if (SlotIsLogical(s))

Why can't we avoid counting invalid slots? I think this needs more
comments. BTW, shouldn't this patch consider changing
effective_wal_level when the last logical slot is invalidated?
Ideally, when logical decoding is not possible in the system, then we
can reduce the wal_level back to replica, no?

6.
Therefore, there
+ * should not be any operations that could wait for the checkpointer until
+ * then.
+ */
+ UpdateLogicalDecodingStatusEndOfRecovery();
+

This part of the comment is not clear to me. Can you please try to
explain with some context?

--
With Regards,
Amit Kapila.