public inbox for [email protected]
help / color / mirror / Atom feedFrom: Chao Li <[email protected]>
To: Joel Jacobson <[email protected]>
Cc: Tom Lane <[email protected]>
Cc: Matheus Alcantara <[email protected]>
Cc: pgsql-hackers <[email protected]>
Subject: Re: Optimize LISTEN/NOTIFY
Date: Wed, 8 Oct 2025 11:43:09 +0800
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<CAK80=jhmE40KVqQ3ho37MArS7cAED1p9m7uikDxcnDmqdW7t8A@mail.gmail.com>
<[email protected]>
<[email protected]>
<CA+hUKGLrMGkWDB0cwTa0RqD+AF7O-Ywgck8aVYKwOQnZgYRRug@mail.gmail.com>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<CAFY6G8dap-bCnAnMG-2Gzew8yv2Vbi9gsx9+yszKMmd57ygfvA@mail.gmail.com>
<[email protected]>
<[email protected]>
<[email protected]>
After several rounds of reviewing, the code is already very good. I just got a few small comments:
> On Oct 8, 2025, at 03:26, Joel Jacobson <[email protected]> wrote:
>
>
> /Joel<optimize_listen_notify-v11.patch>
1
```
+ channels = GetPendingNotifyChannels();
+
LWLockAcquire(NotifyQueueLock, LW_EXCLUSIVE);
- for (ProcNumber i = QUEUE_FIRST_LISTENER; i != INVALID_PROC_NUMBER; i = QUEUE_NEXT_LISTENER(i))
+ foreach(lc, channels)
```
I don’t see where “channels” is freed. GetPendingNotifyChannels() creates a list of Nodes, both the list and Nodes the list points to should be freed.
2
```
+ foreach(lc, channels)
{
- int32 pid = QUEUE_BACKEND_PID(i);
- QueuePosition pos;
+ char *channel = strVal(lfirst(lc));
+ ChannelEntry *entry;
+ ProcNumber *listeners;
+ ChannelHashKey key;
- Assert(pid != InvalidPid);
- pos = QUEUE_BACKEND_POS(i);
- if (QUEUE_BACKEND_DBOID(i) == MyDatabaseId)
+ if (channel_hash == NULL)
+ entry = NULL;
+ else
```
I wonder whether or not “channel_hash” can be NULL here? Maybe possible if a channel is un-listened while the event is pending?
So, maybe add a comment here to explain the logic.
3
The same piece of code as 2.
I think the code can be optimized a little bit. First, we can initialize entry to NULL, then we don’t the if-else. Second, “key” is only used for dshash_find(), so it can defined where it is used.
foreach(lc, channels)
{
char *channel = strVal(lfirst(lc));
ChannelEntry *entry = NULL;
ProcNumber *listeners;
//ChannelHashKey key;
if (channel_hash != NULL)
{
ChannelHashKey key;
ChannelHashPrepareKey(&key, MyDatabaseId, channel);
entry = dshash_find(channel_hash, &key, false);
}
if (entry == NULL)
continue; /* No listeners registered for this channel */
4
```
+ if (signaled[i] || QUEUE_BACKEND_WAKEUP_PENDING(i))
+ continue;
```
I wonder if “signaled[i]” is a duplicate flag of "QUEUE_BACKEND_WAKEUP_PENDING(i)”?
I understand signaled is local, and QUEUE_BACKEND_WAKEUP_PENDING is in shared memory and may be set by other processes, but in local, when signaled[I] is set, QUEUE_BACKEND_WAKEUP_PENDING(i) is also set. And because of NotifyQueueLock, other process should not be able to cleanup the flag.
But if “signals” is really needed, maybe we can use Bitmapset (src/backend/nodes/bitmapset.c), that would use 1/8 of memories comparing to the bool array.
5
```
/*
@@ -1865,6 +2087,7 @@ asyncQueueReadAllNotifications(void)
LWLockAcquire(NotifyQueueLock, LW_SHARED);
/* Assert checks that we have a valid state entry */
Assert(MyProcPid == QUEUE_BACKEND_PID(MyProcNumber));
+ QUEUE_BACKEND_WAKEUP_PENDING(MyProcNumber) = false;
```
This piece of code originally only read the shared memory, so it can use LW_SHARED lock mode, but now it writes to the shared memory, do we need to change the lock mode to “exclusive”?
6
```
+static inline void
+ChannelHashPrepareKey(ChannelHashKey *key, Oid dboid, const char *channel)
+{
+ memset(key, 0, sizeof(ChannelHashKey));
+ key->dboid = dboid;
+ strlcpy(key->channel, channel, NAMEDATALEN);
+}
```
Do we really need the memset()? If “channel” is of length NAMEDATALEN, then it still results in a non-0 terminated key->channel; if channel is shorter than NAMEDATALEN, strlcpy will auto add a tailing ‘\0’. I think previous code should have ensured length of channel should be less than NAMEDATALEN.
7
```
*
* Resist the temptation to make this really large. While that would save
* work in some places, it would add cost in others. In particular, this
@@ -246,6 +280,7 @@ typedef struct QueueBackendStatus
Oid dboid; /* backend's database OID, or InvalidOid */
ProcNumber nextListener; /* id of next listener, or INVALID_PROC_NUMBER */
QueuePosition pos; /* backend has read queue up to here */
+ bool wakeup_pending; /* signal sent but not yet processed */
} QueueBackendStatus;
```
In the same structure, rest of fields are all in camel case, I think it’s better to rename the new field to “wakeupPending”.
8
```
@@ -288,11 +323,91 @@ typedef struct AsyncQueueControl
ProcNumber firstListener; /* id of first listener, or
* INVALID_PROC_NUMBER */
TimestampTz lastQueueFillWarn; /* time of last queue-full msg */
+ dsa_handle channel_hash_dsa;
+ dshash_table_handle channel_hash_dsh;
QueueBackendStatus backend[FLEXIBLE_ARRAY_MEMBER];
```
Same as 7, but in this case, type names are not camel case, maybe okay for field names. I don’t have a strong opinion here.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
reply
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: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected]
Subject: Re: Optimize LISTEN/NOTIFY
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox