On Tue, Jul 22, 2025 at 11:44 PM shveta malik <shveta.ma...@gmail.com> wrote: > > On Tue, Jul 22, 2025 at 5:03 AM Masahiko Sawada <sawada.m...@gmail.com> wrote: > > > > Yes, I agree. The main patch focuses on the part where we > > automatically change the effective WAL level upon the logical slot > > creation and deletion (and potentially remove 'logical' from > > wal_level), and other things are implemented as additional features in > > a separate patch. > > I am keeping my focus on patch001 until we decide further on how to > protect the slot.
Yeah, I also dropped the additional feature patch from the patch set for now.
> Apart from few comments in [1], please find one
> more concern:
>
> There is a race condition between creating and dropping a replication
> slot when enabling or disabling logical decoding. We might end up with
> logical decoding disabled even when a logical slot is present.
>
> Steps:
> 1) Set wal_level=replica on primary.
> 2) Create logical_slot1 which will enable logical decoding, causing
> effective_wal_level to become logical.
> 3) Drop logical_slot1 and pause execution inside
> DisableLogicalDecodingIfNecessary() right after the
> 'n_inuse_logical_slots' check using a debugger.
> 4) In another session, create logical_slot2. It will attempt to enable
> logical-decoding but since it is already enabled,
> EnsureLogicalDecodingEnabled() will be a no-op.
> 5) Release debugger of drop-slot, it will disable logical decoding.
>
> Ultimately, logical_slot2is present while logical decoding is disabled
> and thus we see this:
>
> postgres=# select slot_name from pg_replication_slots;
> slot_name
> ---------------
> logical_slot2
>
> postgres=# show effective_wal_level;
> effective_wal_level
> ---------------------
> replica
> (1 row)
>
> postgres=# select pg_logical_slot_get_changes('logical_slot2', NULL,
> NULL, 'proto_version', '4', 'publication_names', 'pub');
> ERROR: logical decoding is not enabled
> HINT: Set "wal_level" >= "logical" or create at least one logical slot.
>
> Shall we acquire LogicalDecodingControlLock in exclusive mode at a
> little earlier stage? Currently we acquire it after
> IsLogicalDecodingEnabled() check. I think we shall acquire it before
> this check in in both enable and disable flow?
Thank you for testing the patch!
I've reworked the locking part in the patch. The attached v4 patch
should address all review comments including your previous
comments[1].
Regards,
[1]
https://www.postgresql.org/message-id/CAJpy0uC0e%3DJ7L4q9RnQ3pbSAtvWy40r9qp3tr41zoogHQmDO8g%40mail.gmail.com
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
v4-0001-Enable-logical-decoding-dynamically-based-on-logi.patch
Description: Binary data
v4-0002-Add-regression-tests-for-online-logical-decoding-.patch
Description: Binary data
