On Fri, Jul 25, 2025 at 11:45 AM Masahiko Sawada <sawada.mshk@gmail.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
