Re: LISTEN/NOTIFY bug: VACUUM sets frozenxid past a xid in async queue

Hi,

On Tue, Nov 4, 2025 at 3:10 PM Heikki Linnakangas <hlinnaka(at)iki(dot)fi> wrote:
>
> On 03/11/2025 23:45, Joel Jacobson wrote:
> > On Mon, Nov 3, 2025, at 12:02, Heikki Linnakangas wrote:
> >> I wrote another little stand-alone performance test program for this,
> >> attached. It launches N connections that send NOTIFYs to a single
> >> channel as fast as possible, and M threads that listen for the
> >> notifications. I ran it with different combinations of N and M, on
> >> 'master' and on REL_14_STABLE (which didn't have SLRU banks) and I
> >> cannot discern any performance difference from these patches. So it
> >> seems that holding the SLRU (bank) lock across the
> >> TransactionIdDidCommit() calls is fine.
> >
> > Nice! That for the benchmark code! I took the liberty of hacking a bit
> > on it, and added support for multiple channels, with separate listener
> > and notifier threads per channel. Each notification now carries the
> > notifier ID, a sequence number, and a send timestamp. Listeners verify
> > that sequence numbers arrive in order and record delivery latency. The
> > program collects latency measurements into fixed buckets and reports
> > them once per second together with total and per-second send/receive
> > counts.
> >
> > Also added a short delay before starting notifiers so that listeners
> > have time to issue their LISTEN commands, and a new --channels option,
> > and the meaning of --listeners and --notifiers was changed to apply per
> > channel.
> >
> > Also fixed so the code could be compiled outside of the PostgreSQL
> > source code repo, if wanting to build this as stand-alone tool.
> >
> > I've benchmarked master vs 0001+0002 and can't notice any differences;
> > see attached output from benchmark runs.
>
> Thanks. After some further testing, I was able to find a scenario where
> this patch significantly reduces performance: if the listening backends
> subscribe to a massive number of channels, like 10000, they spend a lot
> of time scanning the linked list of subscribed channels in
> IsListeningOn(). With the patch, those checks were performed while
> holding the SLRU lock, and it started to show up as lock contention
> between notifiers and listeners. To demonstrate that, attached is
> another version of the test program that adds an --extra-channels=N
> argument. If you set it to e.g. 10000, each listener backends calls
> LISTEN on 10000 additional channels that are never notified. They just
> make the listenChannels list longer. With that and the patches I posted
> previously, I'm getting:
>
> $ PGHOST=localhost PGDB=postgres://localhost/postgres
> ./async-notify-test-3 --listeners=50 --notifiers=4 --channels=1
> --extra-channels=10000
> 10 s: 12716 sent (1274/s), 635775 received (63737/s)
> 0.00-0.01ms 0 (0.0%) avg: 0.000ms
> 0.01-0.10ms 0 (0.0%) avg: 0.000ms
> 0.10-1.00ms # 1915 (0.3%) avg: 0.807ms
> 1.00-10.00ms ######### 633550 (99.7%) avg: 3.502ms
> 10.00-100.00ms # 310 (0.0%) avg: 11.423ms
> >100.00ms 0 (0.0%) avg: 0.000ms
> ^C
>
> Whereas on 'master', I see about 2-3x more notifies/s:
>
> $ PGHOST=localhost PGDB=postgres://localhost/postgres
> ./async-notify-test-3 --listeners=50 --notifiers=4 --channels=1
> --extra-channels=10000
> 10 s: 32057 sent (3296/s), 1602995 received (164896/s)
> 0.00-0.01ms 0 (0.0%) avg: 0.000ms
> 0.01-0.10ms # 11574 (0.7%) avg: 0.078ms
> 0.10-1.00ms ###### 1082960 (67.6%) avg: 0.577ms
> 1.00-10.00ms ### 508199 (31.7%) avg: 1.489ms
> 10.00-100.00ms # 262 (0.0%) avg: 16.178ms
> >100.00ms 0 (0.0%) avg: 0.000ms
> ^C
>
> Fortunately that's easy to fix: We can move the IsListeningOn() check
> after releasing the lock. See attached.
>

Thank you for working on this!

It seems that 0002 handles errors during NotifyMyFrontEnd a little differently.

With master, in case of a failure during NotifyMyFrontEnd, the
listener's position in PG_FINALLY is set to the beginning of the next
notification, since we advance the "current" position only if the
previous notification was successfully sent.

With 0002, we advance the "current" position while copying
notifications to the local buffer, and begin sending them after the
position has already been advanced for all copied notifications. So in
case of a failure, the listener's position in PG_FINALLY is set to the
beginning of the next page or queue head. This means we can lose
notifications that were copied but were not sent.

If we want to preserve the previous behavior, maybe we could use a new
local position while copying notifications and only advance the
"current" position while sending notifications to the frontend?

Best regards,
Arseniy Mukhin