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.

Daniel Gustafsson <daniel@yesql.se> writes:
> On 14 Jan 2025, at 05:24, lxiaogang5 <lxiaogang5@gmail.com> wrote:
>> There is a logical defect in the function ReplicationSlotCreate (creating a new slot) when it internally traverses
theReplicationSlotCtl->replication_slots[max_replication_slots] array. When an available slot is found (slot = s), it
shouldbreak 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".

If we intended to support hundreds or thousands of replication
slots, it'd perhaps be worthwhile to replace this with a hashtable
lookup.  But that doesn't seem like a very credible use-case.
For typical numbers of slots this way is likely faster than
messing with a hashtable.

            regards, tom lane