Re: Optimize LISTEN/NOTIFY

On Wed, Oct 8, 2025, at 20:46, Tom Lane wrote:
> "Joel Jacobson" <joel@compiler.org> writes:
>> On Tue, Oct 7, 2025, at 22:15, Tom Lane wrote:
>>> 5. ChannelHashAddListener: "already registered" case is not reached,
>>> which surprises me a bit, and neither is the "grow the array" stanza.
>
>> I've added a test for the "grow the array" stanza.
>
>> The "already registered" case seems impossible to reach, since the
>> caller, Exec_ListenCommit, returns early if IsListeningOn.
>
> Maybe we should remove the check for "already registered" then,
> or reduce it to an Assert?  Seems pointless to check twice.
>
> Or thinking a little bigger: why are we maintaining the set of
> channels-listened-to both as a list and a hash?  Could we remove
> the list form?

Yes, it was indeed possible to remove the list form.

Some functions got a bit more complex, but I think it's worth it since a
single source of truth seems like an important design goal.

This also made LISTEN faster when a backend is listening on plenty of
channels, since we can now lookup the channel in the hash, instead of
having to go through the list as before. The additional linear scan of
the listenersArray didn't add any noticeable extra cost even with
thousands of listening backends for the channel.

I also tried to keep listenersArray sorted and binary-search it, but
even with thousands of listening backends, I couldn't measure any
overall latency difference of LISTEN, so I kept the linear scan to keep
it simple.

In Exec_ListenCommit, I've now inlined code that is similar to
IsListeningOn. I didn't want to use IsListeningOn since it felt wasteful
having to do dshash_find, when we instead can just use
dshash_find_or_insert, to handle both cases.

I also added a static int numChannelsListeningOn variable, to avoid the
possibly expensive operation of going through the entire hash, to be
able to check `numChannelsListeningOn == 0` instead of the now removed
`listenChannels == NIL`. It's of course critical to keep
numChannelsListeningOn in sync, but I think it should be safe? Would of
course be better to avoid this variable. Maybe the extra cycles that
would cost would be worth it?

/Joel