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

On Mon, Oct 27, 2025 at 7:44 AM 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)
>
> ======
> 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?
>

Logical-decoding enabling/disabling is not db-specific. If we move the
counter-increment below db-oid check, we may end up wrongly requesting
disabling of logical decoding when we drop a particular DB while
another DB still has slots.

> ~~~
>
> 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/
>
> ~~~
>
> 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++?

I agree with this one.

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

Apart from the comments by Peter above, I have no more comments on
002. Verified, it works well.

thanks
Shveta