Arseniy Mukhin <arseniy.mukhin.dev@gmail.com> writes:
> I think "Direct advancement" is a good idea. But the way it's
> implemented now has a concurrency bug. Listeners store its current
> position in the local variable 'pos' during the reading in
> asyncQueueReadAllNotifications() and don't hold NotifyQueueLock. It
> means that some notifier can directly advance the listener's position
> while the listener has an old value in the local variable. The same
> time we use listener positions to find out the limit we can truncate
> the queue in asyncQueueAdvanceTail().
Good catch!
I think we can perhaps salvage the idea if we invent a separate
"advisory" queue position field, which tells its backend "hey,
you could skip as far as here if you want", but is not used for
purposes of SLRU truncation. Alternatively, split the queue pos
into "this is where to read next" and "this is as much as I'm
definitively done with", where the second field gets advanced at
the end of asyncQueueReadAllNotifications. Not sure which
view would be less confusing (in the end I guess they're nearly
the same thing, differently explained).
A different line of thought could be to get rid of
asyncQueueReadAllNotifications's optimization of moving the
queue pos only once, per
* (We could alternatively retake NotifyQueueLock and move the position
* before handling each individual message, but that seems like too much
* lock traffic.)
Since we only need shared lock to advance our own queue pos,
maybe that wouldn't be too awful. Not sure.
regards, tom lane
