Re: Optimize LISTEN/NOTIFY - Mailing list pgsql-hackers

On Wed, Oct 15, 2025, at 05:19, Chao Li wrote:
>  * B enters PreCommit_Notify(), it gets the NotifyQueueLock first, it
> records headBeforeWrite = 1 and writes to 3, and records headAfterWrite
> = 3.
>  * Now QueueHead is 3.
>  * C enters PreCommit_Notify(),  it records headBeforeWrite = 3 and
> writes to 5, and records headAfterWrite = 5.

No, when C enters PreCommit_Notify, it will be waiting on the
heavyweight lock, currently held by B, which B will hold
until it commits. It will then see headBeforeWrite = 3.

>  * Now QueueHead is 5
>  * C starts to run AtCommit_Notify(), as A’s head is 1, doesn’t equal
> to C’s headBeforeWrite, C won’t advance A’s head.
>  * A starts to run AtCommit_Notify(), A’s head equals to B’s
> beforeHeadWrite, B will advance A’s head to 3.

No, like explained above, B cannot be running here,
it must have committed already (or aborted) since C
was waiting on the heavyweight lock held by B.

The example therefore seems invalid to me.

> I agree with Tom that GetPendingNotifyChannels() is too heavy and unnecessary.
>
> In PreCommit_Notify(), we can maintain a local hash table to record
> pending nofications’ channel names. dahash also supports hash table in
> local memory.

I'm confused, I assume you mean "dynahash" since there is no "dahash"
in the sources? I see dynahash has local-to-a-backend support,
but I don't see why we would need a hash table for this,
we just iterate over it once in SignalBackends,
I think the local list is fine.

The latest version gets rid of GetPendingNotifyChannels()
and replaces it with the local list pendingNotifyChannels.

> And the local static numChannelsListeningOn is also not needed. We can
> get the count from the local hash.

No, you're mixing up the data structures.
The local hash you suggested was for pending notify channels,
but numChannelsListeningOn was needed when we didn't have
listenChannels. Now that I've reverted back to listenChannels,
I also replaced `(numChannelsListeningOn == 0)`
with `(listenChannels == NIL)`.

> WRT to v6, I got a few new comments:
...
> In this comment, you refer to “channelHash” and “the shared channel
> hash table”, they are the same thing, but easy to make readers to
> misunderstand.

Right, will try to improve this in the next version.

>  pg_listening_channels(PG_FUNCTION_ARGS)
>  {
>  FuncCallContext *funcctx;
> + List   *listenChannels;
...
> listenChannels is only used within the “if”, so it’s definition can be
> moved into the “if”.

Comment not applicable since local variable listenChannels has now been
removed from pg_listening_channels, now using the original static
listenChannels instead.

> + /* Check for lagging backends when the queue spans multiple pages */
> + if (queue_length > 0)
...
> I wonder why this check is needed. If queue_length is 0, can we return
> immediately from SignalBackends()?

This check has been removed in the latest version.

/Joel