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

On Sun, Oct 26, 2025 at 7:14 PM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> Hi Sawada-San,
>
> Some review comments for v22-0002:
>
> ======
> doc/src/sgml/logicaldecoding.sgml
>
> 1.
> 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.
> + from a system or invalidated 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>
>
> I felt that "Conversely..." sentence should be worded more like the
> 1st sentence of the paragraph.
>
> CURRENT
> Conversely, when the last logical replication slot is dropped from a
> system or invalidated with <varname>wal_level</varname> set to
> <literal>replica</literal>, logical decoding is automatically
> disabled.
>
> SUGGESTION
> Conversely, if <varname>wal_level</varname> is
> <literal>replica</literal> and the last logical replication slot is
> dropped or invalidated, logical decoding is automatically disabled.
>
> (Aside: most of this suggestion maybe belongs for patch 0001 -- only
> the "or invalidated" part is added for patch 0002)

Looks good.

>
> ======
> src/backend/replication/slot.c
>
> ReplicationSlotsDropDBSlots:
>
> 2.
> - nlogicalslots++;
> + if (s->data.invalidated == RS_INVAL_NONE)
> + n_valid_logicalslots++;
>
> /* not our database, skip */
> if (s->data.database != dboid)
> continue;
>
> Should that counter increment be moved to be *below* the "not our
> database, skip" code?

As Shveta already pointed out, we need to increment the counter before
the check since we want to disable logical decoding only when there
are no logical slots on the database cluster.

>
> ~~~
>
> CheckLogicalSlotExists:
>
> 3.
> /*
> - * Returns true if there is at least in-use logical replication slot.
> + * Returns true if there is at least in-use valid logical replication slot.
> */
> bool
> CheckLogicalSlotExists(void)
>
> Typo? /is at least in-use valid/is at least one in-use valid/

Fixed.

>
> ~~~
>
> 4.
> + if (SlotIsLogical(s))
> + {
> + if (s->data.invalidated == RS_INVAL_NONE)
> + n_valid_logicalslots++;
> +
> + /* Prevent invalidation of logical slots during binary upgrade */
> + if (IsBinaryUpgrade)
> + continue;
> + }
>
> Is it better to do the binary check first, before the n_valid_logicalslots++?

Why do you think it's better?

>
> ======
> src/test/recovery/t/049_effective_wal_level.pl
>
> 5.
> -# Promote the standby4 and check if effective_wal_level remains 'logical' even
> -# after the promotion since it has an invalidated logical slot.
> +# Promote the standby4 and check if effective_wal_level is now 'logical' after
> +# the promotion since there is no valid logical slot.
> $standby4->promote;
> -test_wal_level($standby4, "replica|logical",
> - "effective_wal_level remains 'logical' even after promotion as it
> has one invalidated slot"
> +test_wal_level($standby4, "replica|replica",
> + "effective_wal_level got decreased to 'replica' as there is no valid
> logical slot"
> );
>
> The test comment seems wrong; shouldn't it say "check if
> effective_wal_level_got decreased to replica"?
>
> ~~~
>
> 6.
> # Drop the invalidated slot, decreasing effective_wal_level to 'replica'.
> @@ -287,7 +326,7 @@ $standby4->safe_psql('postgres',
> qq[select pg_drop_replication_slot('standby4_slot')]);
> wait_for_logical_decoding_disabled($standby4);
> test_wal_level($standby4, "replica|replica",
> - "effective_wal_level got decreased to 'replica' after dropping the
> last invalidated slot"
> + "effective_wal_level doesn't change after dropping the last invalidated slot"
> );
>
> The test comment seems wrong; effective_wal_level was already
> 'replica' before this test.

Fixed.

Regards,

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