From: Chao Li <li.evan.chao@gmail.com>
Message-Id: <AC5E365D-7AD9-47AE-B2C6-25756712B188@gmail.com>
Content-Type: multipart/mixed;
	boundary="Apple-Mail=_FFEB6AA3-0D2D-4781-BC67-A5F276E1E243"
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3826.700.81.1.4\))
Subject: Re: Buffer locking is special (hints, checksums, AIO writes)
Date: Wed, 14 Jan 2026 10:26:07 +0800
In-Reply-To: <jtg5cu4n6h5lib3kzx66ju4yhh6kmviaud7oq6dtut6c4q4rdi@xwsfoagt3c2b>
Cc: Kirill Reshke <reshkekirill@gmail.com>,
 Heikki Linnakangas <hlinnaka@iki.fi>,
 Melanie Plageman <melanieplageman@gmail.com>,
 Matthias van de Meent <boekewurm+postgres@gmail.com>,
 pgsql-hackers@postgresql.org,
 Thomas Munro <thomas.munro@gmail.com>,
 Noah Misch <noah@leadboat.com>,
 Robert Haas <robertmhaas@gmail.com>,
 Michael Paquier <michael.paquier@gmail.com>
To: Andres Freund <andres@anarazel.de>
References: <lneuyxqxamqoayd2ntau3lqjblzdckw6tjgeu4574ezwh4tzlg@noioxkquezdw>
 <c00d4a9f-c0f8-49ca-bf27-29c6641f2fc6@iki.fi>
 <q3ybyaiownp4ivt6xfy55s5llvtq6vqsd4zbt5kn6hacxdcpp4@2ngd4h4bev5d>
 <dsbacri4xldlobl3jbqqlksefsyrwj7lzljbohxsbqjus3z4o7@otb6rge4w7az>
 <1108f18d-cf7c-4f17-b29c-a119fe42f7e5@iki.fi>
 <5dwlfu2jyzkyf3nrlzxxblxctb6xio5es73ptgsahjnmfu5miu@772rc764hfhi>
 <ossv2eistssmubfsir6xjll76tynvxv5lup4zkrfzjkud7fycw@rf5vii6l6cha>
 <4csodkvvfbfloxxjlkgsnl2lgfv2mtzdl7phqzd4jxjadxm4o5@usw7feyb5bzf>
 <CALdSSPgyc7VuMLUZ8J7v2G-PebBa_vEi+mp-cLkcOwNycB56Hw@mail.gmail.com>
 <kjdgvws34gpnz4y6xu5aaeul5mspgy3ahvyiw4lndb3ecsacdb@b2ccyowotpqh>
 <jtg5cu4n6h5lib3kzx66ju4yhh6kmviaud7oq6dtut6c4q4rdi@xwsfoagt3c2b>
Archived-At: <https://www.postgresql.org/message-id/AC5E365D-7AD9-47AE-B2C6-25756712B188%40gmail.com>
Precedence: bulk


--Apple-Mail=_FFEB6AA3-0D2D-4781-BC67-A5F276E1E243
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain;
	charset=utf-8


> On Jan 13, 2026, at 08:33, Andres Freund <andres@anarazel.de> wrote:
>=20
> Hi,
>=20
> On 2026-01-12 12:45:03 -0500, Andres Freund wrote:
>> I'm doing another pass through 0003 and will push that if I don't =
find
>> anything significant.
>=20
> Done, after adjust two comments in minor ways.
>=20
>=20
>> Also working on doing comment polishing of the later patches, found a =
few
>> things, but not quite enough to be worth reposting yet.
>=20
> Here are the remaining commits, with a bit of polish:
>=20
> - fixed references to old names in some places (lwlocks, release_ok)
>=20
> - Aded an assert that we don't already hold a lock in =
BufferLockConditional()
>=20
> - typo and grammar fixes
>=20
> - updated the commit message of the LW_FLAG_RELEASE_OK, as "requested" =
by
>  Melanie. I hope this explains the situation better.
>=20
> - added a commit that renames ResOwnerReleaseBufferPin to
>  ResOwnerReleaseBuffer (et al), as it now also releases content locks =
if held
>=20
>  I kept this separate as I'm not yet sure about the new name, =
partially due
>  to there also being a "buffer io" resowner.  I tried "buffer =
ownership" for
>  the resowner that tracks pins and locks, but that was long and not =
clearly
>  better.
>=20
> Greetings,
>=20
> Andres Freund
> =
<v10-0001-lwlock-Invert-meaning-of-LW_FLAG_RELEASE_OK.patch><v10-0002-bufm=
gr-Make-definitions-related-to-buffer-descri.patch><v10-0003-bufmgr-Change=
-BufferDesc.state-to-be-a-64-bit-at.patch><v10-0004-bufmgr-Implement-buffe=
r-content-locks-independen.patch><v10-0005-Require-share-exclusive-lock-to=
-set-hint-bits-an.patch><v10-0006-WIP-Make-UnlockReleaseBuffer-more-effici=
ent.patch><v10-0007-WIP-bufmgr-Don-t-copy-pages-while-writing-out.patch><v=
10-0008-WIP-bufmgr-Rename-ResOwnerReleaseBufferPin.patch>

Hi Andres,

So far I=E2=80=99ve only reviewed 0001 and 0002. I=E2=80=99m not very =
familiar with this area, so the review has been a bit slow.

