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

"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.

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.

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.  (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.)

It would be good if the patch's comments made these points ...
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.


> 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.

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.

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.  Also, why is the
second loop only paying attention to backends in the same DB?

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.

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.

            regards, tom lane