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

On Fri, Jul 25, 2025 at 11:45 AM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
> 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].
>

Thank You for the patch. I have not reviewed fully, but please find
few comments:

1)
CreateReplicationSlot():

Assert(cmd->kind == REPLICATION_KIND_LOGICAL);
+ EnsureLogicalDecodingEnabled();
CheckLogicalDecodingRequirements();
ReplicationSlotCreate(...);

We may have another race-condition here. We have
EnsureLogicalDecodingEnabled() before ReplicationSlotCreate(). It
means we are enabling logical-decoding before incrementing
LogicalDecodingCtl->n_logical_slots. So before we increment
LogicalDecodingCtl->n_logical_slots through ReplicationSlotCreate(),
another session may try to meanwhile drop the logical slot (another
one and last one), and thus it may end up disabling logical-decoding
as it will find n_logical_slots as 0.

Steps:
a) Create logical slot logical_slot1 on primary.
b) Create publication pub1.
c) During Create-sub on subscriber, stop walsender after
EnsureLogicalDecodingEnabled() by attaching debugger.
d) Drop logical_slot1 on primary.
e) Release the walsender debugger.

2)
create_logical_replication_slot:

ReplicationSlotCreate(name, true
....
+ EnsureLogicalDecodingEnabled();
+ CheckLogicalDecodingRequirements();

Earlier we had CheckLogicalDecodingRequirements() before we actually
created the slot. Now we had it after slot-creation. It makes sense to
do Logical-Decoding related checks post EnsureLogicalDecodingEnabled,
but 'CheckSlotRequirements' should be done prior to slot-creation.
Otherwise we will end up creating the slot and later dropping it when
it should not have been created in the first place (for say wal_level
< replica).

3)
+ EnsureLogicalDecodingEnabled();
+

We can get rid of this from slotsync as this is no-op on standby

4)
pg_sync_replication_slots()
if (!RecoveryInProgress())
ereport(ERROR,

errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
errmsg("replication slots can only be
synchronized to a standby server"));

+ EnsureLogicalDecodingEnabled();

This API is called on standby alone, so EnsureLogicalDecodingEnabled
is not needed here either.

thanks
Shveta