> On Oct 8, 2025, at 22:53, Joel Jacobson <[email protected]> wrote:
>
>> 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.
>
> Per suggestion from Tom Lane I reverted back GetPendingNotifyChannels(),
> so this comment is not applicable any longer.
I think you just reverted the usage of list_member() and makeNode(), but
returned “channels” is still built by “lappend()” that allocates memory for the
List structure. So you need to use “list_free(channels)” to free the memory.
>> 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”?
>
> 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 “false”, but other backends may
set its state to “true”, that is a race condition. So I still think an
exclusive lock is needed.
>
>> 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.
>
> Yes, I think we need memset, since I fear that when the hash table keys
> 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 wrongly
> considered different.
>
> 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 use
strnlen() to get length of channel name, so that bytes after ‘\0’ won’t be
used.
And I guess you missed comment 9:
9
```
+ int allocated_listeners; /* Allocated size of
array */
```
For “size” here, I guess you meant “length”, though “size” also works, but
usually “size” means bytes occupied by an array and “length” means number of
elements of an array. So, “length” would be clearer here.
And I got a new comment for v12:
10
```
+ found = false;
+ foreach(q, channels)
+ {
+ char *existing = (char *) lfirst(q);
+
+ if (strcmp(existing, channel) == 0)
+ {
```
Might be safer to do “strncmp(existing, channel, NAMEDATALEN)”.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
