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

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
>
> 1.
> +       <para>
> +        It is important to note that when
> <varname>wal_level</varname> is set to
> +        <literal>replica</literal>, the effective WAL level can
> automatically change
> +        based on the presence of <link
> linkend="logicaldecoding-replication-slots">
> +        logical replication slots</link>. The system automatically
> increases the
> +        effective WAL level to <literal>logical</literal> when
> creating the first
> +        logical replication slot, and decreases it back to
> <literal>replica</literal>
> +        when dropping the last logical replication slot. The current
> effective WAL
> +        level can be monitored through <xref
> linkend="guc-effective-wal-level"/>
> +        parameter.
> +       </para>
>
>
> As you say "It is important to note...". So, should this all be using
> <note> SGML markup?

I'm not sure when we have to use <note> but I find that it's not
necessary here. I think we can change later if others think so too.

>
> ~~~
>
> 2.
> +      <indexterm>
> +       <primary><varname>effective_wal_level</varname> configuration
> parameter</primary>
> +      </indexterm>
>
> Should this even be called a "configuration parameter", given that you
> can't configure it?

I think we call even pre-set GUC parameters "configuration
parameters", so it should follow.

>
> ======
> 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.

>
> ~
>
> 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.

>
> ~
>
> 5d.
> A missing closing parenthesis. "(See xxx"

Fixed.

Regards,

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