From: Tom Lane <tgl@sss.pgh.pa.us>
To: Daniel Gustafsson <daniel@yesql.se>
cc: lxiaogang5 <lxiaogang5@gmail.com>, pgsql-bugs <pgsql-bugs@postgresql.org>
Subject: 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.
In-reply-to: <D57D44E8-C6FE-41FD-9D43-105BB9B73366@yesql.se>
References: <tencent_4F926019B88B74E2E7F51E57@qq.com>
 <D57D44E8-C6FE-41FD-9D43-105BB9B73366@yesql.se>
Comments: In-reply-to Daniel Gustafsson <daniel@yesql.se>
	message dated "Tue, 14 Jan 2025 11:57:34 +0100"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <678864.1736864074.1@sss.pgh.pa.us>
Content-Transfer-Encoding: quoted-printable
Date: Tue, 14 Jan 2025 09:14:34 -0500
Message-ID: <678865.1736864074@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/678865.1736864074%40sss.pgh.pa.us>
Precedence: bulk

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 (creati=
ng a new slot) when it internally traverses the ReplicationSlotCtl->replic=
ation_slots[max_replication_slots] array. When an available slot is found =
(slot =3D s), it should break the current for loop. This issue still exist=
s 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 o=
ther
> performance problems as well (max_replication_slots is by default 10).

Are we reading the same code?

    for (i =3D 0; i < max_replication_slots; i++)
    {
        ReplicationSlot *s =3D &ReplicationSlotCtl->replication_slots[i];

        if (s->in_use && strcmp(name, NameStr(s->data.name)) =3D=3D 0)
            ereport(ERROR,
                    (errcode(ERRCODE_DUPLICATE_OBJECT),
                     errmsg("replication slot \"%s\" already exists", name=
)));
        if (!s->in_use && slot =3D=3D NULL)
            slot =3D 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 =3D=3D 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