Re: walsender: Assert MyReplicationSlot is set before use

[Chao Li]

> On Feb 3, 2026, at 13:12, Fujii Masao <masao.fu...@gmail.com> wrote:
> 
> On Tue, Feb 3, 2026 at 12:38 PM Chao Li <li.evan.c...@gmail.com> wrote:
>>> I think we cannot assume the slot type here. A suitable checking might
>>> be: If a physical slot was acquired during logical replication, report an 
>>> error,
>>> just like we do in StartReplication().
>> 
>> Good point. In StartReplication(), we check MyReplicationSlot is not 
>> logical, correspondingly, in StartLogicalReplication(), we should check 
>> MyReplicationSlot is not physical.
> 
> StartLogicalReplication() calls CreateDecodingContext() after
> ReplicationSlotAcquire(), and CreateDecodingContext() seems to
> already perform this check. Isn't that sufficient?
> 
> Regards,
> 
> 
> -- 
> Fujii Masao

Ah, thank you so much for pointing out that. I didn’t notice 
CreateDecodingContext() has done the check already:
```
        /* shorter lines... */
        slot = MyReplicationSlot;

        /* first some sanity checks that are unlikely to be violated */
        if (slot == NULL)
                elog(ERROR, "cannot perform logical decoding without an 
acquired slot");

        /* make sure the passed slot is suitable, these are user facing errors 
*/
        if (SlotIsPhysical(slot))
                ereport(ERROR,
                                
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
                                 errmsg("cannot use physical replication slot 
for logical decoding")));
```

So, adding the check in StartLogicalReplication() is redundant. But I noticed 
the error message misses an “a” before “physical replication”. Looking at the 
error message in StartReplicaton():
```
        ReplicationSlotAcquire(cmd->slotname, true, true);
        if (SlotIsLogical(MyReplicationSlot))
                ereport(ERROR,
                                
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
                                 errmsg("cannot use a logical replication slot 
for physical replication")));
```

It has an “a” before “logical replication”. Also I fixed that in 
CreateDecodingContext().

PFA v3:

* 0001:
  - removed the validation of logical slot
  - added a comment in StartLogicalReplication() to explain sanity check is 
deferred
  - added an “a” in the error message in CreateDecodingContext()
* 0002: updated the expected error message in the TAP script

With v3 applied, the TAP test still passed.

Best reagards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

v3-0001-walsender-Assert-MyReplicationSlot-is-set-before-.patch
Description: Binary data

v3-0002-Add-TAP-test-for-logical-slot-type-race.patch
Description: Binary data