Re: Optimize LISTEN/NOTIFY

From: Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com>
To: Joel Jacobson <joel(at)compiler(dot)org>
Cc: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Matheus Alcantara <matheusssilv97(at)gmail(dot)com>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Optimize LISTEN/NOTIFY
Date: 2025-10-09 08:39:43
Message-ID: D5ACBB76-9367-4DC8-AF0F-F844B6028628@gmail.com
Views: Whole Thread | Raw Message | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

> On Oct 9, 2025, at 16:07, Joel Jacobson <joel(at)compiler(dot)org> wrote:
>
>>>> ```
>>>> /*
>>>> @@ -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.
>

Ah… That’s true. This comment is resolved.

>>>>
>>
>> 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.
>

Okay, I think I misunderstood hash_function. So, this comment is also resolved.

I am thinking loudly. When a hash key is created, it has been memset to 0, meaning that in key->channel, all bytes after ‘\0’ are also 0, there should not be any random bytes in hash key, so that in channelHashFunc(), we don’t need to to use strnlen() anymore, which improves performance a little bit. Like this:

h = DatumGetUInt32(hash_uint32(k->dboid));
h ^= DatumGetUInt32(hash_any((const unsigned char *) k->channel,
sizeof(k->channel)));

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Rahila Syed 2025-10-09 08:43:25 Re: Enhancing Memory Context Statistics Reporting
Previous Message Amit Kapila 2025-10-09 08:16:28 Re: Logical Replication of sequences