On Wed, Aug 20, 2025 at 3:11 AM shveta malik <shveta.ma...@gmail.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
v8-0001-Enable-logical-decoding-dynamically-based-on-logi.patch
Description: Binary data
