Re: Optimize LISTEN/NOTIFY

On Tue, Oct 14, 2025, at 23:19, Tom Lane wrote:
> "Joel Jacobson" <joel@compiler.org> writes:
>> Having investigated this, the "direct advancement" approach seems
>> correct to me.
>
>> (I understand the exclusive lock in PreCommit_Notify on NotifyQueueLock
>> is of course needed because there are other operations that don't
>> acquire the heavyweight-lock, that take shared/exclusive lock on
>> NotifyQueueLock to read/modify QUEUE_HEAD, so the exclusive lock on
>> NotifyQueueLock in PreCommit_Notify is needed, since it modifies the
>> QUEUE_HEAD.)
>
> Right.  What the heavyweight lock buys for us in this context is that
> we can be sure no other would-be notifier can insert any messages
> in between ours, even though we may take and release NotifyQueueLock
> several times to allow readers to sneak in.  That in turn means that
> it's safe to advance readers over that whole set of messages if we
> know we didn't wake them up for any of those messages.

Right.

> There is a false-positive possibility if a reader was previously
> signaled but hasn't yet awoken: we will think that maybe we signaled
> it and hence not advance its pointer.  This is an error in the safe
> direction however, and it will advance its pointer when it does
> wake up.

I've added a comment on this in SignalBackends.

> A potential complaint is that we are doubling down on the need for
> that heavyweight lock, despite the upthread discussion about maybe
> getting rid of it for better scalability.  However, this patch
> only requires holding a lock across all the insertions, not holding
> it through commit which I think is the true scalability blockage.
>
> If we did want to get rid of that lock, we'd only need to stop
> releasing NotifyQueueLock at insertion page boundary crossings,
> which I suspect isn't really that useful anyway.

Right. So if the upthread discussion would get rid of the heavyweight
lock we would just need to hold the exclusive lock across all
insertions. Good to know the two efforts are not conflicting.

> (In connection
> with that though, I think you ought to capture both the "before" and
> "after" pointers within that lock interval, not expend another lock
> acquisition later.)

Fixed.

> It would be good if the patch's comments made these points ...

I've added a comment inside PreCommit_Notify on how it would suffice to
hold the exclusive lock across all insertions, for the purpose of
setting the "before" and "after" pointers, if the heavyweight lock would
be removed.

> also, the comments above struct AsyncQueueControl need to be
> updated, because changing some other backend's queue pos is
> not legal under any of the stated rules.

Fixed.

>> Given all the experiments since my earlier message, here is a fresh,
>> self-contained write-up:
>
> I'm getting itchy about removing the local listenChannels list,
> because what you've done is to replace it with a shared data
> structure that can't be accessed without a good deal of locking
> overhead.  That seems like it could easily be a net loss.

I agree, I also prefer the local listenChannels list.
I've changed it back.

> Also, I really do not like this implementation of
> GetPendingNotifyChannels, as it looks like O(N^2) effort.
> The potentially large length of the list it builds is scary too,
> considering the comments that SignalBackends had better not fail.
> If we have to do it that way it'd be better to collect the list
> during PreCommit_Notify.

I agree. I've removed GetPendingNotifyChannels and added a local list,
named pendingNotifyChannels instead, collected during PreCommit_Notify.

> The "Avoid needing to wake listening backends" loop should probably
> be combined with the loop after it; I don't quite see the point of
> iterating over all the listening backends twice.

I agree. Fixed.

> Also, why is the
> second loop only paying attention to backends in the same DB?

Fixed. (We're already sure it's the same DB, since that's part of the
hash key. I've removed the redundant check.)

> I don't love adding queueHeadBeforeWrite and queueHeadAfterWrite into
> the pendingNotifies data structure, as they have no visible connection
> to that.  In particular, we will have multiple NotificationList
> structs when there's nested transactions, and it's certainly
> meaningless to have such fields in more than one place.  Probably
> just making them independent static variables is the best way.

Fixed.

> The overall layout of what the patch injects where needs another
> look.  I don't like inserting code before typedefs and static
> variables within a module: that's not our normal layout style.

Fixed.

/Joel