Thread
-
Get rid of WALBufMappingLock
Yura Sokolov <y.sokolov@postgrespro.ru> — 2025-01-19T00:11:32Z
Good day, hackers. During discussion of Increasing NUM_XLOGINSERT_LOCKS [1], Andres Freund used benchmark which creates WAL records very intensively. While I this it is not completely fair (1MB log records are really rare), it pushed me to analyze write-side waiting of XLog machinery. First I tried to optimize WaitXLogInsertionsToFinish, but without great success (yet). While profiling, I found a lot of time is spend in the memory clearing under global WALBufMappingLock: MemSet((char *) NewPage, 0, XLOG_BLCKSZ); It is obvious scalability bottleneck. So "challenge was accepted". Certainly, backend should initialize pages without exclusive lock. But which way to ensure pages were initialized? In other words, how to ensure XLogCtl->InitializedUpTo is correct. I've tried to play around WALBufMappingLock with holding it for a short time and spinning on XLogCtl->xlblocks[nextidx]. But in the end I found WALBufMappingLock is useless at all. Instead of holding lock, it is better to allow backends to cooperate: - I bound ConditionVariable to each xlblocks entry, - every backend now checks every required block pointed by InitializedUpto was successfully initialized or sleeps on its condvar, - when backend sure block is initialized, it tries to update InitializedUpTo with conditional variable. Andres's benchmark looks like: c=100 && install/bin/psql -c checkpoint -c 'select pg_switch_wal()' postgres && install/bin/pgbench -n -M prepared -c$c -j$c -f <(echo "SELECT pg_logical_emit_message(true, 'test', repeat('0', 1024*1024));";) -P1 -T45 postgres So, it generate 1M records as fast as possible for 45 seconds. Test machine is Ryzen 5825U (8c/16th) limited to 2GHz. Config: max_connections = 1000 shared_buffers = 1024MB fsync = off wal_sync_method = fdatasync full_page_writes = off wal_buffers = 1024MB checkpoint_timeout = 1d Results are: "average for 45 sec" /"1 second max outlier" Results for master @ d3d098316913 : 25 clients: 2908 /3230 50 clients: 2759 /3130 100 clients: 2641 /2933 200 clients: 2419 /2707 400 clients: 1928 /2377 800 clients: 1689 /2266 With v0-0001-Get-rid-of-WALBufMappingLock.patch : 25 clients: 3103 /3583 50 clients: 3183 /3706 100 clients: 3106 /3559 200 clients: 2902 /3427 400 clients: 2303 /2717 800 clients: 1925 /2329 Combined with v0-0002-several-attempts-to-lock-WALInsertLocks.patch No WALBufMappingLock + attempts on XLogInsertLock: 25 clients: 3518 /3750 50 clients: 3355 /3548 100 clients: 3226 /3460 200 clients: 3092 /3299 400 clients: 2575 /2801 800 clients: 1946 /2341 This results are with untouched NUM_XLOGINSERT_LOCKS == 8. [1] http://postgr.es/m/flat/3b11fdc2-9793-403d-b3d4-67ff9a00d447%40postgrespro.ru PS. Increasing NUM_XLOGINSERT_LOCKS to 64 gives: 25 clients: 3457 /3624 50 clients: 3215 /3500 100 clients: 2750 /3000 200 clients: 2535 /2729 400 clients: 2163 /2400 800 clients: 1700 /2060 While doing this on master: 25 clients 2645 /2953 50 clients: 2562 /2968 100 clients: 2364 /2756 200 clients: 2266 /2564 400 clients: 1868 /2228 800 clients: 1527 /2133 So, patched version with increased NUM_XLOGINSERT_LOCKS looks no worse than unpatched without increasing num of locks. ------- regards Yura Sokolov aka funny-falcon -
Re: Get rid of WALBufMappingLock
Yura Sokolov <y.sokolov@postgrespro.ru> — 2025-01-19T00:31:20Z
19.01.2025 03:11, Yura Sokolov пишет: > Good day, hackers. > > During discussion of Increasing NUM_XLOGINSERT_LOCKS [1], Andres Freund > used benchmark which creates WAL records very intensively. While I this > it is not completely fair (1MB log records are really rare), it pushed > me to analyze write-side waiting of XLog machinery. > > First I tried to optimize WaitXLogInsertionsToFinish, but without great > success (yet). > > While profiling, I found a lot of time is spend in the memory clearing > under global WALBufMappingLock: > > MemSet((char *) NewPage, 0, XLOG_BLCKSZ); > > It is obvious scalability bottleneck. > > So "challenge was accepted". > > Certainly, backend should initialize pages without exclusive lock. But > which way to ensure pages were initialized? In other words, how to > ensure XLogCtl->InitializedUpTo is correct. > > I've tried to play around WALBufMappingLock with holding it for a short > time and spinning on XLogCtl->xlblocks[nextidx]. But in the end I found > WALBufMappingLock is useless at all. > > Instead of holding lock, it is better to allow backends to cooperate: > - I bound ConditionVariable to each xlblocks entry, > - every backend now checks every required block pointed by > InitializedUpto was successfully initialized or sleeps on its condvar, > - when backend sure block is initialized, it tries to update > InitializedUpTo with conditional variable. > > Andres's benchmark looks like: > > c=100 && install/bin/psql -c checkpoint -c 'select pg_switch_wal()' > postgres && install/bin/pgbench -n -M prepared -c$c -j$c -f <(echo > "SELECT pg_logical_emit_message(true, 'test', repeat('0', > 1024*1024));";) -P1 -T45 postgres > > So, it generate 1M records as fast as possible for 45 seconds. > > Test machine is Ryzen 5825U (8c/16th) limited to 2GHz. > Config: > > max_connections = 1000 > shared_buffers = 1024MB > fsync = off > wal_sync_method = fdatasync > full_page_writes = off > wal_buffers = 1024MB > checkpoint_timeout = 1d > > Results are: "average for 45 sec" /"1 second max outlier" > > Results for master @ d3d098316913 : > 25 clients: 2908 /3230 > 50 clients: 2759 /3130 > 100 clients: 2641 /2933 > 200 clients: 2419 /2707 > 400 clients: 1928 /2377 > 800 clients: 1689 /2266 > > With v0-0001-Get-rid-of-WALBufMappingLock.patch : > 25 clients: 3103 /3583 > 50 clients: 3183 /3706 > 100 clients: 3106 /3559 > 200 clients: 2902 /3427 > 400 clients: 2303 /2717 > 800 clients: 1925 /2329 > > Combined with v0-0002-several-attempts-to-lock-WALInsertLocks.patch > > No WALBufMappingLock + attempts on XLogInsertLock: > 25 clients: 3518 /3750 > 50 clients: 3355 /3548 > 100 clients: 3226 /3460 > 200 clients: 3092 /3299 > 400 clients: 2575 /2801 > 800 clients: 1946 /2341 > > This results are with untouched NUM_XLOGINSERT_LOCKS == 8. > > [1] http://postgr.es/m/flat/3b11fdc2-9793-403d- > b3d4-67ff9a00d447%40postgrespro.ru > > > PS. > Increasing NUM_XLOGINSERT_LOCKS to 64 gives: > 25 clients: 3457 /3624 > 50 clients: 3215 /3500 > 100 clients: 2750 /3000 > 200 clients: 2535 /2729 > 400 clients: 2163 /2400 > 800 clients: 1700 /2060 > > While doing this on master: > 25 clients 2645 /2953 > 50 clients: 2562 /2968 > 100 clients: 2364 /2756 > 200 clients: 2266 /2564 > 400 clients: 1868 /2228 > 800 clients: 1527 /2133 > > So, patched version with increased NUM_XLOGINSERT_LOCKS looks no worse > than unpatched without increasing num of locks. I'm too brave... or too sleepy (it's 3:30am)... But I took the risk of sending a patch to commitfest: https://commitfest.postgresql.org/52/5511/ ------ regards Yura Sokolov aka funny-falcon -
Re: Get rid of WALBufMappingLock
Alexander Korotkov <aekorotkov@gmail.com> — 2025-02-06T22:26:04Z
Hi! On Sun, Jan 19, 2025 at 2:11 AM Yura Sokolov <y.sokolov@postgrespro.ru> wrote: > > During discussion of Increasing NUM_XLOGINSERT_LOCKS [1], Andres Freund > used benchmark which creates WAL records very intensively. While I this > it is not completely fair (1MB log records are really rare), it pushed > me to analyze write-side waiting of XLog machinery. > > First I tried to optimize WaitXLogInsertionsToFinish, but without great > success (yet). > > While profiling, I found a lot of time is spend in the memory clearing > under global WALBufMappingLock: > > MemSet((char *) NewPage, 0, XLOG_BLCKSZ); > > It is obvious scalability bottleneck. > > So "challenge was accepted". > > Certainly, backend should initialize pages without exclusive lock. But > which way to ensure pages were initialized? In other words, how to > ensure XLogCtl->InitializedUpTo is correct. > > I've tried to play around WALBufMappingLock with holding it for a short > time and spinning on XLogCtl->xlblocks[nextidx]. But in the end I found > WALBufMappingLock is useless at all. > > Instead of holding lock, it is better to allow backends to cooperate: > - I bound ConditionVariable to each xlblocks entry, > - every backend now checks every required block pointed by > InitializedUpto was successfully initialized or sleeps on its condvar, > - when backend sure block is initialized, it tries to update > InitializedUpTo with conditional variable. Looks reasonable for me, but having ConditionVariable per xlog buffer seems overkill for me. Find an attached revision, where I've implemented advancing InitializedUpTo without ConditionVariable. After initialization of each buffer there is attempt to do CAS for InitializedUpTo in a loop. So, multiple processes will try to advance InitializedUpTo, they could hijack initiative from each other, but there is always a leader which will finish the work. There is only one ConditionVariable to wait for InitializedUpTo being advanced. I didn't benchmark my version, just checked that tests passed. ------ Regards, Alexander Korotkov Supabase
-
Re: Get rid of WALBufMappingLock
Yura Sokolov <y.sokolov@postgrespro.ru> — 2025-02-07T11:02:48Z
07.02.2025 01:26, Alexander Korotkov пишет: > Hi! > > On Sun, Jan 19, 2025 at 2:11 AM Yura Sokolov <y.sokolov@postgrespro.ru> wrote: >> >> During discussion of Increasing NUM_XLOGINSERT_LOCKS [1], Andres Freund >> used benchmark which creates WAL records very intensively. While I this >> it is not completely fair (1MB log records are really rare), it pushed >> me to analyze write-side waiting of XLog machinery. >> >> First I tried to optimize WaitXLogInsertionsToFinish, but without great >> success (yet). >> >> While profiling, I found a lot of time is spend in the memory clearing >> under global WALBufMappingLock: >> >> MemSet((char *) NewPage, 0, XLOG_BLCKSZ); >> >> It is obvious scalability bottleneck. >> >> So "challenge was accepted". >> >> Certainly, backend should initialize pages without exclusive lock. But >> which way to ensure pages were initialized? In other words, how to >> ensure XLogCtl->InitializedUpTo is correct. >> >> I've tried to play around WALBufMappingLock with holding it for a short >> time and spinning on XLogCtl->xlblocks[nextidx]. But in the end I found >> WALBufMappingLock is useless at all. >> >> Instead of holding lock, it is better to allow backends to cooperate: >> - I bound ConditionVariable to each xlblocks entry, >> - every backend now checks every required block pointed by >> InitializedUpto was successfully initialized or sleeps on its condvar, >> - when backend sure block is initialized, it tries to update >> InitializedUpTo with conditional variable. > > Looks reasonable for me, but having ConditionVariable per xlog buffer > seems overkill for me. Find an attached revision, where I've > implemented advancing InitializedUpTo without ConditionVariable. > After initialization of each buffer there is attempt to do CAS for > InitializedUpTo in a loop. So, multiple processes will try to advance > InitializedUpTo, they could hijack initiative from each other, but > there is always a leader which will finish the work. > > There is only one ConditionVariable to wait for InitializedUpTo being advanced. > > I didn't benchmark my version, just checked that tests passed. Good day, Alexander. I've got mixed but quite close result for both approaches (single or many ConditionVariable) on the notebook. Since I have no access to larger machine, I can't prove "many" is way better (or discover it worse). Given patch after cleanup looks a bit smaller and clearer, I agree to keep just single condition variable. Cleaned version is attached. I've changed condition for broadcast a bit ("less" instead "not equal"): - buffer's border may already go into future, - and then other backend will reach not yet initialized buffer and will broadcast. ------- regards Yura Sokolov aka funny-falcon -
Re: Get rid of WALBufMappingLock
Yura Sokolov <y.sokolov@postgrespro.ru> — 2025-02-07T11:24:49Z
07.02.2025 14:02, Yura Sokolov пишет: > 07.02.2025 01:26, Alexander Korotkov пишет: >> Hi! >> >> On Sun, Jan 19, 2025 at 2:11 AM Yura Sokolov <y.sokolov@postgrespro.ru> wrote: >>> >>> During discussion of Increasing NUM_XLOGINSERT_LOCKS [1], Andres Freund >>> used benchmark which creates WAL records very intensively. While I this >>> it is not completely fair (1MB log records are really rare), it pushed >>> me to analyze write-side waiting of XLog machinery. >>> >>> First I tried to optimize WaitXLogInsertionsToFinish, but without great >>> success (yet). >>> >>> While profiling, I found a lot of time is spend in the memory clearing >>> under global WALBufMappingLock: >>> >>> MemSet((char *) NewPage, 0, XLOG_BLCKSZ); >>> >>> It is obvious scalability bottleneck. >>> >>> So "challenge was accepted". >>> >>> Certainly, backend should initialize pages without exclusive lock. But >>> which way to ensure pages were initialized? In other words, how to >>> ensure XLogCtl->InitializedUpTo is correct. >>> >>> I've tried to play around WALBufMappingLock with holding it for a short >>> time and spinning on XLogCtl->xlblocks[nextidx]. But in the end I found >>> WALBufMappingLock is useless at all. >>> >>> Instead of holding lock, it is better to allow backends to cooperate: >>> - I bound ConditionVariable to each xlblocks entry, >>> - every backend now checks every required block pointed by >>> InitializedUpto was successfully initialized or sleeps on its condvar, >>> - when backend sure block is initialized, it tries to update >>> InitializedUpTo with conditional variable. >> >> Looks reasonable for me, but having ConditionVariable per xlog buffer >> seems overkill for me. Find an attached revision, where I've >> implemented advancing InitializedUpTo without ConditionVariable. >> After initialization of each buffer there is attempt to do CAS for >> InitializedUpTo in a loop. So, multiple processes will try to advance >> InitializedUpTo, they could hijack initiative from each other, but >> there is always a leader which will finish the work. >> >> There is only one ConditionVariable to wait for InitializedUpTo being advanced. >> >> I didn't benchmark my version, just checked that tests passed. > > Good day, Alexander. Seems I was mistaken twice. > I've got mixed but quite close result for both approaches (single or many > ConditionVariable) on the notebook. Since I have no access to larger > machine, I can't prove "many" is way better (or discover it worse). 1. "many condvars" (my variant) is strictly worse with num locks = 64 and when pg_logical_emit_message emits just 1kB instead of 1MB. Therefore, "single condvar" is strictly better. > Given patch after cleanup looks a bit smaller and clearer, I agree to keep > just single condition variable. > > Cleaned version is attached. > > I've changed condition for broadcast a bit ("less" instead "not equal"): > - buffer's border may already go into future, > - and then other backend will reach not yet initialized buffer and will > broadcast. 2. I've inserted abort if "buffer's border went into future", and wasn't able to trigger it. So I returned update-loop's body to your variant. ------- regards Yura Sokolov aka funny-falcon -
Re: Get rid of WALBufMappingLock
Alexander Korotkov <aekorotkov@gmail.com> — 2025-02-07T11:39:09Z
On Fri, Feb 7, 2025 at 1:24 PM Yura Sokolov <y.sokolov@postgrespro.ru> wrote: > 07.02.2025 14:02, Yura Sokolov пишет: > > 07.02.2025 01:26, Alexander Korotkov пишет: > >> Hi! > >> > >> On Sun, Jan 19, 2025 at 2:11 AM Yura Sokolov <y.sokolov@postgrespro.ru> wrote: > >>> > >>> During discussion of Increasing NUM_XLOGINSERT_LOCKS [1], Andres Freund > >>> used benchmark which creates WAL records very intensively. While I this > >>> it is not completely fair (1MB log records are really rare), it pushed > >>> me to analyze write-side waiting of XLog machinery. > >>> > >>> First I tried to optimize WaitXLogInsertionsToFinish, but without great > >>> success (yet). > >>> > >>> While profiling, I found a lot of time is spend in the memory clearing > >>> under global WALBufMappingLock: > >>> > >>> MemSet((char *) NewPage, 0, XLOG_BLCKSZ); > >>> > >>> It is obvious scalability bottleneck. > >>> > >>> So "challenge was accepted". > >>> > >>> Certainly, backend should initialize pages without exclusive lock. But > >>> which way to ensure pages were initialized? In other words, how to > >>> ensure XLogCtl->InitializedUpTo is correct. > >>> > >>> I've tried to play around WALBufMappingLock with holding it for a short > >>> time and spinning on XLogCtl->xlblocks[nextidx]. But in the end I found > >>> WALBufMappingLock is useless at all. > >>> > >>> Instead of holding lock, it is better to allow backends to cooperate: > >>> - I bound ConditionVariable to each xlblocks entry, > >>> - every backend now checks every required block pointed by > >>> InitializedUpto was successfully initialized or sleeps on its condvar, > >>> - when backend sure block is initialized, it tries to update > >>> InitializedUpTo with conditional variable. > >> > >> Looks reasonable for me, but having ConditionVariable per xlog buffer > >> seems overkill for me. Find an attached revision, where I've > >> implemented advancing InitializedUpTo without ConditionVariable. > >> After initialization of each buffer there is attempt to do CAS for > >> InitializedUpTo in a loop. So, multiple processes will try to advance > >> InitializedUpTo, they could hijack initiative from each other, but > >> there is always a leader which will finish the work. > >> > >> There is only one ConditionVariable to wait for InitializedUpTo being advanced. > >> > >> I didn't benchmark my version, just checked that tests passed. > > > > Good day, Alexander. > > Seems I was mistaken twice. > > > I've got mixed but quite close result for both approaches (single or many > > ConditionVariable) on the notebook. Since I have no access to larger > > machine, I can't prove "many" is way better (or discover it worse). > > 1. "many condvars" (my variant) is strictly worse with num locks = 64 and > when pg_logical_emit_message emits just 1kB instead of 1MB. > > Therefore, "single condvar" is strictly better. > > > Given patch after cleanup looks a bit smaller and clearer, I agree to keep > > just single condition variable. > > > > Cleaned version is attached. > > > > I've changed condition for broadcast a bit ("less" instead "not equal"): > > - buffer's border may already go into future, > > - and then other backend will reach not yet initialized buffer and will > > broadcast. > > 2. I've inserted abort if "buffer's border went into future", and wasn't > able to trigger it. > > So I returned update-loop's body to your variant. Good, thank you. I think 0001 patch is generally good, but needs some further polishing, e.g. more comments explaining how does it work. Regarding 0002 patch, it looks generally reasonable. But are 2 attempts always optimal? Are there cases of regression, or cases when more attempts are even better? Could we have there some self-adjusting mechanism like what we have for spinlocks? ------ Regards, Alexander Korotkov Supabase -
Re: Get rid of WALBufMappingLock
Alexander Korotkov <aekorotkov@gmail.com> — 2025-02-08T10:07:16Z
On Fri, Feb 7, 2025 at 1:39 PM Alexander Korotkov <aekorotkov@gmail.com> wrote: > Good, thank you. I think 0001 patch is generally good, but needs some > further polishing, e.g. more comments explaining how does it work. Two things comes to my mind worth rechecking about 0001. 1) Are XLogCtl->InitializeReserved, XLogCtl->InitializedUpTo and XLogCtl->xlblocks always page-aligned? Because algorithm seems to be sensitive to that. If so, I would propose to explicitly comment that and add corresponding asserts. 2) Check if there are concurrency issues between AdvanceXLInsertBuffer() and switching to the new WAL file. ------ Regards, Alexander Korotkov Supabase
-
Re: Get rid of WALBufMappingLock
Yura Sokolov <y.sokolov@postgrespro.ru> — 2025-02-12T18:16:12Z
08.02.2025 13:07, Alexander Korotkov пишет: > On Fri, Feb 7, 2025 at 1:39 PM Alexander Korotkov <aekorotkov@gmail.com> wrote: >> Good, thank you. I think 0001 patch is generally good, but needs some >> further polishing, e.g. more comments explaining how does it work. I tried to add more comments. I'm not good at, so recommendations are welcome. > Two things comes to my mind worth rechecking about 0001. > 1) Are XLogCtl->InitializeReserved, XLogCtl->InitializedUpTo and > XLogCtl->xlblocks always page-aligned? Because algorithm seems to be > sensitive to that. If so, I would propose to explicitly comment that > and add corresponding asserts. They're certainly page aligned, since they are page borders. I added assert on alignment of InitializeReserved for the sanity. > 2) Check if there are concurrency issues between > AdvanceXLInsertBuffer() and switching to the new WAL file. There are no issues: 1. CopyXLogRecordToWAL for isLogSwitch follows same protocol, ie uses GetXLogBuffer to zero-out WAL page. 2. WALINSERT_SPECIAL_SWITCH forces exclusive lock on all insertion locks, so switching wal is not concurrent. (Although, there is no need in this exclusiveness, imho.) > Regarding 0002 patch, it looks generally reasonable. But are 2 > attempts always optimal? Are there cases of regression, or cases when > more attempts are even better? Could we have there some > self-adjusting mechanism like what we have for spinlocks? Well, I chose to perform 3 probes (2 conditional attempts + 1 unconditional) based on intuition. I have some experience in building hash tables, and cuckoo-hashing theory tells 2 probes is usually enough to reach 50% fill-rate, and 3 probes enough for ~75% fill rate. Since each probe is cache miss, it is hardly sensible to do more probes. 3 probes did better than 2 in other benchmark [1], although there were NUM_XLOGINSERT_LOCK increased. Excuse me for not bencmarking different choices here. I'll try to do measurements in next days. [1] https://postgr.es/m/3b11fdc2-9793-403d-b3d4-67ff9a00d447%40postgrespro.ru ------- regards Yura Sokolov aka funny-falcon
-
Re: Get rid of WALBufMappingLock
Alexander Korotkov <aekorotkov@gmail.com> — 2025-02-13T09:34:38Z
On Wed, Feb 12, 2025 at 8:16 PM Yura Sokolov <y.sokolov@postgrespro.ru> wrote: > 08.02.2025 13:07, Alexander Korotkov пишет: > > On Fri, Feb 7, 2025 at 1:39 PM Alexander Korotkov <aekorotkov@gmail.com> wrote: > >> Good, thank you. I think 0001 patch is generally good, but needs some > >> further polishing, e.g. more comments explaining how does it work. > > I tried to add more comments. I'm not good at, so recommendations are welcome. > > > Two things comes to my mind worth rechecking about 0001. > > 1) Are XLogCtl->InitializeReserved, XLogCtl->InitializedUpTo and > > XLogCtl->xlblocks always page-aligned? Because algorithm seems to be > > sensitive to that. If so, I would propose to explicitly comment that > > and add corresponding asserts. > > They're certainly page aligned, since they are page borders. > I added assert on alignment of InitializeReserved for the sanity. > > > 2) Check if there are concurrency issues between > > AdvanceXLInsertBuffer() and switching to the new WAL file. > > There are no issues: > 1. CopyXLogRecordToWAL for isLogSwitch follows same protocol, ie uses > GetXLogBuffer to zero-out WAL page. > 2. WALINSERT_SPECIAL_SWITCH forces exclusive lock on all insertion locks, > so switching wal is not concurrent. (Although, there is no need in this > exclusiveness, imho.) Good, thank you. I've also revised commit message and comments. But I see another issue with this patch. In the worst case, we do XLogWrite() by ourselves, and it could potentially could error out. Without patch, that would cause WALBufMappingLock be released and XLogCtl->InitializedUpTo not advanced. With the patch, that would cause other processes infinitely waiting till we finish the initialization. Possible solution would be to save position of the page to be initialized, and set it back to XLogCtl->InitializeReserved on error (everywhere we do LWLockReleaseAll()). We also must check that on error we only set XLogCtl->InitializeReserved to the past, because there could be multiple concurrent failures. Also we need to broadcast XLogCtl->InitializedUpToCondVar to wake up waiters. > > Regarding 0002 patch, it looks generally reasonable. But are 2 > > attempts always optimal? Are there cases of regression, or cases when > > more attempts are even better? Could we have there some > > self-adjusting mechanism like what we have for spinlocks? > > Well, I chose to perform 3 probes (2 conditional attempts + 1 > unconditional) based on intuition. I have some experience in building hash > tables, and cuckoo-hashing theory tells 2 probes is usually enough to reach > 50% fill-rate, and 3 probes enough for ~75% fill rate. Since each probe is > cache miss, it is hardly sensible to do more probes. > > 3 probes did better than 2 in other benchmark [1], although there were > NUM_XLOGINSERT_LOCK increased. > > Excuse me for not bencmarking different choices here. I'll try to do > measurements in next days. > > [1] https://postgr.es/m/3b11fdc2-9793-403d-b3d4-67ff9a00d447%40postgrespro.ru Ok, let's wait for your measurements. ------ Regards, Alexander Korotkov Supabase
-
Re: Get rid of WALBufMappingLock
Yura Sokolov <y.sokolov@postgrespro.ru> — 2025-02-13T09:45:26Z
13.02.2025 12:34, Alexander Korotkov пишет: > On Wed, Feb 12, 2025 at 8:16 PM Yura Sokolov <y.sokolov@postgrespro.ru> wrote: >> 08.02.2025 13:07, Alexander Korotkov пишет: >>> On Fri, Feb 7, 2025 at 1:39 PM Alexander Korotkov <aekorotkov@gmail.com> wrote: >>>> Good, thank you. I think 0001 patch is generally good, but needs some >>>> further polishing, e.g. more comments explaining how does it work. >> >> I tried to add more comments. I'm not good at, so recommendations are welcome. >> >>> Two things comes to my mind worth rechecking about 0001. >>> 1) Are XLogCtl->InitializeReserved, XLogCtl->InitializedUpTo and >>> XLogCtl->xlblocks always page-aligned? Because algorithm seems to be >>> sensitive to that. If so, I would propose to explicitly comment that >>> and add corresponding asserts. >> >> They're certainly page aligned, since they are page borders. >> I added assert on alignment of InitializeReserved for the sanity. >> >>> 2) Check if there are concurrency issues between >>> AdvanceXLInsertBuffer() and switching to the new WAL file. >> >> There are no issues: >> 1. CopyXLogRecordToWAL for isLogSwitch follows same protocol, ie uses >> GetXLogBuffer to zero-out WAL page. >> 2. WALINSERT_SPECIAL_SWITCH forces exclusive lock on all insertion locks, >> so switching wal is not concurrent. (Although, there is no need in this >> exclusiveness, imho.) > > Good, thank you. I've also revised commit message and comments. > > But I see another issue with this patch. In the worst case, we do > XLogWrite() by ourselves, and it could potentially could error out. > Without patch, that would cause WALBufMappingLock be released and > XLogCtl->InitializedUpTo not advanced. With the patch, that would > cause other processes infinitely waiting till we finish the > initialization. > > Possible solution would be to save position of the page to be > initialized, and set it back to XLogCtl->InitializeReserved on error > (everywhere we do LWLockReleaseAll()). We also must check that on > error we only set XLogCtl->InitializeReserved to the past, because > there could be multiple concurrent failures. Also we need to > broadcast XLogCtl->InitializedUpToCondVar to wake up waiters. The single place where AdvanceXLInsertBuffer is called outside of critical section is in XLogBackgroundFlush. All other call stacks will issue server restart if XLogWrite will raise error inside of AdvanceXLInsertBuffer. XLogBackgroundFlush explicitely avoids writing buffers by passing opportunistic=true parameter. Therefore, error in XLogWrite will not cause hang in AdvanceXLInsertBuffer since server will shutdown/restart. Perhaps, we just need to insert `Assert(CritSectionCount > 0);` before call to XLogWrite here? ------- regards Yura Sokolov aka funny-falcon
-
Re: Get rid of WALBufMappingLock
Alexander Korotkov <aekorotkov@gmail.com> — 2025-02-13T10:08:13Z
On Thu, Feb 13, 2025 at 11:45 AM Yura Sokolov <y.sokolov@postgrespro.ru> wrote: > 13.02.2025 12:34, Alexander Korotkov пишет: > > On Wed, Feb 12, 2025 at 8:16 PM Yura Sokolov <y.sokolov@postgrespro.ru> wrote: > >> 08.02.2025 13:07, Alexander Korotkov пишет: > >>> On Fri, Feb 7, 2025 at 1:39 PM Alexander Korotkov <aekorotkov@gmail.com> wrote: > >>>> Good, thank you. I think 0001 patch is generally good, but needs some > >>>> further polishing, e.g. more comments explaining how does it work. > >> > >> I tried to add more comments. I'm not good at, so recommendations are welcome. > >> > >>> Two things comes to my mind worth rechecking about 0001. > >>> 1) Are XLogCtl->InitializeReserved, XLogCtl->InitializedUpTo and > >>> XLogCtl->xlblocks always page-aligned? Because algorithm seems to be > >>> sensitive to that. If so, I would propose to explicitly comment that > >>> and add corresponding asserts. > >> > >> They're certainly page aligned, since they are page borders. > >> I added assert on alignment of InitializeReserved for the sanity. > >> > >>> 2) Check if there are concurrency issues between > >>> AdvanceXLInsertBuffer() and switching to the new WAL file. > >> > >> There are no issues: > >> 1. CopyXLogRecordToWAL for isLogSwitch follows same protocol, ie uses > >> GetXLogBuffer to zero-out WAL page. > >> 2. WALINSERT_SPECIAL_SWITCH forces exclusive lock on all insertion locks, > >> so switching wal is not concurrent. (Although, there is no need in this > >> exclusiveness, imho.) > > > > Good, thank you. I've also revised commit message and comments. > > > > But I see another issue with this patch. In the worst case, we do > > XLogWrite() by ourselves, and it could potentially could error out. > > Without patch, that would cause WALBufMappingLock be released and > > XLogCtl->InitializedUpTo not advanced. With the patch, that would > > cause other processes infinitely waiting till we finish the > > initialization. > > > > Possible solution would be to save position of the page to be > > initialized, and set it back to XLogCtl->InitializeReserved on error > > (everywhere we do LWLockReleaseAll()). We also must check that on > > error we only set XLogCtl->InitializeReserved to the past, because > > there could be multiple concurrent failures. Also we need to > > broadcast XLogCtl->InitializedUpToCondVar to wake up waiters. > > The single place where AdvanceXLInsertBuffer is called outside of critical > section is in XLogBackgroundFlush. All other call stacks will issue server > restart if XLogWrite will raise error inside of AdvanceXLInsertBuffer. > > XLogBackgroundFlush explicitely avoids writing buffers by passing > opportunistic=true parameter. > > Therefore, error in XLogWrite will not cause hang in AdvanceXLInsertBuffer > since server will shutdown/restart. > > Perhaps, we just need to insert `Assert(CritSectionCount > 0);` before call > to XLogWrite here? You're correct. I just reflected this in the next revision of the patch. ------ Regards, Alexander Korotkov Supabase
-
Re: Get rid of WALBufMappingLock
Pavel Borisov <pashkin.elfe@gmail.com> — 2025-02-13T16:39:32Z
Hi, Yura and Alexander! On Thu, 13 Feb 2025 at 14:08, Alexander Korotkov <aekorotkov@gmail.com> wrote: > > On Thu, Feb 13, 2025 at 11:45 AM Yura Sokolov <y.sokolov@postgrespro.ru> wrote: > > 13.02.2025 12:34, Alexander Korotkov пишет: > > > On Wed, Feb 12, 2025 at 8:16 PM Yura Sokolov <y.sokolov@postgrespro.ru> wrote: > > >> 08.02.2025 13:07, Alexander Korotkov пишет: > > >>> On Fri, Feb 7, 2025 at 1:39 PM Alexander Korotkov <aekorotkov@gmail.com> wrote: > > >>>> Good, thank you. I think 0001 patch is generally good, but needs some > > >>>> further polishing, e.g. more comments explaining how does it work. > > >> > > >> I tried to add more comments. I'm not good at, so recommendations are welcome. > > >> > > >>> Two things comes to my mind worth rechecking about 0001. > > >>> 1) Are XLogCtl->InitializeReserved, XLogCtl->InitializedUpTo and > > >>> XLogCtl->xlblocks always page-aligned? Because algorithm seems to be > > >>> sensitive to that. If so, I would propose to explicitly comment that > > >>> and add corresponding asserts. > > >> > > >> They're certainly page aligned, since they are page borders. > > >> I added assert on alignment of InitializeReserved for the sanity. > > >> > > >>> 2) Check if there are concurrency issues between > > >>> AdvanceXLInsertBuffer() and switching to the new WAL file. > > >> > > >> There are no issues: > > >> 1. CopyXLogRecordToWAL for isLogSwitch follows same protocol, ie uses > > >> GetXLogBuffer to zero-out WAL page. > > >> 2. WALINSERT_SPECIAL_SWITCH forces exclusive lock on all insertion locks, > > >> so switching wal is not concurrent. (Although, there is no need in this > > >> exclusiveness, imho.) > > > > > > Good, thank you. I've also revised commit message and comments. > > > > > > But I see another issue with this patch. In the worst case, we do > > > XLogWrite() by ourselves, and it could potentially could error out. > > > Without patch, that would cause WALBufMappingLock be released and > > > XLogCtl->InitializedUpTo not advanced. With the patch, that would > > > cause other processes infinitely waiting till we finish the > > > initialization. > > > > > > Possible solution would be to save position of the page to be > > > initialized, and set it back to XLogCtl->InitializeReserved on error > > > (everywhere we do LWLockReleaseAll()). We also must check that on > > > error we only set XLogCtl->InitializeReserved to the past, because > > > there could be multiple concurrent failures. Also we need to > > > broadcast XLogCtl->InitializedUpToCondVar to wake up waiters. > > > > The single place where AdvanceXLInsertBuffer is called outside of critical > > section is in XLogBackgroundFlush. All other call stacks will issue server > > restart if XLogWrite will raise error inside of AdvanceXLInsertBuffer. > > > > XLogBackgroundFlush explicitely avoids writing buffers by passing > > opportunistic=true parameter. > > > > Therefore, error in XLogWrite will not cause hang in AdvanceXLInsertBuffer > > since server will shutdown/restart. > > > > Perhaps, we just need to insert `Assert(CritSectionCount > 0);` before call > > to XLogWrite here? > > You're correct. I just reflected this in the next revision of the patch. I've looked at the patchset v6. The overall goal to avoid locking looks good. Both patches look right to me. The only thing I'm slightly concerned about if patchset could demonstrate performance differences in any real workload. I appreciate it as a beautiful optimization anyway. I have the following proposals to change the code and comments: For patch 0001: - Struct XLBlocks contains just one pg_atomic_uint64 member. Is it still needed as a struct? These changes make a significant volume of changes to the patch, being noop. Maybe it was inherited from v1 and not needed anymore. - Furthermore when xlblocks became a struct comments like: > and xlblocks values certainly do. xlblocks values are changed need to be changed to xlblocks.bound. This could be avoided by changing back xlblocks from type XLBlocks * to pg_atomic_uint64 * - It's worth more detailed commenting InitializedUpTo/InitializedUpToCondVar than: + * It is updated to successfully initialized buffer's identities, perhaps + * waiting on conditional variable bound to buffer. "perhaps waiting" could also be in style "maybe/even while AAA waits BBB" "lock-free with cooperation with" -> "lock-free accompanied by changes to..." - Comment inside typedef struct XLogCtlData: /* 1st byte ptr-s + XLOG_BLCKSZ (and condvar * for) */ need to be returned back /* 1st byte ptr-s + XLOG_BLCKSZ */ - Commented out code for cleanup in the final patch: //ConditionVariableBroadcast(&XLogCtl->xlblocks[nextidx].condvar); - in AdvanceXLInsertBuffer() npages initialised to 0 but it is not increased anymore Block under > if (XLOG_DEBUG && npages > 0) became unreachable (InvalidXLogRecPtr + 1) is essentially 0+1 and IMO this semantically calls for adding #define FirstValidXLogRecPtr 1 Typo in a commit message: %s/usign/using/g For patch 0002: I think Yura's explanation from above in this thread need to get place in a commit message and in a comment to this: > int attempts = 2; Comments around: "try another lock next time" could be modified to reflect that we do repeat twice Kind regards, Pavel Borisov Supabase
-
Re: Get rid of WALBufMappingLock
Alexander Korotkov <aekorotkov@gmail.com> — 2025-02-13T20:59:06Z
Hi, Pavel! On Thu, Feb 13, 2025 at 6:39 PM Pavel Borisov <pashkin.elfe@gmail.com> wrote: > On Thu, 13 Feb 2025 at 14:08, Alexander Korotkov <aekorotkov@gmail.com> wrote: > > > > On Thu, Feb 13, 2025 at 11:45 AM Yura Sokolov <y.sokolov@postgrespro.ru> wrote: > > > 13.02.2025 12:34, Alexander Korotkov пишет: > > > > On Wed, Feb 12, 2025 at 8:16 PM Yura Sokolov <y.sokolov@postgrespro.ru> wrote: > > > >> 08.02.2025 13:07, Alexander Korotkov пишет: > > > >>> On Fri, Feb 7, 2025 at 1:39 PM Alexander Korotkov <aekorotkov@gmail.com> wrote: > > > >>>> Good, thank you. I think 0001 patch is generally good, but needs some > > > >>>> further polishing, e.g. more comments explaining how does it work. > > > >> > > > >> I tried to add more comments. I'm not good at, so recommendations are welcome. > > > >> > > > >>> Two things comes to my mind worth rechecking about 0001. > > > >>> 1) Are XLogCtl->InitializeReserved, XLogCtl->InitializedUpTo and > > > >>> XLogCtl->xlblocks always page-aligned? Because algorithm seems to be > > > >>> sensitive to that. If so, I would propose to explicitly comment that > > > >>> and add corresponding asserts. > > > >> > > > >> They're certainly page aligned, since they are page borders. > > > >> I added assert on alignment of InitializeReserved for the sanity. > > > >> > > > >>> 2) Check if there are concurrency issues between > > > >>> AdvanceXLInsertBuffer() and switching to the new WAL file. > > > >> > > > >> There are no issues: > > > >> 1. CopyXLogRecordToWAL for isLogSwitch follows same protocol, ie uses > > > >> GetXLogBuffer to zero-out WAL page. > > > >> 2. WALINSERT_SPECIAL_SWITCH forces exclusive lock on all insertion locks, > > > >> so switching wal is not concurrent. (Although, there is no need in this > > > >> exclusiveness, imho.) > > > > > > > > Good, thank you. I've also revised commit message and comments. > > > > > > > > But I see another issue with this patch. In the worst case, we do > > > > XLogWrite() by ourselves, and it could potentially could error out. > > > > Without patch, that would cause WALBufMappingLock be released and > > > > XLogCtl->InitializedUpTo not advanced. With the patch, that would > > > > cause other processes infinitely waiting till we finish the > > > > initialization. > > > > > > > > Possible solution would be to save position of the page to be > > > > initialized, and set it back to XLogCtl->InitializeReserved on error > > > > (everywhere we do LWLockReleaseAll()). We also must check that on > > > > error we only set XLogCtl->InitializeReserved to the past, because > > > > there could be multiple concurrent failures. Also we need to > > > > broadcast XLogCtl->InitializedUpToCondVar to wake up waiters. > > > > > > The single place where AdvanceXLInsertBuffer is called outside of critical > > > section is in XLogBackgroundFlush. All other call stacks will issue server > > > restart if XLogWrite will raise error inside of AdvanceXLInsertBuffer. > > > > > > XLogBackgroundFlush explicitely avoids writing buffers by passing > > > opportunistic=true parameter. > > > > > > Therefore, error in XLogWrite will not cause hang in AdvanceXLInsertBuffer > > > since server will shutdown/restart. > > > > > > Perhaps, we just need to insert `Assert(CritSectionCount > 0);` before call > > > to XLogWrite here? > > > > You're correct. I just reflected this in the next revision of the patch. > > I've looked at the patchset v6. Oh, sorry, I really did wrong. I've done git format-patch for wrong local branch for v5 and v6. Patches I've sent for v5 and v6 are actually the same as my v1. This is really pity. Please, find the right version of patchset attached. ------ Regards, Alexander Korotkov Supabase
-
Re: Get rid of WALBufMappingLock
Pavel Borisov <pashkin.elfe@gmail.com> — 2025-02-14T09:45:44Z
Hi, Alexander! On Fri, 14 Feb 2025 at 00:59, Alexander Korotkov <aekorotkov@gmail.com> wrote: > > Hi, Pavel! > > On Thu, Feb 13, 2025 at 6:39 PM Pavel Borisov <pashkin.elfe@gmail.com> wrote: > > On Thu, 13 Feb 2025 at 14:08, Alexander Korotkov <aekorotkov@gmail.com> wrote: > > > > > > On Thu, Feb 13, 2025 at 11:45 AM Yura Sokolov <y.sokolov@postgrespro.ru> wrote: > > > > 13.02.2025 12:34, Alexander Korotkov пишет: > > > > > On Wed, Feb 12, 2025 at 8:16 PM Yura Sokolov <y.sokolov@postgrespro.ru> wrote: > > > > >> 08.02.2025 13:07, Alexander Korotkov пишет: > > > > >>> On Fri, Feb 7, 2025 at 1:39 PM Alexander Korotkov <aekorotkov@gmail.com> wrote: > > > > >>>> Good, thank you. I think 0001 patch is generally good, but needs some > > > > >>>> further polishing, e.g. more comments explaining how does it work. > > > > >> > > > > >> I tried to add more comments. I'm not good at, so recommendations are welcome. > > > > >> > > > > >>> Two things comes to my mind worth rechecking about 0001. > > > > >>> 1) Are XLogCtl->InitializeReserved, XLogCtl->InitializedUpTo and > > > > >>> XLogCtl->xlblocks always page-aligned? Because algorithm seems to be > > > > >>> sensitive to that. If so, I would propose to explicitly comment that > > > > >>> and add corresponding asserts. > > > > >> > > > > >> They're certainly page aligned, since they are page borders. > > > > >> I added assert on alignment of InitializeReserved for the sanity. > > > > >> > > > > >>> 2) Check if there are concurrency issues between > > > > >>> AdvanceXLInsertBuffer() and switching to the new WAL file. > > > > >> > > > > >> There are no issues: > > > > >> 1. CopyXLogRecordToWAL for isLogSwitch follows same protocol, ie uses > > > > >> GetXLogBuffer to zero-out WAL page. > > > > >> 2. WALINSERT_SPECIAL_SWITCH forces exclusive lock on all insertion locks, > > > > >> so switching wal is not concurrent. (Although, there is no need in this > > > > >> exclusiveness, imho.) > > > > > > > > > > Good, thank you. I've also revised commit message and comments. > > > > > > > > > > But I see another issue with this patch. In the worst case, we do > > > > > XLogWrite() by ourselves, and it could potentially could error out. > > > > > Without patch, that would cause WALBufMappingLock be released and > > > > > XLogCtl->InitializedUpTo not advanced. With the patch, that would > > > > > cause other processes infinitely waiting till we finish the > > > > > initialization. > > > > > > > > > > Possible solution would be to save position of the page to be > > > > > initialized, and set it back to XLogCtl->InitializeReserved on error > > > > > (everywhere we do LWLockReleaseAll()). We also must check that on > > > > > error we only set XLogCtl->InitializeReserved to the past, because > > > > > there could be multiple concurrent failures. Also we need to > > > > > broadcast XLogCtl->InitializedUpToCondVar to wake up waiters. > > > > > > > > The single place where AdvanceXLInsertBuffer is called outside of critical > > > > section is in XLogBackgroundFlush. All other call stacks will issue server > > > > restart if XLogWrite will raise error inside of AdvanceXLInsertBuffer. > > > > > > > > XLogBackgroundFlush explicitely avoids writing buffers by passing > > > > opportunistic=true parameter. > > > > > > > > Therefore, error in XLogWrite will not cause hang in AdvanceXLInsertBuffer > > > > since server will shutdown/restart. > > > > > > > > Perhaps, we just need to insert `Assert(CritSectionCount > 0);` before call > > > > to XLogWrite here? > > > > > > You're correct. I just reflected this in the next revision of the patch. > > > > I've looked at the patchset v6. > > Oh, sorry, I really did wrong. I've done git format-patch for wrong > local branch for v5 and v6. Patches I've sent for v5 and v6 are > actually the same as my v1. This is really pity. Please, find the > right version of patchset attached. I've rechecked v7. In v6 a proposal from [1] was not reflected. Now it landed in v7. Other changes are not regarding code behavior. The things from my previous review that still could apply to v7: For 0001: Comment change proposed: "lock-free with cooperation with" -> "lock-free accompanied by changes to..." (maybe other variant) I propose a new define: #define FirstValidXLogRecPtr 1 While FirstValidXLogRecPtr = InvalidXLogRecPtr + 1 is true in the code that has no semantical meaning and it's better to avoid using direct arithmetics to relate meaning of FirstValidXLogRecPtr from InvalidXLogRecPtr. For 0002 both comments proposals from my message applied to v6 apply to v7 as well [1] https://www.postgresql.org/message-id/d6799557-e352-42c8-80cc-ed36e3b8893c%40postgrespro.ru Regards, Pavel Borisov Supabase
-
Re: Get rid of WALBufMappingLock
Alexander Korotkov <aekorotkov@gmail.com> — 2025-02-14T10:24:15Z
On Fri, Feb 14, 2025 at 11:45 AM Pavel Borisov <pashkin.elfe@gmail.com> wrote: > On Fri, 14 Feb 2025 at 00:59, Alexander Korotkov <aekorotkov@gmail.com> wrote: > > On Thu, Feb 13, 2025 at 6:39 PM Pavel Borisov <pashkin.elfe@gmail.com> wrote: > > > On Thu, 13 Feb 2025 at 14:08, Alexander Korotkov <aekorotkov@gmail.com> wrote: > > > > > > > > On Thu, Feb 13, 2025 at 11:45 AM Yura Sokolov <y.sokolov@postgrespro.ru> wrote: > > > > > 13.02.2025 12:34, Alexander Korotkov пишет: > > > > > > On Wed, Feb 12, 2025 at 8:16 PM Yura Sokolov <y.sokolov@postgrespro.ru> wrote: > > > > > >> 08.02.2025 13:07, Alexander Korotkov пишет: > > > > > >>> On Fri, Feb 7, 2025 at 1:39 PM Alexander Korotkov <aekorotkov@gmail.com> wrote: > > > > > >>>> Good, thank you. I think 0001 patch is generally good, but needs some > > > > > >>>> further polishing, e.g. more comments explaining how does it work. > > > > > >> > > > > > >> I tried to add more comments. I'm not good at, so recommendations are welcome. > > > > > >> > > > > > >>> Two things comes to my mind worth rechecking about 0001. > > > > > >>> 1) Are XLogCtl->InitializeReserved, XLogCtl->InitializedUpTo and > > > > > >>> XLogCtl->xlblocks always page-aligned? Because algorithm seems to be > > > > > >>> sensitive to that. If so, I would propose to explicitly comment that > > > > > >>> and add corresponding asserts. > > > > > >> > > > > > >> They're certainly page aligned, since they are page borders. > > > > > >> I added assert on alignment of InitializeReserved for the sanity. > > > > > >> > > > > > >>> 2) Check if there are concurrency issues between > > > > > >>> AdvanceXLInsertBuffer() and switching to the new WAL file. > > > > > >> > > > > > >> There are no issues: > > > > > >> 1. CopyXLogRecordToWAL for isLogSwitch follows same protocol, ie uses > > > > > >> GetXLogBuffer to zero-out WAL page. > > > > > >> 2. WALINSERT_SPECIAL_SWITCH forces exclusive lock on all insertion locks, > > > > > >> so switching wal is not concurrent. (Although, there is no need in this > > > > > >> exclusiveness, imho.) > > > > > > > > > > > > Good, thank you. I've also revised commit message and comments. > > > > > > > > > > > > But I see another issue with this patch. In the worst case, we do > > > > > > XLogWrite() by ourselves, and it could potentially could error out. > > > > > > Without patch, that would cause WALBufMappingLock be released and > > > > > > XLogCtl->InitializedUpTo not advanced. With the patch, that would > > > > > > cause other processes infinitely waiting till we finish the > > > > > > initialization. > > > > > > > > > > > > Possible solution would be to save position of the page to be > > > > > > initialized, and set it back to XLogCtl->InitializeReserved on error > > > > > > (everywhere we do LWLockReleaseAll()). We also must check that on > > > > > > error we only set XLogCtl->InitializeReserved to the past, because > > > > > > there could be multiple concurrent failures. Also we need to > > > > > > broadcast XLogCtl->InitializedUpToCondVar to wake up waiters. > > > > > > > > > > The single place where AdvanceXLInsertBuffer is called outside of critical > > > > > section is in XLogBackgroundFlush. All other call stacks will issue server > > > > > restart if XLogWrite will raise error inside of AdvanceXLInsertBuffer. > > > > > > > > > > XLogBackgroundFlush explicitely avoids writing buffers by passing > > > > > opportunistic=true parameter. > > > > > > > > > > Therefore, error in XLogWrite will not cause hang in AdvanceXLInsertBuffer > > > > > since server will shutdown/restart. > > > > > > > > > > Perhaps, we just need to insert `Assert(CritSectionCount > 0);` before call > > > > > to XLogWrite here? > > > > > > > > You're correct. I just reflected this in the next revision of the patch. > > > > > > I've looked at the patchset v6. > > > > Oh, sorry, I really did wrong. I've done git format-patch for wrong > > local branch for v5 and v6. Patches I've sent for v5 and v6 are > > actually the same as my v1. This is really pity. Please, find the > > right version of patchset attached. > > I've rechecked v7. In v6 a proposal from [1] was not reflected. Now it > landed in v7. > > Other changes are not regarding code behavior. The things from my > previous review that still could apply to v7: > > For 0001: > > Comment change proposed: > "lock-free with cooperation with" -> "lock-free accompanied by changes > to..." (maybe other variant) Good catch. I've rephrased this comment even more. > I propose a new define: > #define FirstValidXLogRecPtr 1 > While FirstValidXLogRecPtr = InvalidXLogRecPtr + 1 is true in the code > that has no semantical meaning and it's better to avoid using direct > arithmetics to relate meaning of FirstValidXLogRecPtr from > InvalidXLogRecPtr. Makes sense, but I'm not sure if this change is required at all. I've reverted this to the state of master, and everything seems to work. > For 0002 both comments proposals from my message applied to v6 apply > to v7 as well Thank you for pointing. For now, I'm concentrated on improvements on 0001. Probably Yura could work on your notes to 0002. ------ Regards, Alexander Korotkov Supabase
-
Re: Get rid of WALBufMappingLock
Yura Sokolov <y.sokolov@postgrespro.ru> — 2025-02-14T14:09:18Z
14.02.2025 13:24, Alexander Korotkov пишет: > On Fri, Feb 14, 2025 at 11:45 AM Pavel Borisov <pashkin.elfe@gmail.com> wrote: >> On Fri, 14 Feb 2025 at 00:59, Alexander Korotkov <aekorotkov@gmail.com> wrote: >>> On Thu, Feb 13, 2025 at 6:39 PM Pavel Borisov <pashkin.elfe@gmail.com> wrote: >>>> On Thu, 13 Feb 2025 at 14:08, Alexander Korotkov <aekorotkov@gmail.com> wrote: >>>>> >>>>> On Thu, Feb 13, 2025 at 11:45 AM Yura Sokolov <y.sokolov@postgrespro.ru> wrote: >>>>>> 13.02.2025 12:34, Alexander Korotkov пишет: >>>>>>> On Wed, Feb 12, 2025 at 8:16 PM Yura Sokolov <y.sokolov@postgrespro.ru> wrote: >>>>>>>> 08.02.2025 13:07, Alexander Korotkov пишет: >>>>>>>>> On Fri, Feb 7, 2025 at 1:39 PM Alexander Korotkov <aekorotkov@gmail.com> wrote: >>>>>>>>>> Good, thank you. I think 0001 patch is generally good, but needs some >>>>>>>>>> further polishing, e.g. more comments explaining how does it work. >>>>>>>> >>>>>>>> I tried to add more comments. I'm not good at, so recommendations are welcome. >>>>>>>> >>>>>>>>> Two things comes to my mind worth rechecking about 0001. >>>>>>>>> 1) Are XLogCtl->InitializeReserved, XLogCtl->InitializedUpTo and >>>>>>>>> XLogCtl->xlblocks always page-aligned? Because algorithm seems to be >>>>>>>>> sensitive to that. If so, I would propose to explicitly comment that >>>>>>>>> and add corresponding asserts. >>>>>>>> >>>>>>>> They're certainly page aligned, since they are page borders. >>>>>>>> I added assert on alignment of InitializeReserved for the sanity. >>>>>>>> >>>>>>>>> 2) Check if there are concurrency issues between >>>>>>>>> AdvanceXLInsertBuffer() and switching to the new WAL file. >>>>>>>> >>>>>>>> There are no issues: >>>>>>>> 1. CopyXLogRecordToWAL for isLogSwitch follows same protocol, ie uses >>>>>>>> GetXLogBuffer to zero-out WAL page. >>>>>>>> 2. WALINSERT_SPECIAL_SWITCH forces exclusive lock on all insertion locks, >>>>>>>> so switching wal is not concurrent. (Although, there is no need in this >>>>>>>> exclusiveness, imho.) >>>>>>> >>>>>>> Good, thank you. I've also revised commit message and comments. >>>>>>> >>>>>>> But I see another issue with this patch. In the worst case, we do >>>>>>> XLogWrite() by ourselves, and it could potentially could error out. >>>>>>> Without patch, that would cause WALBufMappingLock be released and >>>>>>> XLogCtl->InitializedUpTo not advanced. With the patch, that would >>>>>>> cause other processes infinitely waiting till we finish the >>>>>>> initialization. >>>>>>> >>>>>>> Possible solution would be to save position of the page to be >>>>>>> initialized, and set it back to XLogCtl->InitializeReserved on error >>>>>>> (everywhere we do LWLockReleaseAll()). We also must check that on >>>>>>> error we only set XLogCtl->InitializeReserved to the past, because >>>>>>> there could be multiple concurrent failures. Also we need to >>>>>>> broadcast XLogCtl->InitializedUpToCondVar to wake up waiters. >>>>>> >>>>>> The single place where AdvanceXLInsertBuffer is called outside of critical >>>>>> section is in XLogBackgroundFlush. All other call stacks will issue server >>>>>> restart if XLogWrite will raise error inside of AdvanceXLInsertBuffer. >>>>>> >>>>>> XLogBackgroundFlush explicitely avoids writing buffers by passing >>>>>> opportunistic=true parameter. >>>>>> >>>>>> Therefore, error in XLogWrite will not cause hang in AdvanceXLInsertBuffer >>>>>> since server will shutdown/restart. >>>>>> >>>>>> Perhaps, we just need to insert `Assert(CritSectionCount > 0);` before call >>>>>> to XLogWrite here? >>>>> >>>>> You're correct. I just reflected this in the next revision of the patch. >>>> >>>> I've looked at the patchset v6. >>> >>> Oh, sorry, I really did wrong. I've done git format-patch for wrong >>> local branch for v5 and v6. Patches I've sent for v5 and v6 are >>> actually the same as my v1. This is really pity. Please, find the >>> right version of patchset attached. >> >> I've rechecked v7. In v6 a proposal from [1] was not reflected. Now it >> landed in v7. >> >> Other changes are not regarding code behavior. The things from my >> previous review that still could apply to v7: >> >> For 0001: >> >> Comment change proposed: >> "lock-free with cooperation with" -> "lock-free accompanied by changes >> to..." (maybe other variant) > > Good catch. I've rephrased this comment even more. > >> I propose a new define: >> #define FirstValidXLogRecPtr 1 >> While FirstValidXLogRecPtr = InvalidXLogRecPtr + 1 is true in the code >> that has no semantical meaning and it's better to avoid using direct >> arithmetics to relate meaning of FirstValidXLogRecPtr from >> InvalidXLogRecPtr. > > Makes sense, but I'm not sure if this change is required at all. I've > reverted this to the state of master, and everything seems to work. > >> For 0002 both comments proposals from my message applied to v6 apply >> to v7 as well > > Thank you for pointing. For now, I'm concentrated on improvements on > 0001. Probably Yura could work on your notes to 0002. I wrote good commit message for 0002 with calculated probabilities and simple Ruby program which calculates them to explain choice of 2 conditional attempts. (At least I hope the message is good). And added simple comment before `int attempts = 2;` Also I simplified 0002 a bit to look a bit prettier (ie without goto), and added static assert on NUM_XLOGINSERT_LOCKS being power of 2. (0001 patch is same as for v8) ------- regards Yura Sokolov aka funny-falcon
-
Re: Get rid of WALBufMappingLock
Yura Sokolov <y.sokolov@postgrespro.ru> — 2025-02-14T14:11:14Z
14.02.2025 17:09, Yura Sokolov пишет: > 14.02.2025 13:24, Alexander Korotkov пишет: >> On Fri, Feb 14, 2025 at 11:45 AM Pavel Borisov <pashkin.elfe@gmail.com> wrote: >>> On Fri, 14 Feb 2025 at 00:59, Alexander Korotkov <aekorotkov@gmail.com> wrote: >>>> On Thu, Feb 13, 2025 at 6:39 PM Pavel Borisov <pashkin.elfe@gmail.com> wrote: >>>>> On Thu, 13 Feb 2025 at 14:08, Alexander Korotkov <aekorotkov@gmail.com> wrote: >>>>>> >>>>>> On Thu, Feb 13, 2025 at 11:45 AM Yura Sokolov <y.sokolov@postgrespro.ru> wrote: >>>>>>> 13.02.2025 12:34, Alexander Korotkov пишет: >>>>>>>> On Wed, Feb 12, 2025 at 8:16 PM Yura Sokolov <y.sokolov@postgrespro.ru> wrote: >>>>>>>>> 08.02.2025 13:07, Alexander Korotkov пишет: >>>>>>>>>> On Fri, Feb 7, 2025 at 1:39 PM Alexander Korotkov <aekorotkov@gmail.com> wrote: >>>>>>>>>>> Good, thank you. I think 0001 patch is generally good, but needs some >>>>>>>>>>> further polishing, e.g. more comments explaining how does it work. >>>>>>>>> >>>>>>>>> I tried to add more comments. I'm not good at, so recommendations are welcome. >>>>>>>>> >>>>>>>>>> Two things comes to my mind worth rechecking about 0001. >>>>>>>>>> 1) Are XLogCtl->InitializeReserved, XLogCtl->InitializedUpTo and >>>>>>>>>> XLogCtl->xlblocks always page-aligned? Because algorithm seems to be >>>>>>>>>> sensitive to that. If so, I would propose to explicitly comment that >>>>>>>>>> and add corresponding asserts. >>>>>>>>> >>>>>>>>> They're certainly page aligned, since they are page borders. >>>>>>>>> I added assert on alignment of InitializeReserved for the sanity. >>>>>>>>> >>>>>>>>>> 2) Check if there are concurrency issues between >>>>>>>>>> AdvanceXLInsertBuffer() and switching to the new WAL file. >>>>>>>>> >>>>>>>>> There are no issues: >>>>>>>>> 1. CopyXLogRecordToWAL for isLogSwitch follows same protocol, ie uses >>>>>>>>> GetXLogBuffer to zero-out WAL page. >>>>>>>>> 2. WALINSERT_SPECIAL_SWITCH forces exclusive lock on all insertion locks, >>>>>>>>> so switching wal is not concurrent. (Although, there is no need in this >>>>>>>>> exclusiveness, imho.) >>>>>>>> >>>>>>>> Good, thank you. I've also revised commit message and comments. >>>>>>>> >>>>>>>> But I see another issue with this patch. In the worst case, we do >>>>>>>> XLogWrite() by ourselves, and it could potentially could error out. >>>>>>>> Without patch, that would cause WALBufMappingLock be released and >>>>>>>> XLogCtl->InitializedUpTo not advanced. With the patch, that would >>>>>>>> cause other processes infinitely waiting till we finish the >>>>>>>> initialization. >>>>>>>> >>>>>>>> Possible solution would be to save position of the page to be >>>>>>>> initialized, and set it back to XLogCtl->InitializeReserved on error >>>>>>>> (everywhere we do LWLockReleaseAll()). We also must check that on >>>>>>>> error we only set XLogCtl->InitializeReserved to the past, because >>>>>>>> there could be multiple concurrent failures. Also we need to >>>>>>>> broadcast XLogCtl->InitializedUpToCondVar to wake up waiters. >>>>>>> >>>>>>> The single place where AdvanceXLInsertBuffer is called outside of critical >>>>>>> section is in XLogBackgroundFlush. All other call stacks will issue server >>>>>>> restart if XLogWrite will raise error inside of AdvanceXLInsertBuffer. >>>>>>> >>>>>>> XLogBackgroundFlush explicitely avoids writing buffers by passing >>>>>>> opportunistic=true parameter. >>>>>>> >>>>>>> Therefore, error in XLogWrite will not cause hang in AdvanceXLInsertBuffer >>>>>>> since server will shutdown/restart. >>>>>>> >>>>>>> Perhaps, we just need to insert `Assert(CritSectionCount > 0);` before call >>>>>>> to XLogWrite here? >>>>>> >>>>>> You're correct. I just reflected this in the next revision of the patch. >>>>> >>>>> I've looked at the patchset v6. >>>> >>>> Oh, sorry, I really did wrong. I've done git format-patch for wrong >>>> local branch for v5 and v6. Patches I've sent for v5 and v6 are >>>> actually the same as my v1. This is really pity. Please, find the >>>> right version of patchset attached. >>> >>> I've rechecked v7. In v6 a proposal from [1] was not reflected. Now it >>> landed in v7. >>> >>> Other changes are not regarding code behavior. The things from my >>> previous review that still could apply to v7: >>> >>> For 0001: >>> >>> Comment change proposed: >>> "lock-free with cooperation with" -> "lock-free accompanied by changes >>> to..." (maybe other variant) >> >> Good catch. I've rephrased this comment even more. >> >>> I propose a new define: >>> #define FirstValidXLogRecPtr 1 >>> While FirstValidXLogRecPtr = InvalidXLogRecPtr + 1 is true in the code >>> that has no semantical meaning and it's better to avoid using direct >>> arithmetics to relate meaning of FirstValidXLogRecPtr from >>> InvalidXLogRecPtr. >> >> Makes sense, but I'm not sure if this change is required at all. I've >> reverted this to the state of master, and everything seems to work. >> >>> For 0002 both comments proposals from my message applied to v6 apply >>> to v7 as well >> >> Thank you for pointing. For now, I'm concentrated on improvements on >> 0001. Probably Yura could work on your notes to 0002. > > I wrote good commit message for 0002 with calculated probabilities and > simple Ruby program which calculates them to explain choice of 2 > conditional attempts. (At least I hope the message is good). And added > simple comment before `int attempts = 2;` > > Also I simplified 0002 a bit to look a bit prettier (ie without goto), and > added static assert on NUM_XLOGINSERT_LOCKS being power of 2. > > (0001 patch is same as for v8) > > ------- > regards > Yura Sokolov aka funny-falcon Oops, forgot to add StaticAssert into v9-0002.
-
Re: Get rid of WALBufMappingLock
Alexander Korotkov <aekorotkov@gmail.com> — 2025-02-15T09:25:00Z
Hi! On Fri, Feb 14, 2025 at 4:11 PM Yura Sokolov <y.sokolov@postgrespro.ru> wrote: > 14.02.2025 17:09, Yura Sokolov пишет: > > 14.02.2025 13:24, Alexander Korotkov пишет: > >> On Fri, Feb 14, 2025 at 11:45 AM Pavel Borisov <pashkin.elfe@gmail.com> wrote: > >>> On Fri, 14 Feb 2025 at 00:59, Alexander Korotkov <aekorotkov@gmail.com> wrote: > >>>> On Thu, Feb 13, 2025 at 6:39 PM Pavel Borisov <pashkin.elfe@gmail.com> wrote: > >>>>> On Thu, 13 Feb 2025 at 14:08, Alexander Korotkov <aekorotkov@gmail.com> wrote: > >>>>>> > >>>>>> On Thu, Feb 13, 2025 at 11:45 AM Yura Sokolov <y.sokolov@postgrespro.ru> wrote: > >>>>>>> 13.02.2025 12:34, Alexander Korotkov пишет: > >>>>>>>> On Wed, Feb 12, 2025 at 8:16 PM Yura Sokolov <y.sokolov@postgrespro.ru> wrote: > >>>>>>>>> 08.02.2025 13:07, Alexander Korotkov пишет: > >>>>>>>>>> On Fri, Feb 7, 2025 at 1:39 PM Alexander Korotkov <aekorotkov@gmail.com> wrote: > >>>>>>>>>>> Good, thank you. I think 0001 patch is generally good, but needs some > >>>>>>>>>>> further polishing, e.g. more comments explaining how does it work. > >>>>>>>>> > >>>>>>>>> I tried to add more comments. I'm not good at, so recommendations are welcome. > >>>>>>>>> > >>>>>>>>>> Two things comes to my mind worth rechecking about 0001. > >>>>>>>>>> 1) Are XLogCtl->InitializeReserved, XLogCtl->InitializedUpTo and > >>>>>>>>>> XLogCtl->xlblocks always page-aligned? Because algorithm seems to be > >>>>>>>>>> sensitive to that. If so, I would propose to explicitly comment that > >>>>>>>>>> and add corresponding asserts. > >>>>>>>>> > >>>>>>>>> They're certainly page aligned, since they are page borders. > >>>>>>>>> I added assert on alignment of InitializeReserved for the sanity. > >>>>>>>>> > >>>>>>>>>> 2) Check if there are concurrency issues between > >>>>>>>>>> AdvanceXLInsertBuffer() and switching to the new WAL file. > >>>>>>>>> > >>>>>>>>> There are no issues: > >>>>>>>>> 1. CopyXLogRecordToWAL for isLogSwitch follows same protocol, ie uses > >>>>>>>>> GetXLogBuffer to zero-out WAL page. > >>>>>>>>> 2. WALINSERT_SPECIAL_SWITCH forces exclusive lock on all insertion locks, > >>>>>>>>> so switching wal is not concurrent. (Although, there is no need in this > >>>>>>>>> exclusiveness, imho.) > >>>>>>>> > >>>>>>>> Good, thank you. I've also revised commit message and comments. > >>>>>>>> > >>>>>>>> But I see another issue with this patch. In the worst case, we do > >>>>>>>> XLogWrite() by ourselves, and it could potentially could error out. > >>>>>>>> Without patch, that would cause WALBufMappingLock be released and > >>>>>>>> XLogCtl->InitializedUpTo not advanced. With the patch, that would > >>>>>>>> cause other processes infinitely waiting till we finish the > >>>>>>>> initialization. > >>>>>>>> > >>>>>>>> Possible solution would be to save position of the page to be > >>>>>>>> initialized, and set it back to XLogCtl->InitializeReserved on error > >>>>>>>> (everywhere we do LWLockReleaseAll()). We also must check that on > >>>>>>>> error we only set XLogCtl->InitializeReserved to the past, because > >>>>>>>> there could be multiple concurrent failures. Also we need to > >>>>>>>> broadcast XLogCtl->InitializedUpToCondVar to wake up waiters. > >>>>>>> > >>>>>>> The single place where AdvanceXLInsertBuffer is called outside of critical > >>>>>>> section is in XLogBackgroundFlush. All other call stacks will issue server > >>>>>>> restart if XLogWrite will raise error inside of AdvanceXLInsertBuffer. > >>>>>>> > >>>>>>> XLogBackgroundFlush explicitely avoids writing buffers by passing > >>>>>>> opportunistic=true parameter. > >>>>>>> > >>>>>>> Therefore, error in XLogWrite will not cause hang in AdvanceXLInsertBuffer > >>>>>>> since server will shutdown/restart. > >>>>>>> > >>>>>>> Perhaps, we just need to insert `Assert(CritSectionCount > 0);` before call > >>>>>>> to XLogWrite here? > >>>>>> > >>>>>> You're correct. I just reflected this in the next revision of the patch. > >>>>> > >>>>> I've looked at the patchset v6. > >>>> > >>>> Oh, sorry, I really did wrong. I've done git format-patch for wrong > >>>> local branch for v5 and v6. Patches I've sent for v5 and v6 are > >>>> actually the same as my v1. This is really pity. Please, find the > >>>> right version of patchset attached. > >>> > >>> I've rechecked v7. In v6 a proposal from [1] was not reflected. Now it > >>> landed in v7. > >>> > >>> Other changes are not regarding code behavior. The things from my > >>> previous review that still could apply to v7: > >>> > >>> For 0001: > >>> > >>> Comment change proposed: > >>> "lock-free with cooperation with" -> "lock-free accompanied by changes > >>> to..." (maybe other variant) > >> > >> Good catch. I've rephrased this comment even more. > >> > >>> I propose a new define: > >>> #define FirstValidXLogRecPtr 1 > >>> While FirstValidXLogRecPtr = InvalidXLogRecPtr + 1 is true in the code > >>> that has no semantical meaning and it's better to avoid using direct > >>> arithmetics to relate meaning of FirstValidXLogRecPtr from > >>> InvalidXLogRecPtr. > >> > >> Makes sense, but I'm not sure if this change is required at all. I've > >> reverted this to the state of master, and everything seems to work. > >> > >>> For 0002 both comments proposals from my message applied to v6 apply > >>> to v7 as well > >> > >> Thank you for pointing. For now, I'm concentrated on improvements on > >> 0001. Probably Yura could work on your notes to 0002. > > > > I wrote good commit message for 0002 with calculated probabilities and > > simple Ruby program which calculates them to explain choice of 2 > > conditional attempts. (At least I hope the message is good). And added > > simple comment before `int attempts = 2;` > > > > Also I simplified 0002 a bit to look a bit prettier (ie without goto), and > > added static assert on NUM_XLOGINSERT_LOCKS being power of 2. > > > > (0001 patch is same as for v8) > > Oops, forgot to add StaticAssert into v9-0002. Thank you. I'm planning to push 0001 if there is no objections. And I'm planning to do more review/revision of 0002. ------ Regards, Alexander Korotkov Supabase
-
Re: Get rid of WALBufMappingLock
Kirill Reshke <reshkekirill@gmail.com> — 2025-02-17T05:19:07Z
Hi! I spotted a typo in v10: + /* + * Page at nextidx wasn't initialized yet, so we cann't move + * InitializedUpto further. It will be moved by backend which + * will initialize nextidx. + */ cann't - > can't moved by backend -> moved by the backend -- Best regards, Kirill Reshke
-
Re: Get rid of WALBufMappingLock
Victor Yegorov <vyegorov@gmail.com> — 2025-02-17T08:46:57Z
Hey. I find “Get rid of WALBufMappingLock" commit message misleading, 'cos Lock it's being replaced by CV, actually. Should the subject be changed to “Replace WALBufMappingLock with ConditionVariable” instead? -- Victor Yegorov
-
Re: Get rid of WALBufMappingLock
Pavel Borisov <pashkin.elfe@gmail.com> — 2025-02-17T09:20:09Z
Hi, Victor! On Mon, 17 Feb 2025 at 12:47, Victor Yegorov <vyegorov@gmail.com> wrote: > > Hey. > > I find “Get rid of WALBufMappingLock" commit message misleading, 'cos Lock it's being replaced by CV, actually. > > Should the subject be changed to “Replace WALBufMappingLock with ConditionVariable” instead? The patch replaces WALBufMappingLock with a lockless algorithm based on atomic variables and CV. Mentioning only CV in the head is only a part of implementation. Also, the header should better reflect what is done on the whole, than the implementation details. So I'd rather see a header like "Replace WALBufMappingLock by lockless algorithm" or "Initialize WAL buffers concurrently without using WALBufMappingLock" or something like that. Kind regards, Pavel Borisov Supabase
-
Re: Get rid of WALBufMappingLock
Pavel Borisov <pashkin.elfe@gmail.com> — 2025-02-17T09:24:06Z
On Mon, 17 Feb 2025 at 13:20, Pavel Borisov <pashkin.elfe@gmail.com> wrote: > > Hi, Victor! > > On Mon, 17 Feb 2025 at 12:47, Victor Yegorov <vyegorov@gmail.com> wrote: > > > > Hey. > > > > I find “Get rid of WALBufMappingLock" commit message misleading, 'cos Lock it's being replaced by CV, actually. > > > > Should the subject be changed to “Replace WALBufMappingLock with ConditionVariable” instead? > > The patch replaces WALBufMappingLock with a lockless algorithm based > on atomic variables and CV. Mentioning only CV in the head is only a > part of implementation. Also, the header should better reflect what is > done on the whole, than the implementation details. So I'd rather see > a header like "Replace WALBufMappingLock by lockless algorithm" or > "Initialize WAL buffers concurrently without using WALBufMappingLock" > or something like that. Update: I see the patch is already committed, so we're late with the naming proposals. I don't see problem with existing commit message TBH. Kind regards, Pavel Borisov
-
Re: Get rid of WALBufMappingLock
Pavel Borisov <pashkin.elfe@gmail.com> — 2025-02-17T09:40:43Z
Hi, Kirill! Per your report, I revised the comment to fix typos. Also some little changes in grammar. Kind regards, Pavel Borisov
-
Re: Get rid of WALBufMappingLock
Pavel Borisov <pashkin.elfe@gmail.com> — 2025-02-17T09:44:25Z
Oops, I send wrong patch as a fix. The right one is attached. Pavel On Mon, 17 Feb 2025 at 13:40, Pavel Borisov <pashkin.elfe@gmail.com> wrote: > > Hi, Kirill! > Per your report, I revised the comment to fix typos. Also some little > changes in grammar. > > Kind regards, > Pavel Borisov
-
Re: Get rid of WALBufMappingLock
Alexander Korotkov <aekorotkov@gmail.com> — 2025-02-17T10:34:47Z
On Mon, Feb 17, 2025 at 11:44 AM Pavel Borisov <pashkin.elfe@gmail.com> wrote: > Oops, I send wrong patch as a fix. > The right one is attached. > > Pavel I've spotted the failure on the buildfarm. https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=batta&dt=2025-02-17%2008%3A05%3A03 I can't quickly guess the reason. I'm going to revert patch for now, then we investigate ------ Regards, Alexander Korotkov Supabase
-
Re: Get rid of WALBufMappingLock
Tom Lane <tgl@sss.pgh.pa.us> — 2025-02-17T16:25:05Z
Alexander Korotkov <aekorotkov@gmail.com> writes: > I've spotted the failure on the buildfarm. > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=batta&dt=2025-02-17%2008%3A05%3A03 > I can't quickly guess the reason. I'm going to revert patch for now, > then we investigate This timeout failure on hachi looks suspicious as well: https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=hachi&dt=2025-02-17%2003%3A05%3A03 Might be relevant that they are both aarch64? regards, tom lane
-
Re: Get rid of WALBufMappingLock
Michael Paquier <michael@paquier.xyz> — 2025-02-18T00:21:13Z
On Mon, Feb 17, 2025 at 11:25:05AM -0500, Tom Lane wrote: > This timeout failure on hachi looks suspicious as well: > > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=hachi&dt=2025-02-17%2003%3A05%3A03 > > Might be relevant that they are both aarch64? Just logged into the host. The logs of the timed out run are still around, and the last information I can see is from lastcommand.log, which seems to have frozen in time when the timeout has begun its vacuuming work: ok 73 + index_including_gist 353 ms # parallel group (16 tests): create_cast errors create_aggregate drop_if_exists infinite_recurse gokiburi is on the same host, and it is currently frozen in time when trying to fetch a WAL buffer. One of the stack traces: #2 0x000000000084ec48 in WaitEventSetWaitBlock (set=0xd34ce0, cur_timeout=-1, occurred_events=0xffffffffadd8, nevents=1) at latch.c:1571 #3 WaitEventSetWait (set=0xd34ce0, timeout=-1, occurred_events=occurred_events@entry=0xffffffffadd8, nevents=nevents@entry=1, wait_event_info=<optimized out>, wait_event_info@entry=134217781) at latch.c:1519 #4 0x000000000084e964 in WaitLatch (latch=<optimized out>, wakeEvents=wakeEvents@entry=33, timeout=timeout@entry=-1, wait_event_info=wait_event_info@entry=134217781) at latch.c:538 #5 0x000000000085d2f8 in ConditionVariableTimedSleep (cv=0xffffec0799b0, timeout=-1, wait_event_info=134217781) at condition_variable.c:163 #6 0x000000000085d1ec in ConditionVariableSleep (cv=0xfffffffffffffffc, wait_event_info=1) at condition_variable.c:98 #7 0x000000000055f4f4 in AdvanceXLInsertBuffer (upto=upto@entry=112064880, tli=tli@entry=1, opportunistic=false) at xlog.c:2224 #8 0x0000000000568398 in GetXLogBuffer (ptr=ptr@entry=112064880, tli=tli@entry=1) at xlog.c:1710 #9 0x000000000055c650 in CopyXLogRecordToWAL (write_len=80, isLogSwitch=false, rdata=0xcc49b0 <hdr_rdt>, StartPos=<optimized out>, EndPos=<optimized out>, tli=1) at xlog.c:1245 #10 XLogInsertRecord (rdata=rdata@entry=0xcc49b0 <hdr_rdt>, fpw_lsn=fpw_lsn@entry=112025520, flags=0 '\000', num_fpi=<optimized out>, num_fpi@entry=0, topxid_included=false) at xlog.c:928 #11 0x000000000056b870 in XLogInsert (rmid=rmid@entry=16 '\020', info=<optimized out>, info@entry=16 '\020') at xloginsert.c:523 #12 0x0000000000537acc in addLeafTuple (index=0xffffebf32950, state=0xffffffffd5e0, leafTuple=0xe43870, current=<optimized out>, parent=<optimized out>, So, yes, something looks really wrong with this patch. Sounds plausible to me that some other buildfarm animals could be stuck without their owners knowing about it. It's proving to be a good idea to force a timeout value in the configuration file of these animals.. -- Michael
-
Re: Get rid of WALBufMappingLock
Alexander Korotkov <aekorotkov@gmail.com> — 2025-02-18T00:29:50Z
On Tue, Feb 18, 2025 at 2:21 AM Michael Paquier <michael@paquier.xyz> wrote: > > On Mon, Feb 17, 2025 at 11:25:05AM -0500, Tom Lane wrote: > > This timeout failure on hachi looks suspicious as well: > > > > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=hachi&dt=2025-02-17%2003%3A05%3A03 > > > > Might be relevant that they are both aarch64? > > Just logged into the host. The logs of the timed out run are still > around, and the last information I can see is from lastcommand.log, > which seems to have frozen in time when the timeout has begun its > vacuuming work: > ok 73 + index_including_gist 353 ms > # parallel group (16 tests): create_cast errors create_aggregate drop_if_exists infinite_recurse > > gokiburi is on the same host, and it is currently frozen in time when > trying to fetch a WAL buffer. One of the stack traces: > #2 0x000000000084ec48 in WaitEventSetWaitBlock (set=0xd34ce0, > cur_timeout=-1, occurred_events=0xffffffffadd8, nevents=1) at > latch.c:1571 > #3 WaitEventSetWait (set=0xd34ce0, timeout=-1, > occurred_events=occurred_events@entry=0xffffffffadd8, > nevents=nevents@entry=1, wait_event_info=<optimized out>, > wait_event_info@entry=134217781) at latch.c:1519 > #4 0x000000000084e964 in WaitLatch (latch=<optimized out>, > wakeEvents=wakeEvents@entry=33, timeout=timeout@entry=-1, > wait_event_info=wait_event_info@entry=134217781) at latch.c:538 > #5 0x000000000085d2f8 in ConditionVariableTimedSleep > (cv=0xffffec0799b0, timeout=-1, wait_event_info=134217781) at > condition_variable.c:163 > #6 0x000000000085d1ec in ConditionVariableSleep > (cv=0xfffffffffffffffc, wait_event_info=1) at condition_variable.c:98 > #7 0x000000000055f4f4 in AdvanceXLInsertBuffer > (upto=upto@entry=112064880, tli=tli@entry=1, opportunistic=false) at > xlog.c:2224 > #8 0x0000000000568398 in GetXLogBuffer (ptr=ptr@entry=112064880, > tli=tli@entry=1) at xlog.c:1710 > #9 0x000000000055c650 in CopyXLogRecordToWAL (write_len=80, > isLogSwitch=false, rdata=0xcc49b0 <hdr_rdt>, StartPos=<optimized out>, > EndPos=<optimized out>, tli=1) at xlog.c:1245 > #10 XLogInsertRecord (rdata=rdata@entry=0xcc49b0 <hdr_rdt>, > fpw_lsn=fpw_lsn@entry=112025520, flags=0 '\000', num_fpi=<optimized > out>, num_fpi@entry=0, topxid_included=false) at xlog.c:928 > #11 0x000000000056b870 in XLogInsert (rmid=rmid@entry=16 '\020', > info=<optimized out>, info@entry=16 '\020') at xloginsert.c:523 > #12 0x0000000000537acc in addLeafTuple (index=0xffffebf32950, > state=0xffffffffd5e0, leafTuple=0xe43870, current=<optimized out>, > parent=<optimized out>, > > So, yes, something looks really wrong with this patch. Sounds > plausible to me that some other buildfarm animals could be stuck > without their owners knowing about it. It's proving to be a good idea > to force a timeout value in the configuration file of these animals.. Tom, Michael, thank you for the information. This patch will be better tested before next attempt. ------ Regards, Alexander Korotkov Supabase
-
Re: Get rid of WALBufMappingLock
Alexander Korotkov <aekorotkov@gmail.com> — 2025-02-25T15:19:29Z
On Tue, Feb 18, 2025 at 2:29 AM Alexander Korotkov <aekorotkov@gmail.com> wrote: > > On Tue, Feb 18, 2025 at 2:21 AM Michael Paquier <michael@paquier.xyz> wrote: > > > > On Mon, Feb 17, 2025 at 11:25:05AM -0500, Tom Lane wrote: > > > This timeout failure on hachi looks suspicious as well: > > > > > > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=hachi&dt=2025-02-17%2003%3A05%3A03 > > > > > > Might be relevant that they are both aarch64? > > > > Just logged into the host. The logs of the timed out run are still > > around, and the last information I can see is from lastcommand.log, > > which seems to have frozen in time when the timeout has begun its > > vacuuming work: > > ok 73 + index_including_gist 353 ms > > # parallel group (16 tests): create_cast errors create_aggregate drop_if_exists infinite_recurse > > > > gokiburi is on the same host, and it is currently frozen in time when > > trying to fetch a WAL buffer. One of the stack traces: > > #2 0x000000000084ec48 in WaitEventSetWaitBlock (set=0xd34ce0, > > cur_timeout=-1, occurred_events=0xffffffffadd8, nevents=1) at > > latch.c:1571 > > #3 WaitEventSetWait (set=0xd34ce0, timeout=-1, > > occurred_events=occurred_events@entry=0xffffffffadd8, > > nevents=nevents@entry=1, wait_event_info=<optimized out>, > > wait_event_info@entry=134217781) at latch.c:1519 > > #4 0x000000000084e964 in WaitLatch (latch=<optimized out>, > > wakeEvents=wakeEvents@entry=33, timeout=timeout@entry=-1, > > wait_event_info=wait_event_info@entry=134217781) at latch.c:538 > > #5 0x000000000085d2f8 in ConditionVariableTimedSleep > > (cv=0xffffec0799b0, timeout=-1, wait_event_info=134217781) at > > condition_variable.c:163 > > #6 0x000000000085d1ec in ConditionVariableSleep > > (cv=0xfffffffffffffffc, wait_event_info=1) at condition_variable.c:98 > > #7 0x000000000055f4f4 in AdvanceXLInsertBuffer > > (upto=upto@entry=112064880, tli=tli@entry=1, opportunistic=false) at > > xlog.c:2224 > > #8 0x0000000000568398 in GetXLogBuffer (ptr=ptr@entry=112064880, > > tli=tli@entry=1) at xlog.c:1710 > > #9 0x000000000055c650 in CopyXLogRecordToWAL (write_len=80, > > isLogSwitch=false, rdata=0xcc49b0 <hdr_rdt>, StartPos=<optimized out>, > > EndPos=<optimized out>, tli=1) at xlog.c:1245 > > #10 XLogInsertRecord (rdata=rdata@entry=0xcc49b0 <hdr_rdt>, > > fpw_lsn=fpw_lsn@entry=112025520, flags=0 '\000', num_fpi=<optimized > > out>, num_fpi@entry=0, topxid_included=false) at xlog.c:928 > > #11 0x000000000056b870 in XLogInsert (rmid=rmid@entry=16 '\020', > > info=<optimized out>, info@entry=16 '\020') at xloginsert.c:523 > > #12 0x0000000000537acc in addLeafTuple (index=0xffffebf32950, > > state=0xffffffffd5e0, leafTuple=0xe43870, current=<optimized out>, > > parent=<optimized out>, > > > > So, yes, something looks really wrong with this patch. Sounds > > plausible to me that some other buildfarm animals could be stuck > > without their owners knowing about it. It's proving to be a good idea > > to force a timeout value in the configuration file of these animals.. > > Tom, Michael, thank you for the information. > This patch will be better tested before next attempt. It seems that I managed to reproduce the issue on my Raspberry PI 4. After running our test suite in a loop for 2 days I found one timeout. I have hypothesis on why it might happen. We don't have protection against two backends in parallel get ReservedPtr mapped to a single XLog buffer. I've talked to Yura off-list about that. He pointer out that XLogWrite() should issue a PANIC in that case, which we didn't observe. However, I'm not sure this analysis is complete. One way or another, we need protection against this situation any way. The updated patch is attached. Now, after acquiring ReservedPtr it waits till OldPageRqstPtr gets initialized. Additionally I've to implement more accurate calculation of OldPageRqstPtr. I run tests with new patch on my Raspberry in a loop. Let's see how it goes. ------ Regards, Alexander Korotkov Supabase
-
Re: Get rid of WALBufMappingLock
Michael Paquier <michael@paquier.xyz> — 2025-02-26T01:03:57Z
On Tue, Feb 25, 2025 at 05:19:29PM +0200, Alexander Korotkov wrote: > It seems that I managed to reproduce the issue on my Raspberry PI 4. > After running our test suite in a loop for 2 days I found one timeout. Hmm. It's surprising to not see a higher occurence. My buildfarm host has caught that on its first run after the patch, for two different animals which are both on the same machine. > One way or another, we need protection against this situation any way. > The updated patch is attached. Now, after acquiring ReservedPtr it > waits till OldPageRqstPtr gets initialized. Additionally I've to > implement more accurate calculation of OldPageRqstPtr. I run tests > with new patch on my Raspberry in a loop. Let's see how it goes. Perhaps you'd prefer that I do more tests with your patch? This is time-consuming for you. This is not a review of the internals of the patch, and I cannot give you access to the host, but if my stuff is the only place where we have a good reproducibility of the issue, I'm OK to grab some time and run a couple of checks to avoid again a freeze of the buildfarm. -- Michael
-
Re: Get rid of WALBufMappingLock
Andrey Borodin <x4mmm@yandex-team.ru> — 2025-02-26T08:52:17Z
> On 25 Feb 2025, at 20:19, Alexander Korotkov <aekorotkov@gmail.com> wrote: > > Hi! One little piece of code looks suspicious to me. But I was not raising concern because I see similar code everywhere in the codebase. But know Kirill asked to me explain what is going on and I cannot. This seems to be relevant… so. + while (upto >= pg_atomic_read_u64(&XLogCtl->InitializedUpTo)) // Assume ConditionVariableBroadcast() happened here, but before next line + ConditionVariableSleep(&XLogCtl->InitializedUpToCondVar, WAIT_EVENT_WAL_BUFFER_INIT); + ConditionVariableCancelSleep(); Won’t this sleep wait forever? I see about 20 other occurrences of similar code, so, perhaps, everything is fine. But I would greatly appreciate a little pointers on why it works. Best regards, Andrey Borodin.
-
Re: Get rid of WALBufMappingLock
Alexander Korotkov <aekorotkov@gmail.com> — 2025-02-26T11:48:47Z
Hi, Michael! On Wed, Feb 26, 2025 at 3:04 AM Michael Paquier <michael@paquier.xyz> wrote: > > On Tue, Feb 25, 2025 at 05:19:29PM +0200, Alexander Korotkov wrote: > > It seems that I managed to reproduce the issue on my Raspberry PI 4. > > After running our test suite in a loop for 2 days I found one timeout. > > Hmm. It's surprising to not see a higher occurence. My buildfarm > host has caught that on its first run after the patch, for two > different animals which are both on the same machine. > > > One way or another, we need protection against this situation any way. > > The updated patch is attached. Now, after acquiring ReservedPtr it > > waits till OldPageRqstPtr gets initialized. Additionally I've to > > implement more accurate calculation of OldPageRqstPtr. I run tests > > with new patch on my Raspberry in a loop. Let's see how it goes. > > Perhaps you'd prefer that I do more tests with your patch? This is > time-consuming for you. This is not a review of the internals of the > patch, and I cannot give you access to the host, but if my stuff is > the only place where we have a good reproducibility of the issue, I'm > OK to grab some time and run a couple of checks to avoid again a > freeze of the buildfarm. Thank you for offering the help. Updated version of patch is attached (I've added one memory barrier there just in case). I would appreciate if you could run it on batta, hachi or similar hardware. ------ Regards, Alexander Korotkov Supabase
-
Re: Get rid of WALBufMappingLock
Michael Paquier <michael@paquier.xyz> — 2025-02-28T07:27:29Z
On Wed, Feb 26, 2025 at 01:48:47PM +0200, Alexander Korotkov wrote: > Thank you for offering the help. Updated version of patch is attached > (I've added one memory barrier there just in case). I would > appreciate if you could run it on batta, hachi or similar hardware. Doing a revert of the revert done in 3fb58625d18f proves that reproducing the error is not really difficult. I've done a make installcheck-world USE_MODULE_DB=1 -j N without assertions, and the point that saw a failure quickly is one of the tests of pgbench: PANIC: could not find WAL buffer for 0/19D366 This one happened for the test "concurrent OID generation" and CREATE TYPE. Of course, as it is a race condition, it is random, but it's taking me only a couple of minutes to see the original issue on my buildfarm host. With assertion failures enabled, same story, and same failure from the pgbench TAP test. Saying that, I have also done similar tests with your v12 for a couple of hours and this looks stable under installcheck-world. I can see that you've reworked quite a bit the surroundings of InitializedFrom in this one. If you apply that once again at some point, the buildfarm will be judge in the long-term, but I am rather confident by saying that the situation looks better here, at least. One thing I would consider doing if you want to gain confidence is tests like the one I saw causing problems with pgbench, with DDL patterns stressing specific paths like this CREATE TYPE case. -- Michael
-
Re: Get rid of WALBufMappingLock
Yura Sokolov <y.sokolov@postgrespro.ru> — 2025-02-28T12:12:36Z
26.02.2025 11:52, Andrey Borodin wrote: >> On 25 Feb 2025, at 20:19, Alexander Korotkov <aekorotkov@gmail.com> wrote: >> >> > > > Hi! > > One little piece of code looks suspicious to me. But I was not raising concern because I see similar code everywhere in the codebase. But know Kirill asked to me explain what is going on and I cannot. > > This seems to be relevant… so. > > + while (upto >= pg_atomic_read_u64(&XLogCtl->InitializedUpTo)) > // Assume ConditionVariableBroadcast() happened here, but before next line > + ConditionVariableSleep(&XLogCtl->InitializedUpToCondVar, WAIT_EVENT_WAL_BUFFER_INIT); > + ConditionVariableCancelSleep(); > > Won’t this sleep wait forever? Because ConditionVariableSleep doesn't sleep for the first time. It just performs ConditionVariablePrepareToSleep and immediately returns. So actual condition of `while` loop is checked at least twice before going to sleep. > > I see about 20 other occurrences of similar code, so, perhaps, everything is fine. But I would greatly appreciate a little pointers on why it works. ------- regards Yura Sokolov aka funny-falcon
-
Re: Get rid of WALBufMappingLock
Yura Sokolov <y.sokolov@postgrespro.ru> — 2025-02-28T12:24:14Z
26.02.2025 14:48, Alexander Korotkov пишет: > Hi, Michael! > > On Wed, Feb 26, 2025 at 3:04 AM Michael Paquier <michael@paquier.xyz> wrote: >> >> On Tue, Feb 25, 2025 at 05:19:29PM +0200, Alexander Korotkov wrote: >>> It seems that I managed to reproduce the issue on my Raspberry PI 4. >>> After running our test suite in a loop for 2 days I found one timeout. >> >> Hmm. It's surprising to not see a higher occurence. My buildfarm >> host has caught that on its first run after the patch, for two >> different animals which are both on the same machine. >> >>> One way or another, we need protection against this situation any way. >>> The updated patch is attached. Now, after acquiring ReservedPtr it >>> waits till OldPageRqstPtr gets initialized. Additionally I've to >>> implement more accurate calculation of OldPageRqstPtr. I run tests >>> with new patch on my Raspberry in a loop. Let's see how it goes. >> >> Perhaps you'd prefer that I do more tests with your patch? This is >> time-consuming for you. This is not a review of the internals of the >> patch, and I cannot give you access to the host, but if my stuff is >> the only place where we have a good reproducibility of the issue, I'm >> OK to grab some time and run a couple of checks to avoid again a >> freeze of the buildfarm. > > Thank you for offering the help. Updated version of patch is attached > (I've added one memory barrier there just in case). I would > appreciate if you could run it on batta, hachi or similar hardware. Good day, Alexander. Checked your additions to patch. They're clear and robust. ------- regards Yura Sokolov aka funny-falcon
-
Re: Get rid of WALBufMappingLock
Alvaro Herrera <alvherre@alvh.no-ip.org> — 2025-02-28T13:13:23Z
On 2025-Feb-28, Michael Paquier wrote: > Saying that, I have also done similar tests with your v12 for a couple > of hours and this looks stable under installcheck-world. I can see > that you've reworked quite a bit the surroundings of InitializedFrom > in this one. If you apply that once again at some point, the > buildfarm will be judge in the long-term, but I am rather confident by > saying that the situation looks better here, at least. Heh, no amount of testing can prove lack of bugs; but for sure "it looks different now, so it must be correct" must be the weakest proof of correctness I've heard of! -- Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/ "I am amazed at [the pgsql-sql] mailing list for the wonderful support, and lack of hesitasion in answering a lost soul's question, I just wished the rest of the mailing list could be like this." (Fotis) https://postgr.es/m/200606261359.k5QDxE2p004593@auth-smtp.hol.gr -
Re: Get rid of WALBufMappingLock
Michael Paquier <michael@paquier.xyz> — 2025-02-28T13:43:58Z
On Fri, Feb 28, 2025 at 02:13:23PM +0100, Álvaro Herrera wrote: > On 2025-Feb-28, Michael Paquier wrote: >> Saying that, I have also done similar tests with your v12 for a couple >> of hours and this looks stable under installcheck-world. I can see >> that you've reworked quite a bit the surroundings of InitializedFrom >> in this one. If you apply that once again at some point, the >> buildfarm will be judge in the long-term, but I am rather confident by >> saying that the situation looks better here, at least. > > Heh, no amount of testing can prove lack of bugs; but for sure "it looks > different now, so it must be correct" must be the weakest proof of > correctness I've heard of! Err, okay. I did use the word "stable" with tests rather than "correct", and I implied upthread that I did not check the correctness nor the internals of the patch. If my words held the meaning you are implying, well, my apologies for the confusion, I guess. I only tested the patch and it was stable while I've noticed a few diffs with the previous version, but I did *not* check its internals at all, nor do I mean that I endorse its logic. I hope that's clear now. -- Michael
-
Re: Get rid of WALBufMappingLock
Alexander Korotkov <aekorotkov@gmail.com> — 2025-03-02T11:58:36Z
On Fri, Feb 28, 2025 at 3:13 PM Álvaro Herrera <alvherre@alvh.no-ip.org> wrote: > On 2025-Feb-28, Michael Paquier wrote: > > > Saying that, I have also done similar tests with your v12 for a couple > > of hours and this looks stable under installcheck-world. I can see > > that you've reworked quite a bit the surroundings of InitializedFrom > > in this one. If you apply that once again at some point, the > > buildfarm will be judge in the long-term, but I am rather confident by > > saying that the situation looks better here, at least. > > Heh, no amount of testing can prove lack of bugs; but for sure "it looks > different now, so it must be correct" must be the weakest proof of > correctness I've heard of! Michael just volunteered to help Yura and me with testing. He wan't intended to be reviewer. And he reported that tests looks much more stable now. I think he is absolutely correct with this. ------ Regards, Alexander Korotkov Supabase
-
Re: Get rid of WALBufMappingLock
Alexander Korotkov <aekorotkov@gmail.com> — 2025-03-02T11:59:17Z
On Fri, Feb 28, 2025 at 3:44 PM Michael Paquier <michael@paquier.xyz> wrote: > > On Fri, Feb 28, 2025 at 02:13:23PM +0100, Álvaro Herrera wrote: > > On 2025-Feb-28, Michael Paquier wrote: > >> Saying that, I have also done similar tests with your v12 for a couple > >> of hours and this looks stable under installcheck-world. I can see > >> that you've reworked quite a bit the surroundings of InitializedFrom > >> in this one. If you apply that once again at some point, the > >> buildfarm will be judge in the long-term, but I am rather confident by > >> saying that the situation looks better here, at least. > > > > Heh, no amount of testing can prove lack of bugs; but for sure "it looks > > different now, so it must be correct" must be the weakest proof of > > correctness I've heard of! > > Err, okay. I did use the word "stable" with tests rather than > "correct", and I implied upthread that I did not check the correctness > nor the internals of the patch. If my words held the meaning you > are implying, well, my apologies for the confusion, I guess. I only > tested the patch and it was stable while I've noticed a few diffs with > the previous version, but I did *not* check its internals at all, nor > do I mean that I endorse its logic. I hope that's clear now. Got it. Michael, thank you very much for your help. ------ Regards, Alexander Korotkov Supabase
-
Re: Get rid of WALBufMappingLock
Alexander Korotkov <aekorotkov@gmail.com> — 2025-03-07T15:08:47Z
On Sun, Mar 2, 2025 at 1:58 PM Alexander Korotkov <aekorotkov@gmail.com> wrote: > > On Fri, Feb 28, 2025 at 3:13 PM Álvaro Herrera <alvherre@alvh.no-ip.org> wrote: > > On 2025-Feb-28, Michael Paquier wrote: > > > > > Saying that, I have also done similar tests with your v12 for a couple > > > of hours and this looks stable under installcheck-world. I can see > > > that you've reworked quite a bit the surroundings of InitializedFrom > > > in this one. If you apply that once again at some point, the > > > buildfarm will be judge in the long-term, but I am rather confident by > > > saying that the situation looks better here, at least. > > > > Heh, no amount of testing can prove lack of bugs; but for sure "it looks > > different now, so it must be correct" must be the weakest proof of > > correctness I've heard of! > > Michael just volunteered to help Yura and me with testing. He wan't > intended to be reviewer. And he reported that tests looks much more > stable now. I think he is absolutely correct with this. Nevertheless, I don't think the bug has gone in v12. I managed to reproduce it on my local Raspberry PI 4. The attached version of patch fixes the bug for me. It adds memory barriers surrounding pg_atomic_compare_exchange_u64(). That certainly not right given this function should already provide full memory barrier semantics. But my investigation shows it doesn't. I'm going to start a separate thread about this. Also, new version of patch contains fix of potential integer overflow during OldPageRqstPtr computation sent off-list my me by Yura. ------ Regards, Alexander Korotkov Supabase
-
Re: Get rid of WALBufMappingLock
Alexander Korotkov <aekorotkov@gmail.com> — 2025-03-13T21:37:59Z
On Fri, Mar 7, 2025 at 5:08 PM Alexander Korotkov <aekorotkov@gmail.com> wrote: > On Sun, Mar 2, 2025 at 1:58 PM Alexander Korotkov <aekorotkov@gmail.com> wrote: > > > > On Fri, Feb 28, 2025 at 3:13 PM Álvaro Herrera <alvherre@alvh.no-ip.org> wrote: > > > On 2025-Feb-28, Michael Paquier wrote: > > > > > > > Saying that, I have also done similar tests with your v12 for a couple > > > > of hours and this looks stable under installcheck-world. I can see > > > > that you've reworked quite a bit the surroundings of InitializedFrom > > > > in this one. If you apply that once again at some point, the > > > > buildfarm will be judge in the long-term, but I am rather confident by > > > > saying that the situation looks better here, at least. > > > > > > Heh, no amount of testing can prove lack of bugs; but for sure "it looks > > > different now, so it must be correct" must be the weakest proof of > > > correctness I've heard of! > > > > Michael just volunteered to help Yura and me with testing. He wan't > > intended to be reviewer. And he reported that tests looks much more > > stable now. I think he is absolutely correct with this. > > Nevertheless, I don't think the bug has gone in v12. I managed to > reproduce it on my local Raspberry PI 4. The attached version of > patch fixes the bug for me. It adds memory barriers surrounding > pg_atomic_compare_exchange_u64(). That certainly not right given this > function should already provide full memory barrier semantics. But my > investigation shows it doesn't. I'm going to start a separate thread > about this. > > Also, new version of patch contains fix of potential integer overflow > during OldPageRqstPtr computation sent off-list my me by Yura. So, as we finally clarified CAS doesn't guarantee full memory barrier on failure. Also, it's not clear when barriers are guaranteed on success. In ARM without LSE implementation, read barrier is provided before change of value and write barrier after change of value. So, it appears that both explicit barriers I've added are required. This revision also comes with format proof of the algorithm. ------ Regards, Alexander Korotkov Supabase
-
Re: Get rid of WALBufMappingLock
Tomas Vondra <tomas@vondra.me> — 2025-03-14T14:30:01Z
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 -
Re: Get rid of WALBufMappingLock
Yura Sokolov <y.sokolov@postgrespro.ru> — 2025-03-21T11:02:19Z
Good day, Tomas 14.03.2025 17:30, Tomas Vondra wrote: > > 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. Yes, I found this bottleneck when I did experiments with increasing NUM_XLOGINSERT_LOCKS [1]. For this patch to be more valuable, there should be more parallel xlog inserters. That is why I initially paired this patch with patch that reduces contention on WALInsertLocks ("0002-Several attempts to lock WALInsertLock", last version at [2]). Certainly, largest bottleneck is WALWriteLock around writting buffers and especially fsync-ing them after write. But this intermediate bottleneck of WALBufMappingLock is also worth to be removed. [1] https://postgr.es/m/flat/3b11fdc2-9793-403d-b3d4-67ff9a00d447%40postgrespro.ru [2] https://postgr.es/m/c31158a3-7c26-4b26-90df-2df8f7bbe736%40postgrespro.ru ------- regards Yura Sokolov aka funny-falcon -
Re: Get rid of WALBufMappingLock
Yura Sokolov <y.sokolov@postgrespro.ru> — 2025-03-31T10:42:01Z
Good day, 14.03.2025 17:30, Tomas Vondra wrote: > 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? I've made similar benchmark on system with two Xeon Gold 5220R with two Samsung SSD 970 PRO 1TB mirrored by md. Configuration changes: wal_sync_method = open_datasync full_page_writes = off synchronous_commit = off checkpoint_timeout = 1d max_connections = 1000 max_wal_size = 4GB min_wal_size = 640MB I variated wal segment size (16MB and 64MB), wal_buffers (128kB, 16MB and 1GB) and record size (1kB, 8kB and 64kB). (I didn't bench 1MB record size, since I don't believe it is critical for performance). Here's results for 64MB segment size and 1GB wal_buffers: +---------+---------+------------+--------------+----------+ | recsize | clients | master_tps | nowalbuf_tps | rel_perf | +---------+---------+------------+--------------+----------+ | 1 | 1 | 47991.0 | 46995.0 | 0.98 | | 1 | 4 | 171930.0 | 171166.0 | 1.0 | | 1 | 16 | 491240.0 | 485132.0 | 0.99 | | 1 | 64 | 514590.0 | 515534.0 | 1.0 | | 1 | 128 | 547222.0 | 543543.0 | 0.99 | | 1 | 256 | 543353.0 | 540802.0 | 1.0 | | 8 | 1 | 40976.0 | 41603.0 | 1.02 | | 8 | 4 | 89003.0 | 92008.0 | 1.03 | | 8 | 16 | 90457.0 | 92282.0 | 1.02 | | 8 | 64 | 89293.0 | 92022.0 | 1.03 | | 8 | 128 | 92687.0 | 92768.0 | 1.0 | | 8 | 256 | 91874.0 | 91665.0 | 1.0 | | 64 | 1 | 11829.0 | 12031.0 | 1.02 | | 64 | 4 | 11959.0 | 12832.0 | 1.07 | | 64 | 16 | 11331.0 | 13417.0 | 1.18 | | 64 | 64 | 11108.0 | 13588.0 | 1.22 | | 64 | 128 | 11089.0 | 13648.0 | 1.23 | | 64 | 256 | 10381.0 | 13542.0 | 1.3 | +---------+---------+------------+--------------+----------+ Numbers for all configurations in attached 'improvements.out' . It shows, removing WALBufMappingLock almost always doesn't harm performance and usually gives measurable gain. (Numbers are average from 4 middle runs out of 6. i.e. I threw minimum and maximum tps from 6 runs and took average from remaining). Also sqlite database is attached with all results. It also contains results for patch "Several attempts to lock WALInsertLock" (named "attempts") and cumulative patch ("nowalbuf-attempts"). Suprisingly, "Several attempts" causes measurable impact in some configurations with hundreds of clients. So, there're more bottlenecks ahead )) Yes, it is still not "real-world" benchmark. But it at least shows patch is harmless. -- regards Yura Sokolov aka funny-falcon -
Re: Get rid of WALBufMappingLock
Alexander Korotkov <aekorotkov@gmail.com> — 2025-03-31T18:18:30Z
On Mon, Mar 31, 2025 at 1:42 PM Yura Sokolov <y.sokolov@postgrespro.ru> wrote: > 14.03.2025 17:30, Tomas Vondra wrote: > > 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? > > I've made similar benchmark on system with two Xeon Gold 5220R with two > Samsung SSD 970 PRO 1TB mirrored by md. > > Configuration changes: > wal_sync_method = open_datasync > full_page_writes = off > synchronous_commit = off > checkpoint_timeout = 1d > max_connections = 1000 > max_wal_size = 4GB > min_wal_size = 640MB > > I variated wal segment size (16MB and 64MB), wal_buffers (128kB, 16MB and > 1GB) and record size (1kB, 8kB and 64kB). > > (I didn't bench 1MB record size, since I don't believe it is critical for > performance). > > Here's results for 64MB segment size and 1GB wal_buffers: > > +---------+---------+------------+--------------+----------+ > | recsize | clients | master_tps | nowalbuf_tps | rel_perf | > +---------+---------+------------+--------------+----------+ > | 1 | 1 | 47991.0 | 46995.0 | 0.98 | > | 1 | 4 | 171930.0 | 171166.0 | 1.0 | > | 1 | 16 | 491240.0 | 485132.0 | 0.99 | > | 1 | 64 | 514590.0 | 515534.0 | 1.0 | > | 1 | 128 | 547222.0 | 543543.0 | 0.99 | > | 1 | 256 | 543353.0 | 540802.0 | 1.0 | > | 8 | 1 | 40976.0 | 41603.0 | 1.02 | > | 8 | 4 | 89003.0 | 92008.0 | 1.03 | > | 8 | 16 | 90457.0 | 92282.0 | 1.02 | > | 8 | 64 | 89293.0 | 92022.0 | 1.03 | > | 8 | 128 | 92687.0 | 92768.0 | 1.0 | > | 8 | 256 | 91874.0 | 91665.0 | 1.0 | > | 64 | 1 | 11829.0 | 12031.0 | 1.02 | > | 64 | 4 | 11959.0 | 12832.0 | 1.07 | > | 64 | 16 | 11331.0 | 13417.0 | 1.18 | > | 64 | 64 | 11108.0 | 13588.0 | 1.22 | > | 64 | 128 | 11089.0 | 13648.0 | 1.23 | > | 64 | 256 | 10381.0 | 13542.0 | 1.3 | > +---------+---------+------------+--------------+----------+ > > Numbers for all configurations in attached 'improvements.out' . It shows, > removing WALBufMappingLock almost always doesn't harm performance and > usually gives measurable gain. > > (Numbers are average from 4 middle runs out of 6. i.e. I threw minimum and > maximum tps from 6 runs and took average from remaining). > > Also sqlite database is attached with all results. It also contains results > for patch "Several attempts to lock WALInsertLock" (named "attempts") and > cumulative patch ("nowalbuf-attempts"). > Suprisingly, "Several attempts" causes measurable impact in some > configurations with hundreds of clients. So, there're more bottlenecks ahead )) > > > Yes, it is still not "real-world" benchmark. But it at least shows patch is > harmless. Thank you for your experiments. Your results shows up to 30% speedups on real hardware, not tmpfs. While this is still a corner case, I think this is quite a results for a pretty local optimization. On small connection number there are some cases above and below 1.0. I think this due to statistical error. If we would calculate average tps ratio across different experiments, for low number of clients it's still above 1.0. sqlite> select clients, avg(ratio) from (select walseg, walbuf, recsize, clients, (avg(tps) filter (where branch = 'nowalbuf'))/(avg(tps) filter (where branch = 'master')) as ratio from results where branch in ('master', 'nowalbuf') group by walseg, walbuf, recsize, clients) x group by clients; 1|1.00546614169766 4|1.00782085856889 16|1.02257892337757 64|1.04400167838906 128|1.04134006876033 256|1.04627949500578 I'm going to push the first patch ("nowalbuf") if no objections. I think the second one ("Several attempts") still needs more work, as there are regressions. ------ Regards, Alexander Korotkov Supabase -
Re: Get rid of WALBufMappingLock
Noah Misch <noah@leadboat.com> — 2025-07-05T04:15:59Z
On Mon, Mar 31, 2025 at 09:18:30PM +0300, Alexander Korotkov wrote: > I'm going to push the first patch ("nowalbuf") if no objections. I completed a post-commit review of this patch. I think the patch is correct. I found some of the variable names and comments challenging, so I made the attached local edits in the course of the review. I don't know whether my choices are better or just tailored to my biases. If someone thinks this would improve the tree, I can polish this and commit it. Absent that, consider these notes to myself and to other post-commit reviewers. -
Re: Get rid of WALBufMappingLock
Alexander Korotkov <aekorotkov@gmail.com> — 2025-07-06T22:48:06Z
On Sat, Jul 5, 2025 at 7:16 AM Noah Misch <noah@leadboat.com> wrote: > > On Mon, Mar 31, 2025 at 09:18:30PM +0300, Alexander Korotkov wrote: > > I'm going to push the first patch ("nowalbuf") if no objections. > > I completed a post-commit review of this patch. I think the patch is correct. Great. I'm glad to hear that. > I found some of the variable names and comments challenging, so I made the > attached local edits in the course of the review. I don't know whether my > choices are better or just tailored to my biases. If someone thinks this > would improve the tree, I can polish this and commit it. Absent that, > consider these notes to myself and to other post-commit reviewers. I took a look to these edits. They look good to me. ------ Regards, Alexander Korotkov Supabase