Overall, 0001 looks good to me. It renames LW_FLAG_RELEASE_OK to =
LW_FLAG_WAKE_IN_PROGRESS and inverts the meaning, which makes sense. I =
only have a small nit on naming: the local variable =
=E2=80=9Cnew_release_in_progress". I see that it=E2=80=99s inherited =
from the old name and was updated from =E2=80=9C_ok" to =
=E2=80=9C_in_progress", but now that the flag itself is renamed, would =
it make sense to rename the variable as well? Something like =
=E2=80=9Cwake_in_progress" or =E2=80=9Cnew_wake_in_progress" might =
better reflect the new flag name.

In 0002, a bunch of new macros are introduced. My initial impression =
wasn=E2=80=99t great, mostly due to the amount of line wrapping. Looking =
a bit closer, I also noticed some duplication, for example, =
"BUF_REFCOUNT_BITS + BUF_USAGECOUNT_BITS" appears more than once; and a =
small inconsistency between BUF_STATE_GET_REFCOUNT and =
BUF_STATE_GET_USAGECOUNT (even though the former doesn=E2=80=99t =
actually need a shift).

I tried a small refactor of the macro definitions in the attached diff =
to see if things could be made a bit more regular. It introduces a =
helper macro MASK() and a BUF_REFCOUNT_SHIFT constant, and removes a bit =
of duplication. If you like it, feel free to take it; otherwise, please =
just ignore it. Note that, the diff is based on 0002.

(I actually hesitated to attach a diff, because if you=E2=80=99ve =
already created a CF entry, the attached diff could break the CI tests. =
If that happens, sorry about that.)

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/


--Apple-Mail=_FFEB6AA3-0D2D-4781-BC67-A5F276E1E243
Content-Disposition: attachment;
	filename=buf_internals_h.diff
Content-Type: application/octet-stream;
	x-unix-mode=0644;
	name="buf_internals_h.diff"
Content-Transfer-Encoding: 7bit

diff --git a/src/include/storage/buf_internals.h b/src/include/storage/buf_internals.h
index 2f607ea2ac5..34e6c6cd54f 100644
--- a/src/include/storage/buf_internals.h
+++ b/src/include/storage/buf_internals.h
@@ -49,28 +49,26 @@
 StaticAssertDecl(BUF_REFCOUNT_BITS + BUF_USAGECOUNT_BITS + BUF_FLAG_BITS == 32,
 				 "parts of buffer state space need to equal 32");
 
-/* refcount related definitions */
-#define BUF_REFCOUNT_ONE 1
-#define BUF_REFCOUNT_MASK \
-	((1U << BUF_REFCOUNT_BITS) - 1)
+#define BUF_REFCOUNT_SHIFT       0
+#define BUF_USAGECOUNT_SHIFT     (BUF_REFCOUNT_SHIFT + BUF_REFCOUNT_BITS)
+#define BUF_FLAG_SHIFT           (BUF_USAGECOUNT_SHIFT + BUF_USAGECOUNT_BITS)
+
+/* mask generator */
+#define MASK(bits) ((1U << (bits)) - 1)
 
+/* refcount related definitions */
+#define BUF_REFCOUNT_ONE         1U
+#define BUF_REFCOUNT_MASK        (MASK(BUF_REFCOUNT_BITS) << BUF_REFCOUNT_SHIFT)
 /* usage count related definitions */
-#define BUF_USAGECOUNT_SHIFT \
-	BUF_REFCOUNT_BITS
-#define BUF_USAGECOUNT_MASK \
-	(((1U << BUF_USAGECOUNT_BITS) - 1) << (BUF_USAGECOUNT_SHIFT))
-#define BUF_USAGECOUNT_ONE \
-	(1U << BUF_REFCOUNT_BITS)
+#define BUF_USAGECOUNT_ONE       (1U << BUF_USAGECOUNT_SHIFT)
+#define BUF_USAGECOUNT_MASK      (MASK(BUF_USAGECOUNT_BITS) << BUF_USAGECOUNT_SHIFT)
 
 /* flags related definitions */
-#define BUF_FLAG_SHIFT \
-	(BUF_REFCOUNT_BITS + BUF_USAGECOUNT_BITS)
-#define BUF_FLAG_MASK \
-	(((1U << BUF_FLAG_BITS) - 1) << BUF_FLAG_SHIFT)
+#define BUF_FLAG_MASK            (MASK(BUF_FLAG_BITS) << BUF_FLAG_SHIFT)
 
 /* Get refcount and usagecount from buffer state */
 #define BUF_STATE_GET_REFCOUNT(state) \
-	((state) & BUF_REFCOUNT_MASK)
+	(((state) & BUF_REFCOUNT_MASK) >> BUF_REFCOUNT_SHIFT)
 #define BUF_STATE_GET_USAGECOUNT(state) \
 	(((state) & BUF_USAGECOUNT_MASK) >> BUF_USAGECOUNT_SHIFT)
 
@@ -81,8 +79,7 @@ StaticAssertDecl(BUF_REFCOUNT_BITS + BUF_USAGECOUNT_BITS + BUF_FLAG_BITS == 32,
  * entry associated with the buffer's tag.
  */
 
-#define BUF_DEFINE_FLAG(flagno)	\
-	(1U << (BUF_REFCOUNT_BITS + BUF_USAGECOUNT_BITS + (flagno)))
+#define BUF_DEFINE_FLAG(flagno)	(1U << (BUF_FLAG_SHIFT + (flagno)))
 
 /* buffer header is locked */
 #define BM_LOCKED					BUF_DEFINE_FLAG( 0)

--Apple-Mail=_FFEB6AA3-0D2D-4781-BC67-A5F276E1E243--