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

On Thu, Oct 16, 2025 at 7:18 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Tue, Oct 14, 2025 at 12:52 AM Peter Smith <smithpb2250@gmail.com> wrote:
> >
> > Hi Sawada-San,
> >
> > I started to look again at this thread. Here are some comments for v18
> > (the documentation only).
>
> Thank you for reviewing the patch!
>
> >
> > ======
> > doc/src/sgml/config.sgml
> >
>
> >
> > ======
> > doc/src/sgml/logicaldecoding.sgml
> >
> > 3.
> > +    <para>
> > +     When <varname>wal_level</varname> is set to <literal>replica</literal>,
> > +     logical decoding is automatically activated upon creation of the first
> > +     logical replication slot. This activation process involves several steps
> > +     and requires waiting for any concurrent transactions to finish, ensuring
> > +     system-wide consistency. Conversely, when the last logical
> > replication slot
> > +     is dropped from a system with <varname>wal_level</varname> set to
> > +     <literal>replica</literal>, logical decoding is automatically disabled.
> > +     Note that the deactivation of logical decoding might take some time as
> > +     it is performed asynchronously by the checkpointer process.
> > +    </para>
> >
> > Here it seems to be describing again the concept of the
> > "effective_wal_level" as in the config.sgml, so should this paragraph
> > also make mention of that terminology, and link to that concept like
> > was done before?
>
> Could you elaborate on why we need to mention effective_wal_level
> here? I thought that in this section, it's sufficient to describe what
> conditions are required to enable or disable logical decoding.
>
> > ~~~
> >
> > 4.
> >       Existing logical slots on standby also get invalidated if
> > -     <varname>wal_level</varname> on the primary is reduced to less than
> > -     <literal>logical</literal>.
> > +     logical decoding is disabled on the primary.
> >
> > Would it be easier just to leave the text as it was, but say
> > "effective_wal_level" instead of "wal_level"?
>
> I find that saying "if logical decoding is disabled" sounds more
> understandable for users than saying "if effective_wal_level is
> reduced to less than logical", but what do you think?
>
> > ======
> > doc/src/sgml/system-views.sgml
> >
> > 5.
> >           <para>
> >            <literal>wal_level_insufficient</literal> means that the
> > -          primary doesn't have a <xref linkend="guc-wal-level"/> sufficient to
> > -          perform logical decoding.  It is set only for logical slots.
> > +          logical decoding is disabled on primary (See
> > +          <xref linkend="logicaldecoding-explanation-log-dec"/>. It is set
> > +          only for logical slots.
> >           </para>
> >
> > 5a.
> > Should this also make use of the "effective_wal_level" terminology?
> >
> > e.g
> > "...means that the logical decoding is disabled on primary (i.e the
> > effective_wal_level is not logical)"
>
> What is the point of mentioning both "logical decoding is disabled"
> and "effective_wal_level is not logical"? I think it isn't necessary
> to mention both things in all the places where we mention logical
> decoding availability, but I'll change it if others also think the
> same.
>

(re my above comments about effective_wal_level)

When reading the patched documentation, it just seemed to me that
although there is the introduced concept / parameter for
"effective_wal_level", it is hardly used at all except buried away
with just a few mentions in the decoding/config sections. In
particular, I was surprised that "effective_wal_level" was not
mentioned once in the entire "Logical Replication" section of the
docs.

I felt this new parameter needed to be referred to much more often
(with an xref to it). Just saying "if logical decoding is disabled" by
itself doesn't convey anything about the "effective" wal level; a user
who hasn't stumbled across those rare mentions of
"effective_wal_level" might easily think that wal_level=replica always
means "logical decoding is disabled", because that's what it always
used to mean until this patch. OTOH, referring to
"effective_wal_level" (with an xref to it) makes it so much clearer.

BTW, I repeated similar comments in my subsequent code review for
things like code-comments and error messages. Basically, I think
saying "logical decoding is enabled/disabled" sounds good, but it just
doesn't convey as much meaning that "effective_wal_level is logical"
has.

> >
> > ~
> >
> > 5b.
> > Would it be better to list all these reasons in alphabetical order?
> >
> > ~
> >
> > 5c.
> > I felt the "It is set only for logical slots" to be a bit vague. IMO
> > it is clearer to say "This reason value is only used for logical
> > slots".
> >
> > Alternatively, maybe the whole list should be restructured like below
> >
> > SUGGESTION
> > The reason for the slot's invalidation. NULL if the slot is not invalidated.
> >
> > Possible reason values for logical or physical slots are:
> > - wal_removed means ...
> > - idle_timeout means ...
> >
> > Possible reason values, only for logical slots are:
> > - rows_removed means ...
> > - wal_level_insufficient means ...
>
> I find that those changes would be a general improvement for the
> description of invalidation_reason. It should be discussed in a
> separate thread.
>

OK.

======
Kind Regards,
Peter Smith.
Fujitsu Australia