Content-Type: text/plain;
	charset=utf-8
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)
From: Chao Li <li.evan.chao@gmail.com>
In-Reply-To: <vrnib3bf726ht72jlccu252qm65lzexutydtagytstsjszpiya@ece6xi7o4nfx>
Date: Thu, 15 Jan 2026 07:20:27 +0800
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>
Content-Transfer-Encoding: quoted-printable
Message-Id: <F6A52B41-2FFA-4592-BD9B-0438F610B6A2@gmail.com>
References: <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>
 <9C2494E1-F446-42DF-B070-7E231B8E6221@gmail.com>
 <vrnib3bf726ht72jlccu252qm65lzexutydtagytstsjszpiya@ece6xi7o4nfx>
To: Andres Freund <andres@anarazel.de>
Archived-At: <https://www.postgresql.org/message-id/F6A52B41-2FFA-4592-BD9B-0438F610B6A2%40gmail.com>
Precedence: bulk


> On Jan 15, 2026, at 00:30, Andres Freund <andres@anarazel.de> wrote:
>=20
> Hi,
>=20
> On 2026-01-14 11:41:19 +0800, Chao Li wrote:
>> Basically, code changes in 0003 is straightforward, just a couple of =
small comments:
>>=20
>> 1
>> ```
>> - * refcounts in buf_internals.h.  This limitation could be lifted by =
using a
>> - * 64bit state; but it's unlikely to be worthwhile as 2^18-1 =
backends exceed
>> - * currently realistic configurations. Even if that limitation were =
removed,
>> - * we still could not a) exceed 2^23-1 because inval.c stores the =
ProcNumber
>> - * as a 3-byte signed integer, b) INT_MAX/4 because some places =
compute
>> - * 4*MaxBackends without any overflow check.  We check that the =
configured
>> - * number of backends does not exceed MAX_BACKENDS in =
InitializeMaxBackends().
>> + * refcounts in buf_internals.h.  This limitation could be lifted, =
but it's
>> ```
>>=20
>> Before this patch, there was room for lifting the limitation. With =
this
>> patch, state is 64bit already, but the significant 32bit will be used =
for
>> buffer locking as stated in buf_internals.h, in other words, there is =
no
>> room for lifting the limitation now. If that=E2=80=99s true, then I =
think we can
>> remove the statements about lifting limitation.
>=20
> I'm not following - there's plenty space for more bits if we need =
that:
>=20
> * State of the buffer itself (in order):
> * - 18 bits refcount
> * - 4 bits usage count
> * - 12 bits of flags
> * - 18 bits share-lock count
> * - 1 bit share-exclusive locked
> * - 1 bit exclusive locked
>=20
> That's 54 bits in total. Which part is in the lower and which in the =
upper
> 32bit isn't relevant for anything afaict?

Because I saw the comment in buf_internals.h:
```
 * NB: A future commit will use a significant portion of the remaining =
bits to
* implement buffer locking as part of the state variable.
```
That seems to indicate all the significant 32 bits will be used for =
buffer locking. Also, there is an assert that concretes the impression:
```
StaticAssertDecl(BUF_REFCOUNT_BITS + BUF_USAGECOUNT_BITS + BUF_FLAG_BITS =
=3D=3D 32,
       "parts of buffer state space need to equal 32");
```

So, I thought we can explain 18bit refcount is good enough without =
mentioning =E2=80=9Clifting=E2=80=9D that potentially adds confusion to =
readers. But anyway, this is not a strong opinion. I won=E2=80=99t =
insist on this comment.

>=20
>=20
>> 2. By searching for =E2=80=9CLockBufHdr=E2=80=9D, I found one place =
missed to update in contrib/pg_prewarm/autoprewarm.c at line 706:
>> ```
>> for (num_blocks =3D 0, i =3D 0; i < NBuffers; i++)
>> {
>> uint32 buf_state; <=3D=3D=3D line 706, should change to uint64
>>=20
>> CHECK_FOR_INTERRUPTS();
>>=20
>> bufHdr =3D GetBufferDescriptor(i);
>>=20
>> /* Lock each buffer header before inspecting. */
>> buf_state =3D LockBufHdr(bufHdr);
>> ```
>=20
> Good catch!  I didn't find any other similar omissions...

I saw you have added this occurrence to v11.

>=20
>=20
>> I will continue reviewing 0004 tomorrow.
>=20
> Cool.
>=20
> I'd like to push
>=20
>  bufmgr: Change BufferDesc.state to be a 64-bit atomic
>  bufmgr: Implement buffer content locks independently of lwlocks
>=20
> pretty soon, so that we then can concentrate on

Other than the =E2=80=9Clifting=E2=80=9D comment, v11 LGTM. But that=E2=80=
=99s not a strong opinion. I explained more above, if you consider =
that=E2=80=99s not a problem, I am totally fine.

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