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(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".

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