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
