Re: There is a defect in the ReplicationSlotCreate() function where it iterates through ReplicationSlotCtl->replication_slots[max_replication_slots] to find a slot but does not break out of the loop when a slot is found.

> On 14 Jan 2025, at 15:14, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> Daniel Gustafsson <daniel(at)yesql(dot)se> writes:
>> On 14 Jan 2025, at 05:24, lxiaogang5 <lxiaogang5(at)gmail(dot)com> wrote:
>>> There is a logical defect in the function ReplicationSlotCreate (creating a new slot) when it internally traverses the ReplicationSlotCtl->replication_slots[max_replication_slots] array. When an available slot is found (slot = s), it should break the current for loop. This issue still exists in the latest code, resulting in wasted CPU resources.
>
>> We could exit early, but any system which max_replication_slots set high enough
>> that the savings are measurable in a non-hot codepath is likely having other
>> performance problems as well (max_replication_slots is by default 10).
>
> Are we reading the same code?
>
> for (i = 0; i < max_replication_slots; i++)
> {
> ReplicationSlot *s = &ReplicationSlotCtl->replication_slots[i];
>
> if (s->in_use && strcmp(name, NameStr(s->data.name)) == 0)
> ereport(ERROR,
> (errcode(ERRCODE_DUPLICATE_OBJECT),
> errmsg("replication slot \"%s\" already exists", name)));
> if (!s->in_use && slot == NULL)
> slot = s;
> }
>
> In the first place, breaking early would be wrong: we would fail to
> scan all slots to check for duplicate name. In the second place,
> the code does choose the first not-in-use slot, because of the
> check for "slot == NULL" before changing the value of "slot".

My interpretation of the proposal was that once the first not-in-use slot
chosen and slot set to s, the for loop can be exited since no further slot
assignment will happen. You are right though that we still need to inspect all
replication slots to check for duplicates.

--
Daniel Gustafsson