On Mon Oct 6, 2025 at 5:11 PM -03, Joel Jacobson wrote:
> The patch is now using dshash. I've been looking at code in launcher.c
> when implementing it. The function init_channel_hash() ended up being
> very similar to launcher.c's logicalrep_launcher_attach_dshmem().
>
Hi,
This is not a complete review, I just read the v9 patch and summarized
some points.
1. You may want to add ChannelEntry and ChannelHashKey to typedefs.list
to get pgindent do the right job on indentation.
2. The ListCell* variables are normally named as lc
+ ListCell *p;
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();
...
}
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);
5. You may want to use list_member() on GetPendingNotifyChannels() to
avoid the inner loop to check for duplicate channel names.
6. s/beind/behind
+ /* Need to signal if a backend has fallen too
far beind */
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
[1] https://commitfest.postgresql.org/patch/6095/
--
Matheus Alcantara
