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

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.

The elephant in the room of course is that a lookup in a linked list is
O(n) and it would be very straightforward to replace it with e.g. a hash
table. We should do that irrespective of this bug fix. But I'm inclined
to do it as a separate followup patch.

- Heikki