Feedback-ID: ic6394509:Fastmail
MIME-Version: 1.0
Date: Thu, 09 Oct 2025 10:07:21 +0200
From: "Joel Jacobson" <joel@compiler.org>
To: "Chao Li" <li.evan.chao@gmail.com>
Cc: "Tom Lane" <tgl@sss.pgh.pa.us>,
 "Matheus Alcantara" <matheusssilv97@gmail.com>,
 pgsql-hackers <pgsql-hackers@postgresql.org>
Message-Id: <1db48c52-aa22-4e9a-a9ec-e80ad0129753@app.fastmail.com>
In-Reply-To: <D9AFF865-94BA-4875-8397-908EB2BF071E@gmail.com>
References: <6899c044-4a82-49be-8117-e6f669765f7e@app.fastmail.com>
 <165530.1752362320@sss.pgh.pa.us>
 <02a7cd37-e2fc-4212-8b19-f8c239c95fb8@app.fastmail.com>
 <e396ecf3-4227-4918-b9ff-e9568dcebcf0@app.fastmail.com>
 <b6a427fe-2ed6-4568-85d9-207a68172617@app.fastmail.com>
 <CAK80=jhmE40KVqQ3ho37MArS7cAED1p9m7uikDxcnDmqdW7t8A@mail.gmail.com>
 <96f00bf1-cc9d-4520-9d02-9e14e7767c88@app.fastmail.com>
 <30c2aa7d-dd6c-4b68-a2e4-f217a1a34acf@app.fastmail.com>
 <CA+hUKGLrMGkWDB0cwTa0RqD+AF7O-Ywgck8aVYKwOQnZgYRRug@mail.gmail.com>
 <0b4d402a-9ac2-4aa8-acf8-8231dbe579ea@app.fastmail.com>
 <c8e84bb2-d2e5-4a77-b547-3a93c5778e7e@app.fastmail.com>
 <3095599.1758644879@sss.pgh.pa.us>
 <0dc6a2cc-5216-4dc1-9dd2-430cafc6095b@app.fastmail.com>
 <E7D8A5CD-C445-4713-B6B7-6CC1D3A71161@gmail.com>
 <baf71275-9083-41be-ae13-f80821541a1a@app.fastmail.com>
 <52CC167F-763B-4ECA-B0B4-DAB381816828@gmail.com>
 <d35a114d-204a-4b40-9284-06749532b724@app.fastmail.com>
 <9186C6D0-F7A9-482A-9183-89E530B57E36@gmail.com>
 <da0996!!d8-ecf9-490d-bbd0-2a5bdb5b115a@app.fastmail.com>
 <D1CF7CA5-213A-4FC1-86DB-052D58904A0A@gmail.com>
 <cad78ff4-aae9-4c1c-8e16-ff0ccd4957f1@app.fastmail.com>
 <1073593.1759423179@sss.pgh.pa.us>
 <4bd5e6c4-6fa7-44bb-869d-59a32a331fa8@app.fastmail.com>
 <CAFY6G8dap-bCnAnMG-2Gzew8yv2Vbi9gsx9+yszKMmd57ygfvA@mail.gmail.com>
 <85828f29-e72e-4400-94f3-9a69bc8dc239@app.fastmail.com>
 <2495353.1759860890@sss.pgh.pa.us>
 <8aeae418-92a6-4bbd-9c06-9574c79e59f7@app.fastmail.com>
 <D5FC0377-6C73-48B0-B559-BC61FF90EC8A@gmail.com>
 <2790345f-03c3-4cae-8f14-886ed9079319@app.fastmail.com>
 <D9AFF865-94BA-4875-8397-908EB2BF071E@gmail.com>
Subject: Re: Optimize LISTEN/NOTIFY
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/1db48c52-aa22-4e9a-a9ec-e80ad0129753%40app.fastmail.com>
Precedence: bulk

On Thu, Oct 9, 2025, at 03:11, Chao Li wrote:
> I think you just reverted the usage of list_member() and makeNode(),=20
> but returned =E2=80=9Cchannels=E2=80=9D is still built by =E2=80=9Clap=
pend()=E2=80=9D that allocates=20
> memory for the List structure. So you need to use =E2=80=9Clist_free(c=
hannels)=E2=80=9D=20
> to free the memory.

Right. However, I'll see if I can make Tom's idea work of possibly remov=
ing the list form, instead.

