On Sat, Nov 8, 2025, at 13:59, Joel Jacobson wrote:
> On Fri, Nov 7, 2025, at 19:59, Joel Jacobson wrote:
>> * The logic in SignalBackends has been reworked and simplified,
>> thanks to the new isAdvancing and advancingPos fields.
>> I now think it's finally easy to reason about why each branch
>> in SignalBackends must be correct.
>
> I was wrong. I wrongly assumed asyncQueueReadAllNotifications would read
> up until head, which it might not actually do:
>
> * Process messages up to the stop position, end of page, or an
> * uncommitted message.
>
> This in turn could cause a listening backend to remain behind, if there
> would be no more notifies, so it unfortunately seems like we will always
> need to signal when a backend isAdvancing, and therefore have no use of
> the advancingPos field.
>
> I will do more correctness and benchmark testing before posting a new
> version. Just wanted to give you a heads up on the bug, so you don't
> waste time reviewing.
Changes since v24:
* Removed the QueuePosition advancingPos QueueBackendStatus field.
* Always signal when isAdvancing is true.
Some benchmarking:
master:
% ./pg_async_notify_test --listeners 1 --notifiers 1 --channels 1000 --sleep 0.1 --sleep-exp 1.01
13 s: 8668 sent (658/s), 8676 received (653/s)
Notification Latency Distribution:
0.00-0.01ms 0 (0.0%) avg: 0.000ms
0.01-0.10ms 0 (0.0%) avg: 0.000ms
0.10-1.00ms # 3 (0.0%) avg: 0.783ms
1.00-10.00ms # 49 (0.6%) avg: 5.559ms
10.00-100.00ms # 168 (1.9%) avg: 56.360mss
>100.00ms ######### 8456 (97.5%) avg: 256.086ms
v25:
% ./pg_async_notify_test --listeners 1 --notifiers 1 --channels 1000 --sleep 0.1 --sleep-exp 1.01
14 s: 27097 sent (1959/s), 27097 received (1960/s)
Notification Latency Distribution:
0.00-0.01ms 0 (0.0%) avg: 0.000ms
0.01-0.10ms # 962 (3.6%) avg: 0.081ms
0.10-1.00ms ######### 25066 (92.5%) avg: 0.321ms
1.00-10.00ms # 1069 (3.9%) avg: 2.104ms
10.00-100.00ms 0 (0.0%) avg: 0.000ms
>100.00ms 0 (0.0%) avg: 0.000ms
On master, I see lots of "waiting for AccessExclusiveLock on object 0"
in the logs for this benchmark setup, but none at all for v25.
I wonder if there would be some way to solve the problem with v24,
without having to always signal when isAdvancing is true with an
advancingPos at or ahead of the notifier's queueHeadAfterWrite. One idea
I had, that I think is a bad idea, but just mentioning it in case you
think it could work, is to check if the new pos is at head, in
asyncQueueReadAllNotifications's PG_FINALLY block, and if not, to let
the listening backend signal itself. I guess this is a bad idea, since
then it could signal itself over and over again, and consume a lot of
resources, if some uncommitted message takes a long time to commit. The
current mechanism limit the rate of wake-ups to the same rate as the
notifies, which seems sensible. Thoughts on this?
/Joel
