From: | "Joel Jacobson" <joel(at)compiler(dot)org> |
---|---|
To: | "Tom Lane" <tgl(at)sss(dot)pgh(dot)pa(dot)us>, "Arseniy Mukhin" <arseniy(dot)mukhin(dot)dev(at)gmail(dot)com> |
Cc: | pgsql-hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Optimize LISTEN/NOTIFY |
Date: | 2025-10-16 09:39:02 |
Message-ID: | 3ef9b541-e835-4dc8-8f7a-b76677f17b36@app.fastmail.com |
Views: | Whole Thread | Raw Message | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Wed, Oct 15, 2025, at 16:16, Tom Lane wrote:
> Arseniy Mukhin <arseniy(dot)mukhin(dot)dev(at)gmail(dot)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've implemented the three ideas presented below, attached as .txt files
that are diffs on top of v19, which has these changes since v17:
0002-optimize_listen_notify-v19.patch:
* Improve wording of top comment per request from Chao Li.
* Add initChannelHash call to top of SignalBackends,
to fix bug reported by Arseniy Mukhin.
> 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.
Above idea is implemented in 0002-optimize_listen_notify-v19-alt1.txt
> 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).
Above idea is implemented in 0002-optimize_listen_notify-v19-alt2.txt
> 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.
Above idea is implemented in 0002-optimize_listen_notify-v19-alt3.txt
/Joel
Attachment | Content-Type | Size |
---|---|---|
0001-optimize_listen_notify-v19.patch | application/octet-stream | 9.3 KB |
0002-optimize_listen_notify-v19.patch | application/octet-stream | 31.3 KB |
0002-optimize_listen_notify-v19-alt1.txt | text/plain | 3.9 KB |
0002-optimize_listen_notify-v19-alt3.txt | text/plain | 7.4 KB |
0002-optimize_listen_notify-v19-alt2.txt | text/plain | 3.2 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Xuneng Zhou | 2025-10-16 09:39:20 | Re: pgstattuple: Use streaming read API in pgstatindex functions |
Previous Message | Mircea Cadariu | 2025-10-16 09:27:25 | Re: [BUG] temporary file usage report with extended protocol and unnamed portals |