public inbox for [email protected]  

help / color / mirror / Atom feed

From: Joel Jacobson <[email protected]>
To: Chao Li <[email protected]>
To: Tom Lane <[email protected]>
Cc: pgsql-hackers <[email protected]>
Subject: Re: Optimize LISTEN/NOTIFY
Date: Wed, 15 Oct 2025 17:36:11 +0200
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<CAK80=jhmE40KVqQ3ho37MArS7cAED1p9m7uikDxcnDmqdW7t8A@mail.gmail.com>
	<[email protected]>
	<[email protected]>
	<CA+hUKGLrMGkWDB0cwTa0RqD+AF7O-Ywgck8aVYKwOQnZgYRRug@mail.gmail.com>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<CAFY6G8dap-bCnAnMG-2Gzew8yv2Vbi9gsx9+yszKMmd57ygfvA@mail.gmail.com>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>

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

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected]
  Subject: Re: Optimize LISTEN/NOTIFY
  In-Reply-To: <[email protected]>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox