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/
