On Mon, Jul 28, 2025 at 10:02 PM shveta malik <shveta.malik@gmail.com> wrote:
>
> 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.
True. EnsureLogicalDecodingEnabled() has to be called after creating a
logical replication slot in order to reliably enable logical decoding.
>
>
> 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.
Thank you for reviewing the patch! Agree with these comments. They
have been addressed in the latest patch I've just submitted[1].
Regards,
[1] https://www.postgresql.org/message-id/CAD21AoB%3DRf-SASOJR2WqvWcrA5Q3S2oUBACVLdJPaA8x6EchBA%40mail.gmail.com
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
