From: Tom Lane <tgl@sss.pgh.pa.us>
To: Daniel Gustafsson <daniel@yesql.se>
Cc: lxiaogang5 <lxiaogang5@gmail.com>
Cc: 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.
Date: Tue, 14 Jan 2025 09:14:34 -0500
Message-ID: <678865.1736864074@sss.pgh.pa.us> (raw)
In-Reply-To: <D57D44E8-C6FE-41FD-9D43-105BB9B73366@yesql.se>
References: <tencent_4F926019B88B74E2E7F51E57@qq.com>
<D57D44E8-C6FE-41FD-9D43-105BB9B73366@yesql.se>
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 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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: tgl@sss.pgh.pa.us, daniel@yesql.se, lxiaogang5@gmail.com
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: <678865.1736864074@sss.pgh.pa.us>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox