Feedback-ID: i0fe9450f:Fastmail
Date: Mon, 10 Apr 2023 13:08:43 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Nathan Bossart <nathandbossart@gmail.com>
Cc: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>,
	Andres Freund <andres@anarazel.de>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: WAL Insertion Lock Improvements
Message-ID: <ZDOLy4frwhnBisis@paquier.xyz>
References: <CALj2ACWkWbheFhkPwMw83CUpzHFGXSV_HXTBxG9+-PZ3ufHE=Q@mail.gmail.com>
 <20221202004042.qwjadmkpq4zky3tn@awork3.anarazel.de>
 <CALj2ACXYpYDboe4i+yXDYoNQj9-G0584hP5x_b=W6hj693qcaA@mail.gmail.com>
 <20221203003158.GA2518778@nathanxps13>
 <20221205183007.s72oygp63s43dqyz@awork3.anarazel.de>
 <CALj2ACUc=Hcd3X+mBNAF2o9Y3r=cxVoz4JBayBTz=jw74KopHQ@mail.gmail.com>
 <CALj2ACUt-YdzxdGZPcr0LcERf0Hv2sTbz4ZvUftQ8Dgk4aVRfA@mail.gmail.com>
 <20230208220603.GA546776@nathanxps13>
 <CALj2ACULfpGBpXY3UPLegFbk0zFUCYvHVom09gwAbQ+NsJTHaw@mail.gmail.com>
 <20230221054948.GA2717@nathanxps13>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="a3LqHsA1XvzfEZZ9"
Content-Disposition: inline
In-Reply-To: <20230221054948.GA2717@nathanxps13>
Archived-At: <https://www.postgresql.org/message-id/ZDOLy4frwhnBisis%40paquier.xyz>
Precedence: bulk


--a3LqHsA1XvzfEZZ9
Content-Type: multipart/mixed; boundary="DKfGcTxj0xWBOD6a"
Content-Disposition: inline


--DKfGcTxj0xWBOD6a
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Mon, Feb 20, 2023 at 09:49:48PM -0800, Nathan Bossart wrote:
> I'm marking this as ready-for-committer.  I think a couple of the comments
> could use some small adjustments, but that probably doesn't need to hold =
up
> this patch.

Apologies.  I was planning to have a thorough look at this patch but
life got in the way and I have not been able to study what's happening
on this thread this close to the feature freeze.

Anyway, I am attaching two modules I have written for the sake of this
thread while beginning my lookup of the patch:
- lwlock_test.tar.gz, validation module for LWLocks with variable
waits.  This module can be loaded with shared_preload_libraries to
have two LWLocks and two variables in shmem, then have 2 backends play
ping-pong with each other's locks.  An isolation test may be possible,
though I have not thought hard about it.  Just use a SQL sequence like
that, for example, with N > 1 (see README):
    Backend 1: SELECT lwlock_test_acquire();
    Backend 2: SELECT lwlock_test_wait(N);
    Backend 1: SELECT lwlock_test_update(N);
    Backend 1: SELECT lwlock_test_release();
- custom_wal.tar.gz, thin wrapper for LogLogicalMessage() able to
generate N records of size M bytes in a single SQL call.  This can be
used to generate records of various sizes for benchmarking, limiting
the overhead of individual calls to pg_logical_emit_message_bytea().
I have begun gathering numbers with WAL records of various size and
length, using pgbench like:
$ cat script.sql
\set record_size 1
\set record_number 5000
SELECT custom_wal(:record_size, :record_number);
$ pgbench -n -c 500 -t 100 -f script.sql
So this limits most the overhead of behind parsing, planning, and most
of the INSERT logic.

I have been trying to get some reproducible numbers, but I think that
I am going to need a bigger maching than what I have been using for
the last few days, up to 400 connections.  It is worth noting that
00d1e02b may influence a bit the results, so we may want to have more
numbers with that in place particularly with INSERTs, and one of the
tests used upthread uses single row INSERTs.

Another question I had: would it be worth having some tests with
pg_wal/ mounted to a tmpfs so as I/O would not be a bottleneck?  It
should be instructive to get more measurement with a fixed number of
transactions and a rather high amount of concurrent connections (1k at
least?), where the contention would be on the variable waits.  My
first impression is that records should not be too small if you want
to see more the effects of this patch, either.

Looking at the patch..  LWLockConflictsWithVar() and
LWLockReleaseClearVar() are the trivial bits.  These are OK.

+    * NB: Note the use of pg_atomic_exchange_u64 as opposed to just
+    * pg_atomic_write_u64 to update the value. Since pg_atomic_exchange_u6=
4 is
+    * a full barrier, we're guaranteed that the subsequent shared memory
+    * reads/writes, if any, happen after we reset the lock variable.

This mentions that the subsequent read/write operations are safe, so
this refers to anything happening after the variable is reset.  As
a full barrier, should be also mention that this is also ordered with
respect to anything that the caller did before clearing the variable?
=46rom this perspective using pg_atomic_exchange_u64() makes sense to me
in LWLockReleaseClearVar().

+            * XXX: Use of a spinlock at the beginning of this function to =
read
+            * current insert position implies memory ordering. That means =
that
+            * the immediate loads and stores to shared memory (for instanc=
e,
+            * in LWLockUpdateVar called via LWLockWaitForVar) don't need an
+            * explicit memory barrier as far as the current usage is
+            * concerned. But that might not be safe in general.
             */
What's the part where this is not safe?  Based on what I see, this
code path is safe because of the previous spinlock.  This is the same
comment as at the beginning of LWLockConflictsWithVar().  Is that
something that we ought to document at the top of LWLockWaitForVar()
as well?  We have one caller of this function currently, but there may
be more in the future.

- * you're about to write out.
+ * you're about to write out. Using an atomic variable for insertingAt avo=
ids
+ * taking any explicit lock for reads and writes.

Hmm.  Not sure that we need to comment at all.

-LWLockUpdateVar(LWLock *lock, uint64 *valptr, uint64 val)
+LWLockUpdateVar(LWLock *lock, pg_atomic_uint64 *valptr, uint64 val)
[...]
    Assert(pg_atomic_read_u32(&lock->state) & LW_VAL_EXCLUSIVE);
=20
-   /* Update the lock's value */
-   *valptr =3D val;

The sensitive change is in LWLockUpdateVar().  I am not completely
sure to understand this removal, though.  Does that influence the case
where there are waiters?

Another thing I was wondering about: how much does the fast-path used
in LWLockUpdateVar() influence the performance numbers?  Am I right to
guess that it counts for most of the gain seen?  Or could it be that
the removal of the spin lock in
LWLockConflictsWithVar()/LWLockWaitForVar() the point that has the
highest effect?
--
Michael

--DKfGcTxj0xWBOD6a
Content-Type: application/gzip
Content-Disposition: attachment; filename="lwlock_test.tar.gz"
Content-Transfer-Encoding: base64

H4sIAAAAAAAAA+0ba3PbNrJfxV+BOm0tObIefk6dOjOqLOs0J8s+SU4yc7nhwCQksaYIliCt
+m7y328XACmSkiy3TtJ2hvuhkYHFYnexT4CtTZ3QmXo8YN98MWgAnJwcyX8B8v82GscH3zSP
jppHJ6cHJ6c43jxsHJ980/hyLC0hEiENYMuA8/ApvG3zeeH+JvCK9OTpk5CJkPg0nAmjHjAR
uaGoG/Vw7pvWjFn3dePP5rSALwFX9J5NHPcLev92/z9snOT9/+D4sFH4/1eAq+uL235nRM6J
u3C5dW9iHDCMzodxZzDqXQ9yExetcSs7tL/frDVq4lfXuOle9vqdi86oDRg7KRSyL6OLIBMe
kP77PoyLHQPwzfb14LLXBXR/alrcmzhTGP0wImfn5LuymDHXhX8TvArZ3/env4mK4XiWG9lM
Tn4YVYrY9Idh2GldXHW+7B5b/P/gtHmU9//m6fFx4f9fAdLOfb4EwxjPHEHm3I5cRuAXJTdc
hNOAjf7VJ+y3kHnC4R4JZzTUzh3wKHQ8Bj+YS0Nmk5Ab2tlrSI6tYuhpQj2bPNDAoXew2QN1
I8CxZtSbOt6U0ICdGQYhexr91rdh8TsalCup0SFzGRWsDf8N8nPvqRNecj2MnAiGVMmc+r7k
E8RgEJxcly9wRxRxEnlWCBKKqpJRzHjk2uSOkUjAGu4Z4YITwXwaADfkjlr3zLMFEpvRB0YY
tWbxKIHQFlIrVIMcNgukyL5LHw0Ks/M5qHJK54zwCfGBhX2fAx+RQG5wn0Q5kRTelstxQmsQ
AiJwCDLZZM7mPHjUGkudrkmtXyMnYOVKFZlcCpJw2ZQTGk2pxAkgeiOJ2gq5QCl8GzmNtpWc
EmwbNYUliSUq4RMgliVflfpZwLETNE+YE6jc0JkzmYNy6oTEFPA54gElweC07A1cIsnNPB6s
KDBFDJYBOckTsoCz8e5w6Ov4R7uIPMeK7atGSJ9zNA8SeaHjarFdGBPgkj6zpGXA1hTyJmws
ojmzYZV05YCFUeAJucSL5ndgg7CvZgFIBcxizgOzq2Qxc8BMlwaf6I9uWE1s7rFa7FmLjKNQ
oR1LUWbA3YC8JU1poIT8HB/vGRl1+p32eL3JvskgH6xFlmczyKGup6uN7XnIiaG/0UFxGf08
xpTL3+ExUBvkTRzR9GEhjJmucxeAvTGh3GHqiJAFBioyDn9SUY6njEDHHWLBIQqppY30zslu
itHdP1QFravkPneO2ZL/m4eHx/n8f3TcPC3y/1eAeibA1deZA9mrG8b+PsYjyFhgp86ECCtw
/BALA8GjwFL26wNylUA+xAQHWdMjDw4lbSgxxx2SdBTGR2bNOLmFULGTn0vzsoP+ghYPpGAf
bFJr5CMEhBC5QYbSLUaSr3FOk728HbTHOarLmGIMO+Pb4WBE3l33LozWiOyqXsi8aY3/MWhd
dXarGf+KV+4a/dage9vqdkibjMbDHsSLm9aw1e93+uR2MGpddiBSPMVCElF+Nwt65ctZ0BHQ
8cLfz4Ra+3IeZMDOcAB/PIcBXPis7f9s5/obQEqvNesL7fF0/G8eHzZX4v/h8elREf+/AtT3
9j8bGGTPyBbMNQsGSqWxbBDjcmfhhLNsj1XTK294IP8mbe4/Bs50FpKyVSHNH3882T9oHBxW
001o1+V31CUX7AHqIn/OvJB0ocP0nyZ0VCVDNgXkpOy+9aDsDYQTPuJIm7oO1OeeQzVPvYvO
YNy77LVbGMCkOBsSphQWl3xOhULqfRXfdu34SnpRm+2khn8SztSjbm32No07mU8DxFuOzB1h
UXvueNlhEfKATlnd8a31E0rE9XNiBv2mZOema+qwfdXq9toQ+6G0uPWwicGjgVw+ZSGZ41OD
ze6iqeztXT4VKCOgvrLZxPFkUXzd/qc57ozG5kXn59suaUot1KVq9aHrXhdcN2RSSeEjdPIM
6pIwiKDRdhfhSKKMJMb/jJIyvlJpT2Wv4E2pVI/vB1QBfveoO7C4nQOyy2WYczas0k1eepk/
NWnI545lRpDTTo5KJb2tCa3nG6TxLt/Rr91+lY7iQ5HZRGeVoU85jajT6TvevexKMlcHSqu4
Cn84Vm4p2YO/ofUY3Pb7isyIQttIZpzfx3c3DrYvQt5mRJ4s4ZbUpMVAIfNrhKkcV5l4eATa
mwdzdTLZKbMcY3YY+euXpyfTjKL9dETozFGKnMg+tZhs56BhNlLcjpz/MgMENgFRwO/yA3fs
ChoUzpRKOAbE5b+w11XrQ6vf6w7K+DeflLOqq0ALWVJ9OFELP8V8ZQSXujwj8V/Uth2MZku7
hxAgq24Bzf17tgvdPjSLXF4U8ADJ0TDEqyZ9s5Vfheej41ZGY+VKRnYUVcqeYW6pAWgDyhtO
rWKUShumZB9dGqqRFojmjRBnhCdQTmtaakvjDeic2crtxgH1rBkrpx8YdqrkoLKiTS2V1mZK
Qyn1ZMzgKeljFSXS33HulkoTHoGHnZMJdYU0hJxS0raYU0p6SikFfAnOCKJkyn8oDiGmTJv6
kgDzwJziPYKMEhmHBCpGCXTQDtiKtCljSPt6lai4jhF5RgWgYkRBKkkEbOmuaXlgPc8J+/Ku
qv/e7Hxo929HvXcdKYfiJ8EayZicOzDQBQIh6z1lOf+DVHEl1u236k+jBEeAClMxU97shTNI
/9OZ5FqzfQO+A8LuubLwOCddttWU5E4owv5bHbNh3Q9lSeHfjf/U8AdapkJR4TiF0UxhLKO3
A2owo5Oj8g9pwhjFq6TxNO4y4GtUsPJS5s57zaGkneFK3eHLMIyXSpgWpKFLCzchb+OOWbf+
1g84BAphbrp7gjUwyLEYEWjXLGA+1FzlznB4PVSHV2ZBMBfT8o5FPY+HioGPO9+LjzuETlBr
2gPAe7MnoNYTWG+zkDpueUcvm0eYH5O7NllIbuKwlrKxHP2KPOPNCWd1ENS+FnMlOr4xNrl4
Qjc9mNDNYa7EndR5Zipn9Gr5fQaoZUYfHC7vZXWBkry3uEBVPqWEJGV6SEy+ssg8Iq82c68c
yeWtqCpl45ND/NRxhuu339vubb2u3UQlfUu7CSe+SlFIdVmGxpcNZm9weW2+a5bXsAbo2zA1
6WdgKkafgYgyo+1d0DCaG+s0lqbQGnZHiUfKmKqC/DP9DU4wdjcgoH9BeCc4eceY9yw3Srxo
Zw27K54rc3Y2Y6QD3kquKIG86vLHxNunsopbq+qJj/lvoh7N7lPqyb9XZtW0KU2sVVc933Vr
g9Td6Ah7AOrFb2fJy5h888LywncfkwcwotJNVb4qYcjAjlemV6Gqo9Wz0W66/mi8EBKlfKA6
J4DR7YxhDnxjfHhQlhJJDAtyOpYMDRjQjQ53bWgmsmMeWyRjf8VjV5pYe+pGCR/9yktBifr5
k3q+03++fr0sbKAkC5hsnVNmgN2dbIdfORNsd1eaZVQCNNXl963hoDfoZlnRDJ4pivIdcUka
C59XYCrOJKmeli/t26xTa2NZxEDxaa8veCrkNWmqIguk9PiCzOk9lKWRfDLVz7W6rMJzid8l
Uy+mqsLD819530+VTDFTJbJSSCUzyspAImVaFTQi1D+ypt5WIY0uMA0GDMpkwdFMZqg5SJZI
EH8+fRzPOQ+5TbIFhIQHoJs5kNIdaBTLBVn+Sdm1h3x7TjTzkvcXcqIVnChdxgWluSw/iX+q
zRVnkrlnxij1lK5j1Pv4WTx5mdeFCT46oY6T+KQtKS5YkIpQTQDSyb6sB/jtBng00I7JITZf
eMk+qUCnohxSWcwc5CB5fsdpVf6kGNgQDaVYm2Oh5NcMuWnzTRHxmQFQ0dMv8Ca+wP91AyNK
vSEsrikW4hS00leqcylDO8lknPxjQRDJn4FEU8fz8m62OabEFpULKmuC4AujCr6j6saqSn7B
hgfjAXtBlFEC417645LnxJiX6HYCDaWYyW9jVChPbfNk8FIqaiW1CprfDD/S8vDrEYGfT4AY
QjmiSqAyFayLRhnXeP36zZMibZdJcwQNqfzMpiozF2Sr720w7PRWaY1+MvK5fJmEXpjKU1zJ
8JTQ3Z7HY//alBY3ZvElaiqJ42lmotDbc5KKcpW0Ya1cXWTL342MyeI3nVlUuMyp/VPx3vv7
IfN+xb0w4J/985+t3/83T0/z77/HjcOD4v33K0D8Ppf9lozsk3H+i/1dA8IUjdzQlM+l3MM1
zVpj11BfCZv4Pw95+J0gjH8HFYHtBOnn0V0DiwUop2R9d04wjRceW0ABBRRQQAEFFFBAAQUU
UEABBRRQQAEFFFBAAQUU8Lnh/wPbrYQAUAAA

--DKfGcTxj0xWBOD6a
Content-Type: application/gzip
Content-Disposition: attachment; filename="custom_wal.tar.gz"
Content-Transfer-Encoding: base64

H4sIAAAAAAAAA+1YbW/aSBDuV/ZXjGglIOUdjqrN5YMFDkUCkgtOG+l6QhuzhlVtr7teJ7mr
+t87axtwSFTuA8ldKz+yZDOe2ZfxzjMzTOhn5nCXvXhCNBG9Xje+I3bvzU6r+6LV7ba6vTed
Zkvrtdq/tfH9Uy5qjShUVOKUUgj1I71973c395Ngcja4HJszOAEbPSG8+S11CTGvLHM6G51N
78sHhmXck9RqrXqzHn5xyfnwdDQ2B+asjwrFrQbUYMh8JqliqR18NMYgmS3kIiwSNJz3z6an
oyHaBcu5LXyHL1F6NYN3J/CqHK6Y6+J9o1eBWi1Y3oUVwn3bjRYsfnk1q5D/2pc/Iy5MYzAx
n3aOffHf7nV247/1ptvJ4/8ZkInukw0IsVY8BE8sIpcBPp2LUC0lm/0xBnanmB9y4VeBco8t
QAm4ZkBBrbgPt5IGAZMgfJQHRDgwFku8uE3dCQtDumTlijZZrkmBrmnBj7xrbelkCUL/pCTV
CPk/rE7yMD8kHuHyg8+xL/4x7nfjv9vp5vn/OdA4yqTzxiOnAY4ahNRqYAsvcCmGOHcgtCUP
lCaGUETSRhJAeYDKVcCYXmEUqxX14YZT6GOCsUzYFBTkE7NXAi5DBsXdd5mVFDVHuIIuNK2E
oCvUOnz6EnGVLGajWE6IYq65oZqyxjyhkgpJJzi9nPat++OXua+quGpVIRemdXkxncGN4Ati
zKCUlETzc8N6PzUmZomMjenw0hia0IeZdTHqW8e/DgdtXVK3n2qOPfHfbnfau/GP4d/O4/8Z
0DiqHQoEjvCC7InC34WCrhqcyLcVj6uCbep/2A/U0zH6Ivhb8uVKQdmuQOvt216t3Wx3qtlC
ZOiKa+wvBuyGuSLwmK9gKEUUpEOMBubUGp2O+oYO/Xgh8DjVxcvE65COQM58uW5PikGy6LC+
KmakjreUWpIRSRa4WClpRzW8pFqKNbD3SUlpYgxH/WNCGvEeP6bV1sa7SdkF/6LsSrYfD5Jx
P0Bc+EUhC0HdCgiopB5TTIbvkIhZpkZb12dK6DG21Zy/iBWT12nFpt2BW1jT8Hw0PT2bf2iV
t5+gckwGVEUeyTB01sK4GGJ/95UUkLELhQzlY9OIekPTQg0c1+q0y00c7J5euuiHmi2taa+o
BICj1N9rW1tEvkJHF1K57k6p6wq7WdbzCqesDSu498xq9HiOkHhotTWaNI8hefw9q5YKX7+u
kMJ6/D9j0V9oU6IlPS8m5rHAb6k9unHv49/glvoKczB6ed/siW12fvSpnsrCgR0uQwVULqM4
mPAcUAgkc/hdFT4zFgBX4HGfezQuCgqFh2esWKxCuqEq3MvMDnXD2D/fcG+BIxkrp4paiF8m
ScPzD2ejQRlF336dFPu/RpYGha+kOHz5v7//b7ce1P+9TjfP/8+Al5mcuO3tCVb7MQkgHf3g
77sSWTCHRq6a32CK0AkI9bFvKJHkr4N5gN2AjxlEy1+5/HrBZSbxlojE1I35jl67WkXJiOVh
nyNHjhw5cuTIkSNHjhw5cuTIcVh8B+QpkM8AKAAA

