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

On Wed, Aug 20, 2025 at 3:11 AM shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
>
> Please find a few comments:

Thank you for reviewing the patch!

>
> 1)
> ReplicationSlotsDropDBSlots:
> + bool dropped = false;
>
> We can name 'dropped ' as 'dropped_logical' similar to ReplicationSlotCleanup.

I think we don't necessarily need to add 'logical' because this
function attempts to drop only logical slots unlike
ReplicationSlotCleanup().

>
> 2)
> ReplicationSlotsDropDBSlots()
> +
> + if (dropped && nlogicalslots == 0)
> + DisableLogicalDecodingIfNecessary();
>
> I could not understand the need of 'nlogicalslots' condition here?
> Once we increment 'nlogicalslots', there is no way we can skip the
> loop without dropping the slot with the only exception of ERROR-ing
> out if active_pid is non NULL. So if the loop has completed and we
> have reached this sage, won't it essentially mean 'nlogicalslots' is 0
> in both cases: a) we actually dropped any slot; b) we did not find
> any slot to drop. Or am I missing something?

I think I should have incremented nlogicalslots even for logical slots
on other databases. What I want to do here is to call
DisableLogicalDecodingIfNecessary() only when we have dropped at least
one logical slots and there is no logical slots on the whole database
cluster as a result. If we have logical slots only on the current
database, we eventually reach the above 'if' statement with
dropped=true and nlogicalslots=0. On the other hand, if we have
logical slots also on other databases, we reach there with
dropped=true and nlogicalslots>0, meaning we don't want to disable
logical decoding. Does it make sense?

>
> Same is the case with ReplicationSlotCleanup().
>
> 3)
> Few typos:
>
> + /*
> + * Update shmem flags. We don't need to care about the order of setting
> + * global flag and writing the WAL record this case since writes are not
> + * allowed yet.
> + */
>
> this case --> in this case
>
> + * This is the authoritative value used by the all process to determine
>
> 'used by all the processes'

Fixed.

> 049_effective_wal_level.pl:
> 4)
>
> Few typos:
> +# Initialize standby2 ndoe form the backup 'my_backup'.
>
> ndoe form --> node from
>
> +# Test the race condition between the startup and logical decoding
> statuc change.
>
> statuc --> status

Fixed.

>
> 5)
> +# Promote the standby2 node that has one logical slot. So the logical decoding
> +# keeps enabled even after the promotion.
> +$standby2->promote;
> +test_wal_level($standby2, "replica|logical",
> + "effective_wal_level keeps 'logical' even after the promotion");
> +$standby2->safe_psql('postgres',
> + qq[select pg_create_logical_replication_slot('standby2_slot2', 'pgoutput')]
> +);
> +$standby2->stop;
>
> Do we need 'pg_create_logical_replication_slot' here?

Yes, I put it to check if we can create a logical slot even after the
promotion. I've added the comment to explain it.

>
> 6)
>
> +test_wal_level($primary, "replica|replica",
> + "effective_wal_level got decreased to 'replica' on primary");
> +test_wal_level($standby3, "logical|replica",
> + "effective_wal_level got decreased to 'replica' on standby");
> +test_wal_level($cascade, "replica|replica",
> + "effective_wal_level got decreased to 'logical' on standby");
> +
>
> Last one should also say: decreased to 'replica' (instead of logical)

Fixed.

I've attached the updated patch.

Regards,

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