Re: Get rid of WALBufMappingLock

Hi,

I've briefly looked at this patch this week, and done a bit of testing.
I don't have any comments about the correctness - it does seem correct
to me and I haven't noticed any crashes/issues, but I'm not familiar
with the WALBufMappingLock enough to have insightful opinions.

I have however decided to do a bit of benchmarking, to better understand
the possible benefits of the change. I happen to have access to an Azure
machine with 2x AMD EPYC 9V33X (176 cores in total), and NVMe SSD that
can do ~1.5GB/s.

The benchmark script (attached) uses the workload mentioned by Andres
some time ago [1]

SELECT pg_logical_emit_message(true, 'test', repeat('0', $SIZE));

with clients (1..196) and sizes 8K, 64K and 1024K. The aggregated
results look like this (this is throughput):

| 8 | 64 | 1024
clients | master patched | master patched | master patched
---------------------------------------------------------------------
1 | 11864 12035 | 7419 7345 | 968 940
4 | 26311 26919 | 12414 12308 | 1304 1293
8 | 38742 39651 | 14316 14539 | 1348 1348
16 | 57299 59917 | 15405 15871 | 1304 1279
32 | 74857 82598 | 17589 17126 | 1233 1233
48 | 87596 95495 | 18616 18160 | 1199 1227
64 | 89982 97715 | 19033 18910 | 1196 1221
96 | 92853 103448 | 19694 19706 | 1190 1210
128 | 95392 103324 | 20085 19873 | 1188 1213
160 | 94933 102236 | 20227 20323 | 1180 1214
196 | 95933 103341 | 20448 20513 | 1188 1199

To put this into a perspective, this throughput relative to master:

clients | 8 64 1024
----------------------------------
1 | 101% 99% 97%
4 | 102% 99% 99%
8 | 102% 102% 100%
16 | 105% 103% 98%
32 | 110% 97% 100%
48 | 109% 98% 102%
64 | 109% 99% 102%
96 | 111% 100% 102%
128 | 108% 99% 102%
160 | 108% 100% 103%
196 | 108% 100% 101%

That does not seem like a huge improvement :-( Yes, there's 1-10%
speedup for the small (8K) size, but for larger chunks it's a wash.

Looking at the pgbench progress, I noticed stuff like this:

...
progress: 13.0 s, 103575.2 tps, lat 0.309 ms stddev 0.071, 0 failed
progress: 14.0 s, 102685.2 tps, lat 0.312 ms stddev 0.072, 0 failed
progress: 15.0 s, 102853.9 tps, lat 0.311 ms stddev 0.072, 0 failed
progress: 16.0 s, 103146.0 tps, lat 0.310 ms stddev 0.075, 0 failed
progress: 17.0 s, 57168.1 tps, lat 0.560 ms stddev 0.153, 0 failed
progress: 18.0 s, 50495.9 tps, lat 0.634 ms stddev 0.060, 0 failed
progress: 19.0 s, 50927.0 tps, lat 0.628 ms stddev 0.066, 0 failed
progress: 20.0 s, 50986.7 tps, lat 0.628 ms stddev 0.062, 0 failed
progress: 21.0 s, 50652.3 tps, lat 0.632 ms stddev 0.061, 0 failed
progress: 22.0 s, 63792.9 tps, lat 0.502 ms stddev 0.168, 0 failed
progress: 23.0 s, 103109.9 tps, lat 0.310 ms stddev 0.072, 0 failed
progress: 24.0 s, 103503.8 tps, lat 0.309 ms stddev 0.071, 0 failed
progress: 25.0 s, 101984.2 tps, lat 0.314 ms stddev 0.073, 0 failed
progress: 26.0 s, 102923.1 tps, lat 0.311 ms stddev 0.072, 0 failed
progress: 27.0 s, 103973.1 tps, lat 0.308 ms stddev 0.072, 0 failed
...

i.e. it fluctuates a lot. I suspected this is due to the SSD doing funny
things (it's a virtual SSD, I'm not sure what model is that behind the
curtains). So I decided to try running the benchmark on tmpfs, to get
the storage out of the way and get the "best case" results.

This makes the pgbench progress perfectly "smooth" (no jumps like in the
output above), and the comparison looks like this:

| 8 | 64 | 1024
clients | master patched | master patched | master patched
---------|---------------------|--------------------|----------------
1 | 32449 32032 | 19289 20344 | 3108 3081
4 | 68779 69256 | 24585 29912 | 2915 3449
8 | 79787 100655 | 28217 39217 | 3182 4086
16 | 113024 148968 | 42969 62083 | 5134 5712
32 | 125884 170678 | 44256 71183 | 4910 5447
48 | 125571 166695 | 44693 76411 | 4717 5215
64 | 122096 160470 | 42749 83754 | 4631 5103
96 | 120170 154145 | 42696 86529 | 4556 5020
128 | 119204 152977 | 40880 88163 | 4529 5047
160 | 116081 152708 | 42263 88066 | 4512 5000
196 | 115364 152455 | 40765 88602 | 4505 4952

and the comparison to master:

clients 8 64 1024
-----------------------------------------
1 99% 105% 99%
4 101% 122% 118%
8 126% 139% 128%
16 132% 144% 111%
32 136% 161% 111%
48 133% 171% 111%
64 131% 196% 110%
96 128% 203% 110%
128 128% 216% 111%
160 132% 208% 111%
196 132% 217% 110%

Yes, with tmpfs the impact looks much more significant. For 8K the
speedup is ~1.3x, for 64K it's up to ~2x, for 1M it's ~1.1x.

That being said, I wonder how big is the impact for practical workloads.
ISTM this workload is pretty narrow / extreme, it'd be much easier if we
had an example of a more realistic workload, benefiting from this. Of
course, it may be the case that there are multiple related bottlenecks,
and we'd need to fix all of them - in which case it'd be silly to block
the improvements on the grounds that it alone does not help.

Another thought is that this is testing the "good case". Can anyone
think of a workload that would be made worse by the patch?

regards

--
Tomas Vondra