--DKfGcTxj0xWBOD6a--

--a3LqHsA1XvzfEZZ9
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmQzi8sACgkQnvQgOdby
QH2RIhAAjbqWHOZiE0JzlgvEO4bRCbyYU7QjYsEgo/eUWM7B6jFQrhzy6WDUtOuo
7HAfTMNO/J+eBNaWDPK0CDXq6ByPt1xaLZyDY03f16DkHJprVbzWOzh9xzaTl5cZ
L2sjkMNaTwwftuHDHqxw+pBcF5cn7nCCTUMseMaFN6znAQvrhS+xaBQQ2Kp2m3ys
ZKdfEp1y42KKiNOHSHZzk8Mzxd49+mdwF5UOZznfj0EKPpCt0WkkMENxw4EM6qsT
BKZ4lWFdwow/NKwsIQOQ5om57EhEPx+zopdHI7rL2bZSFNyxv4pGSruE12GXrIHr
VxQi/AfgKZ9gr0b6vi8xgEjJYE2ucUd35m7THg+eBI3U0Jj1FkaEiZtD+VzA5+pG
Ltuqu4IvZCMPdRN7KTgXsaoBRC8xgczAM8TMkvru4VEY+Et4cili90/i0mZYjGlw
6/kuST44Oi/7MsdbeUgnk1/YsFwOC3xri9Gp3YHkafxQnwokAifa9ma37xFTVAIr
CnzP4/PtJukiMQYYzeQnf2ZbT0oGUENVt6afj8iRjW9/uikf8NlzhAAoBjmaQ5s1
6f6rxf1NCYK0AUV5Ft3TO/TiKCDvBhqA2eIl/3UjmXfbWDipWL9/SQ2xVLMgLB/I
OCQNW6eKSG2rJ5itIx2eEHNsfj0urvJmMt7yrCGmVDNflC7ulnc=
=dIOy
-----END PGP SIGNATURE-----

--a3LqHsA1XvzfEZZ9--