On Thu, Aug 21, 2025 at 10:34 PM Masahiko Sawada <sawada.m...@gmail.com> wrote:
>
> 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().
Okay, I see. I missed that point earlier.
>
> >
> > 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?
>
Yes, it makes sense after incrementing 'nlogicalslots' even for other databases.
> >
> > 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.
>
Okay, makes sense.
> >
> > 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
