Thread: Decouple last important WAL record LSN from WAL insert locks
Hi,
While working on something else, I noticed that each WAL insert lock
tracks its own last important WAL record's LSN (lastImportantAt) and
both the bgwriter and checkpointer later computes the max
value/server-wide last important WAL record's LSN via
GetLastImportantRecPtr(). While doing so, each WAL insertion lock is
acquired in exclusive mode in a for loop. This seems like too much
overhead to me. I quickly coded a patch (attached herewith) that
tracks the server-wide last important WAL record's LSN in
XLogCtlInsert (lastImportantPos) protected with a spinlock and gets
rid of lastImportantAt from each WAL insert lock. I ran pgbench with a
simple insert [1] and the results are below. While the test was run,
the GetLastImportantRecPtr() was called 4-5 times.
# of clients HEAD PATCHED
1 83 82
2 159 157
4 303 302
8 576 570
16 1104 1095
32 2055 2041
64 2286 2295
128 2270 2285
256 2302 2253
512 2205 2290
768 2224 2180
1024 2109 2150
2048 1941 1936
4096 1856 1848
It doesn't seem to hurt (for this use-case) anyone, however there
might be some benefit if bgwriter and checkpointer come in the way of
WAL inserters. With the patch, the extra exclusive lock burden on WAL
insert locks is gone. Since the amount of work the WAL inserters do
under the new spinlock is very minimal (updating
XLogCtlInsert->lastImportantPos), it may not be an issue. Also, it's
worthwhile to look at the existing comment [2], which doesn't talk
about the performance impact of having a lock.
Thoughts?
[1]
./configure --prefix=$PWD/inst/ CFLAGS="-O3" > install.log && make -j
8 install > install.log 2>&1 &
cd inst/bin
./pg_ctl -D data -l logfile stop
rm -rf data logfile insert.sql
free -m
sudo su -c 'sync; echo 3 > /proc/sys/vm/drop_caches'
free -m
./initdb -D data
./pg_ctl -D data -l logfile start
./psql -d postgres -c 'ALTER SYSTEM SET shared_buffers = "8GB";'
./psql -d postgres -c 'ALTER SYSTEM SET max_wal_size = "32GB";'
./psql -d postgres -c 'ALTER SYSTEM SET max_connections = "4096";'
./psql -d postgres -c 'ALTER SYSTEM SET bgwriter_delay = "10ms";'
./pg_ctl -D data -l logfile restart
./pgbench -i -s 1 -d postgres
./psql -d postgres -c "ALTER TABLE pgbench_accounts DROP CONSTRAINT
pgbench_accounts_pkey;"
cat << EOF >> insert.sql
\set aid random(1, 10 * :scale)
\set delta random(1, 100000 * :scale)
INSERT INTO pgbench_accounts (aid, bid, abalance) VALUES (:aid, :aid, :delta);
EOF
for c in 1 2 4 8 16 32 64 128 256 512 768 1024 2048 4096; do echo -n
"$c ";./pgbench -n -M prepared -U ubuntu postgres -b simple-update
-c$c -j$c -T5 2>&1|grep '^tps'|awk '{print $3}';done
[2]
* records. Tracking the WAL activity directly in WALInsertLock has the
* advantage of not needing any additional locks to update the value.
--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
Attachment
Hi, On 2022-11-23 19:12:07 +0530, Bharath Rupireddy wrote: > While working on something else, I noticed that each WAL insert lock > tracks its own last important WAL record's LSN (lastImportantAt) and > both the bgwriter and checkpointer later computes the max > value/server-wide last important WAL record's LSN via > GetLastImportantRecPtr(). While doing so, each WAL insertion lock is > acquired in exclusive mode in a for loop. This seems like too much > overhead to me. GetLastImportantRecPtr() should be a very rare operation, so it's fine for it to be expensive. The important thing is for the maintenance of the underlying data to be very cheap. > I quickly coded a patch (attached herewith) that > tracks the server-wide last important WAL record's LSN in > XLogCtlInsert (lastImportantPos) protected with a spinlock and gets > rid of lastImportantAt from each WAL insert lock. That strikes me as a very bad idea. It adds another point of contention to a very very hot code path, to make a very rare code path cheaper. Greetings, Andres Freund
On Sun, Nov 27, 2022 at 2:43 AM Andres Freund <andres@anarazel.de> wrote: > > Hi, > > On 2022-11-23 19:12:07 +0530, Bharath Rupireddy wrote: > > While working on something else, I noticed that each WAL insert lock > > tracks its own last important WAL record's LSN (lastImportantAt) and > > both the bgwriter and checkpointer later computes the max > > value/server-wide last important WAL record's LSN via > > GetLastImportantRecPtr(). While doing so, each WAL insertion lock is > > acquired in exclusive mode in a for loop. This seems like too much > > overhead to me. > > GetLastImportantRecPtr() should be a very rare operation, so it's fine for it > to be expensive. The important thing is for the maintenance of the underlying > data to be very cheap. > > > I quickly coded a patch (attached herewith) that > > tracks the server-wide last important WAL record's LSN in > > XLogCtlInsert (lastImportantPos) protected with a spinlock and gets > > rid of lastImportantAt from each WAL insert lock. > > That strikes me as a very bad idea. It adds another point of contention to a > very very hot code path, to make a very rare code path cheaper. Thanks for the response. I agree that GetLastImportantRecPtr() gets called rarely, however, what concerns me is that it's taking all the WAL insertion locks when it gets called. Is tracking lastImportantPos as pg_atomic_uint64 in XLogCtlInsert any better than an explicit spinlock? I think it's better on platforms where atomics are supported, however, it boils down to using a spin lock on the platforms where atomics aren't supported. -- Bharath Rupireddy PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Hi, On 2022-11-28 11:42:19 +0530, Bharath Rupireddy wrote: > On Sun, Nov 27, 2022 at 2:43 AM Andres Freund <andres@anarazel.de> wrote: > > On 2022-11-23 19:12:07 +0530, Bharath Rupireddy wrote: > > > While working on something else, I noticed that each WAL insert lock > > > tracks its own last important WAL record's LSN (lastImportantAt) and > > > both the bgwriter and checkpointer later computes the max > > > value/server-wide last important WAL record's LSN via > > > GetLastImportantRecPtr(). While doing so, each WAL insertion lock is > > > acquired in exclusive mode in a for loop. This seems like too much > > > overhead to me. > > > > GetLastImportantRecPtr() should be a very rare operation, so it's fine for it > > to be expensive. The important thing is for the maintenance of the underlying > > data to be very cheap. > > > > > I quickly coded a patch (attached herewith) that > > > tracks the server-wide last important WAL record's LSN in > > > XLogCtlInsert (lastImportantPos) protected with a spinlock and gets > > > rid of lastImportantAt from each WAL insert lock. > > > > That strikes me as a very bad idea. It adds another point of contention to a > > very very hot code path, to make a very rare code path cheaper. > > Thanks for the response. I agree that GetLastImportantRecPtr() gets > called rarely, however, what concerns me is that it's taking all the > WAL insertion locks when it gets called. So what? It's far from the only operation doing so. And in contrast to most of the other places (c.f. WALInsertLockAcquireExclusive()) it only takes one of them at a time. > Is tracking lastImportantPos as pg_atomic_uint64 in XLogCtlInsert any > better than an explicit spinlock? I think it's better on platforms > where atomics are supported, however, it boils down to using a spin > lock on the platforms where atomics aren't supported. A central atomic in XLogCtlInsert would be better than a spinlock protected variable, but still bad. We do *not* want to have more central state that needs to be manipulated, we want *less*. If we wanted to optimize this - and I haven't seen any evidence it's worth doing so - we should just optimize the lock acquisitions in GetLastImportantRecPtr() away. *Without* centralizing the state. Greetings, Andres Freund
On Mon, Nov 28, 2022 at 11:55 PM Andres Freund <andres@anarazel.de> wrote: > > > Is tracking lastImportantPos as pg_atomic_uint64 in XLogCtlInsert any > > better than an explicit spinlock? I think it's better on platforms > > where atomics are supported, however, it boils down to using a spin > > lock on the platforms where atomics aren't supported. > > A central atomic in XLogCtlInsert would be better than a spinlock protected > variable, but still bad. We do *not* want to have more central state that > needs to be manipulated, we want *less*. Agreed. > If we wanted to optimize this - and I haven't seen any evidence it's worth > doing so - we should just optimize the lock acquisitions in > GetLastImportantRecPtr() away. *Without* centralizing the state. Hm. I can think of converting lastImportantAt from XLogRecPtr to pg_atomic_uint64 and letting it stay within the WALInsertLock structure. This prevents torn-reads and also avoids WAL insertion lock acquire-release cycles in GetLastImportantRecPtr(). Please see the attached patch herewith. If this idea is worth it, I would like to bring this and the other thread [1] that converts insertingAt to atomic and modifies other WAL insert locks related code under one roof and start a new thread. BTW, the patch at [1] seems to be showing a good benefit for high-concurrent inserts with small records. Thoughts? [1] https://www.postgresql.org/message-id/CALj2ACWkWbheFhkPwMw83CUpzHFGXSV_HXTBxG9%2B-PZ3ufHE%3DQ%40mail.gmail.com -- Bharath Rupireddy PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com