Re: Optimize LISTEN/NOTIFY

On Tue, Oct 7, 2025, at 14:40, Matheus Alcantara wrote:
> This is not a complete review, I just read the v9 patch and summarized
> some points.

Many thanks for the review!

> 1. You may want to add ChannelEntry and ChannelHashKey to typedefs.list
> to get pgindent do the right job on indentation.

Fixed.

> 2. The ListCell* variables are normally named as lc
> +       ListCell   *p;

I agree, better to be consistent. I renamed the variables this patch
adds, but I didn't change the existing ListCell *p variables in async.c.
Would we want to harmonize it to just *lc everywhere in async.c?
I notice we also use ListCell *l in async.c at some places.

> 3. This block on ChannelHashRemoveListener() seems contradictory. You
> early return if channel_hash == NULL and then call init_channel_hash
> that it will early return if channel_hash != NULL. So if channel_hash !=
> NULL I don't think that we need to call init_channel_hash()?
> +       if (channel_hash == NULL)
> +               return;
> +
> +       init_channel_hash();
>
> A similar check also exists on SignalBackends()
>     if (channel_hash == NULL)
>         ...
>     else
>     {
>         // channel_hash is != NULL, so init_channel_hash will early
>         // return.
>         init_channel_hash();
>         ...
>     }

Ahh, right, I agree. I've removed the unnecessary init_channel_hash()
calls.

> 4. The ChannelHashRemoveListener() release lock logic could be
> simplified to something like the following, what do you think?
> +                       if (entry->num_listeners == 0)
> +                       {
> +                               dsa_free(channel_dsa, entry->listeners_array);
> +                               dshash_delete_entry(channel_hash, entry);
> +                       }
> +                       break;
> +               }
> +       }
> +
> +       /* Not found in list */
> +       dshash_release_lock(channel_hash, entry);

That would be nicer, but I noted that dshash_delete_entry() releases the
lock just like dshash_release_lock(), so then I think we would need to
return; after dshash_delete_entry(), to prevent attempting to release
the lock twice?

> 5. You may want to use list_member() on GetPendingNotifyChannels() to
> avoid the inner loop to check for duplicate channel names.

Ahh, much nicer! Fixed.

> 6. s/beind/behind
> +                       /* Need to signal if a backend has fallen too
> far beind */

Fixed.

> 7. I'm wondering if we could add some TAP tests for this? I think that
> adding a case to ensure that we can grown the dshash correctly and also
> we manage multiple backends to the same channel properly. This CF [1]
> has some examples of how TAP tests can be created to test LISTEN/NOTIFY

I will look over the tests. Maybe we should add some elog DEBUG at the
new code paths, and ensure the tests at least cover all of them?

/Joel