>>> ```
>>> /*
>>> @@ -1865,6 +2087,7 @@ asyncQueueReadAllNotifications(void)
>>> LWLockAcquire(NotifyQueueLock, LW_SHARED);
>>> /* Assert checks that we have a valid state entry */
>>> Assert(MyProcPid =3D=3D QUEUE_BACKEND_PID(MyProcNumber));
>>> + QUEUE_BACKEND_WAKEUP_PENDING(MyProcNumber) =3D false;
>>> ```
>>>=20
>>> This piece of code originally only read the shared memory, so it can=20
>>> use LW_SHARED lock mode, but now it writes to the shared memory, do =
we=20
>>> need to change the lock mode to =E2=80=9Cexclusive=E2=80=9D?
>>=20
>> No, LW_SHARED is sufficient here, since the backend only modifies its=
 own state,
>> and no other backend could do that, without holding an exclusive lock.
>
> Yes, the backend only modifies its own state to =E2=80=9Cfalse=E2=80=9D=
, but other=20
> backends may set its state to =E2=80=9Ctrue=E2=80=9D, that is a race c=
ondition. So I=20
> still think an exclusive lock is needed.

No, other backends cannot alter our state without holding an exclusive l=
ock,
and they cannot obtain an exclusive lock on our backend until we've rele=
ased
the shared lock we're holding.

>>> 6
>>> ```
>>> +static inline void
>>> +ChannelHashPrepareKey(ChannelHashKey *key, Oid dboid, const char *c=
hannel)
>>> +{
>>> + memset(key, 0, sizeof(ChannelHashKey));
>>> + key->dboid =3D dboid;
>>> + strlcpy(key->channel, channel, NAMEDATALEN);
>>> +}
>>> ```
>>>=20
>>> Do we really need the memset()? If =E2=80=9Cchannel=E2=80=9D is of l=
ength NAMEDATALEN,=20
>>> then it still results in a non-0 terminated key->channel; if channel=
 is=20
>>> shorter than NAMEDATALEN, strlcpy will auto add a tailing =E2=80=98\=
0=E2=80=99. I think=20
>>> previous code should have ensured length of channel should be less t=
han=20
>>> NAMEDATALEN.
>>=20
>> Yes, I think we need memset, since I fear that when the hash table ke=
ys
>> are compared, every byte of the struct might be inspected, so without
>> zero-initializing it, there could be unused bytes after the null
>> terminator, that could then cause logically identical keys to be wron=
gly
>> considered different.
>>=20
>> I haven't checked the implementation though, but my gut feeling says
>> it's better to be a bit paranoid here.
>
> The hash function channel_hash_func() is defined by your own code, it=20
> use strnlen() to get length of channel name, so that bytes after =E2=80=
=98\0=E2=80=99=20
> won=E2=80=99t be used.

No, the hash function is not used for comparison.
We're using the default dshash_memcmp for comparison:

```
/* parameters for the channel hash table */
static const dshash_parameters channelDSHParams =3D {
	sizeof(ChannelHashKey),
	sizeof(ChannelEntry),
	dshash_memcmp,
	channelHashFunc,
	dshash_memcpy,
	LWTRANCHE_NOTIFY_CHANNEL_HASH
};
```

Looking at its implementation, we can see it's using memcmp under the ho=
od:

```
/*
 * A compare function that forwards to memcmp.
 */
int
dshash_memcmp(const void *a, const void *b, size_t size, void *arg)
{
	return memcmp(a, b, size);
}
```

Here, the input parameter `size` comes from `sizeof(ChannelHashKey)`,
so it will include all bytes in the comparison.


> And I guess you missed comment 9:
>
> 9
> ```
> + int allocated_listeners; /* Allocated size of array */
> ```
>
> For =E2=80=9Csize=E2=80=9D here, I guess you meant =E2=80=9Clength=E2=80=
=9D, though =E2=80=9Csize=E2=80=9D also works,=20
> but usually =E2=80=9Csize=E2=80=9D means bytes occupied by an array an=
d =E2=80=9Clength=E2=80=9D means=20
> number of elements of an array. So, =E2=80=9Clength=E2=80=9D would be =
clearer here.

Agreed, will change.

> And I got a new comment for v12:
>
> 10
> ```
> + found =3D false;
> + foreach(q, channels)
> + {
> + char   *existing =3D (char *) lfirst(q);
> +
> + if (strcmp(existing, channel) =3D=3D 0)
> + {
> ```
>
> Might be safer to do =E2=80=9Cstrncmp(existing, channel, NAMEDATALEN)=E2=
=80=9D.

Good idea, will change.

/Joel