public inbox for [email protected]
help / color / mirror / Atom feedFrom: Joel Jacobson <[email protected]>
To: Chao Li <[email protected]>
Cc: Tom Lane <[email protected]>
Cc: Matheus Alcantara <[email protected]>
Cc: pgsql-hackers <[email protected]>
Subject: Re: Optimize LISTEN/NOTIFY
Date: Thu, 09 Oct 2025 10:07:21 +0200
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]>
<[email protected]>
<[email protected]>
<[email protected]>
On Thu, Oct 9, 2025, at 03:11, Chao Li wrote:
> 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.
Right. However, I'll see if I can make Tom's idea work of possibly removing 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 == 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.
No, other backends cannot alter our state without holding an exclusive lock,
and they cannot obtain an exclusive lock on our backend until we've released
the shared lock we're holding.
>>> 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.
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 = {
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 hood:
```
/*
* 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 “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.
Agreed, will change.
> 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)”.
Good idea, will change.
/Joel
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