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: <58821140-0182-4FF3-9A4F-5B168DDB9243@gmail.com>
Date: Thu, 15 Jan 2026 14:22:09 +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: <732580B4-962F-4EBD-8811-F4667D79747D@gmail.com>
References: <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>
 <F6A52B41-2FFA-4592-BD9B-0438F610B6A2@gmail.com>
 <cj5mcjdpucvw4a54hehslr3ctukavrbnxltvuzzhqnimvpju5e@cy3g3mnsefwz>
 <58821140-0182-4FF3-9A4F-5B168DDB9243@gmail.com>
To: Andres Freund <andres@anarazel.de>
Archived-At: <https://www.postgresql.org/message-id/732580B4-962F-4EBD-8811-F4667D79747D%40gmail.com>
Precedence: bulk


> On Jan 15, 2026, at 08:04, Chao Li <li.evan.chao@gmail.com> wrote:
>=20
>=20
>=20
>> On Jan 15, 2026, at 07:37, Andres Freund <andres@anarazel.de> wrote:
>>=20
>> Hi,
>>=20
>> On 2026-01-15 07:20:27 +0800, Chao Li wrote:
>>>> On Jan 15, 2026, at 00:30, Andres Freund <andres@anarazel.de> =
wrote:
>>>> 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?
>>>=20
>>> 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.
>>=20
>> A significant portion !=3D all. As the above excerpt from the comment =
shows, the
>> locking uses 20 bits. We could increase max backends by 5 bits =
without running
>> out of bits (we'd need space both in the refcount bitspace as well as =
the
>> share-lock bitspace).
>=20
> Make sense. I think I misread the comment.
>=20
>>=20
>>=20
>>> 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");
>>> ```
>>=20
>> You can see that being relaxed in the subsequent commit, when we =
start to use
>> more bits.
>>=20
>=20
> Sure. I plan to review 0003-0005 today. I believe I will get better =
understanding.

I have finished reviewing 0003-0005. Basically, 0003 and 0004 just =
delete some unused functions. I only searched over the source tree to =
ensure no usages of them. I spent time on 0005. The code logic are solid =
already, I didn't find any correctness issue and only got some small =
comments:

1 - 0004 - commit message
```
subsequent commits fixing a typo an a parameter name.
```

Typo: a typo an a parameter name =3D> a typo in a parameter name

2 - 0005 - bufmgr.c
```
-	 * Pin it, share-lock it, write it.  (FlushBuffer will do =
nothing if the
-	 * buffer is clean by the time we've locked it.)
+	 * Pin it, share-exclusive-lock it, write it.  (FlushBuffer will =
do
+	 * nothing if the buffer is clean by the time we've locked it.)
 	 */
 	PinBuffer_Locked(bufHdr);
```

I think we don=E2=80=99t need to mention lock type in this comment, =
because the logic belongs to FlushUnlockedBuffer(). Also, FlushBuffer is =
misleading here, because we call FlushUnlockedBuffer() here, and =
FlushUnlockedBuffer() in turn calls FlushBuffer().

So, I think we can simplify the comment as something like =E2=80=9CPin =
it and flush the buffer"

3 - 0005 - bufmgr.c
```
+inline void
+MarkBufferDirtyHint(Buffer buffer, bool buffer_std)
```

It=E2=80=99s quite uncommon to extern an inline function. I think =
usually if we want to make an inline function accessible from external, =
we define it =E2=80=9Cstatic inline=E2=80=9D in a header file. So, I =
guess =E2=80=9Cinline=E2=80=9D is a typo here.

4 - 0005 - bufmgr.c
```
/*
* MarkBufferDirtyHint
*
* Mark a buffer dirty for non-critical changes.
*
* This is essentially the same as MarkBufferDirty, except:
*
* 1. The caller does not write WAL; so if checksums are enabled, we may =
need
* to write an XLOG_FPI_FOR_HINT WAL record to protect against torn =
pages.
* 2. The caller might have only a share-exclusive-lock instead of an
* exclusive-lock on the buffer's content lock.
```

For point 2, do we need to mention =E2=80=9CFor shared buffers=E2=80=9D. =
Because this function also handles local buffer that doesn=E2=80=99t =
require a lock.

5 - 0005 - bufmgr.c
```
+/*
+ * Helper for BufferBeginSetHintBits() and BufferSetHintBits16().

+static inline bool
+SharedBufferBeginSetHintBits(Buffer buffer, BufferDesc *buf_hdr, uint64 =
*lockstate)
```

In the previous function comment:
```
- * MarkBufferDirtyHint
+ * Shared-buffer only helper for MarkBufferDirtyHint() and
+ * BufferSetHintBits16().
```

It mentions =E2=80=9CShared-buffer only helper=E2=80=9D. I think =
SharedBufferBeginSetHintBits is also only for shared buffer, maybe also =
add =E2=80=9CShared-buffer only=E2=80=9D before =E2=80=9Chelper=E2=80=9D =
in the comment.

6 - 0005 - bufmgr.c
```
+	}
+
+}
```

Nit: In function SharedBufferBeginSetHintBits, the last empty line is =
not needed.

7 - 0005 - bufmgr.c
```
+ * This checks if the current lock mode already suffices to allow hint =
bits
+ * being set and, if not, whether the current lock can be upgraded.
+ */
+static inline bool
+SharedBufferBeginSetHintBits(Buffer buffer, BufferDesc *buf_hdr, uint64 =
*lockstate)
```

Nit: "if not, whether the current lock can be upgraded=E2=80=9D might be =
read as =E2=80=9Clock can be upgraded, so the caller still need to take =
some action to upgrade the lock=E2=80=9D, but the function has upgraded =
the lock when returning true. So, how about explicitly state something =
like: "if not, it attempts to atomically upgrade it to share-exclusive. =
Returns true if hint bits may be set (with or without an upgrade), false =
otherwise."

8 - 0005 - fsmpage.c
```
 * needs to be updated. exclusive_lock_held should be set to true if the
* caller is already holding an exclusive lock, to avoid extra work.
```

The function comment of fsm_search_avail() needs to be updated. =
exclusive_lock_held should be set to true if the caller is already =
holding a **share-exclusive or** exclusive lock.

Maybe the parameter name =E2=80=9Cexclusive_lock_held=E2=80=9D can be =
enhanced as well.

9 - 0005 - fsmpage.c
```
+		if (!exclusive_lock_held)
+			BufferFinishSetHintBits(buf, false, true);
```

Nit: just curious why set the third parameter as =E2=80=9Ctrue=E2=80=9D? =
When the second (mark_dirty) is false, the third parameter is not used =
at all.

10 - 0005 - freespace.c
```
-	 * Reset the next slot pointer. This encourages the use of =
low-numbered
-	 * pages, increasing the chances that a later vacuum can =
truncate the
-	 * relation. We don't bother with marking the page dirty if it =
wasn't
-	 * already, since this is just a hint.
+	 * Try to reset the next slot pointer. This encourages the use =
of
+	 * low-numbered pages, increasing the chances that a later =
vacuum can
+	 * truncate the relation. We don't bother with marking the page =
dirty if
+	 * it wasn't already, since this is just a hint.
 	 */
 	LockBuffer(buf, BUFFER_LOCK_SHARE);
-	((FSMPage) PageGetContents(page))->fp_next_slot =3D 0;
+	if (BufferBeginSetHintBits(buf))
+	{
+		((FSMPage) PageGetContents(page))->fp_next_slot =3D 0;
+		BufferFinishSetHintBits(buf, false, false);
+	}
```

Before this patch, we unconditionally set fp_next_slot, now the setting =
might be skipped. You have add =E2=80=9CTry to=E2=80=9D in the comment =
that has explained the possibility of skipping setting fp_next_slot. =
Would it be better to add a brief statement for what will result in when =
skipping setting fp_next_slot? Something like =E2=80=9CSkipping the =
update only affects reuse, not correctness=E2=80=9D.

Lately, I saw you have done this in nbtinsert.c:
```
* mark the index entry killed. It's ok if we're not
* allowed to, this isn't required for correctness.
```
which, I think, confirmed my comment.

11 - 0005 Maybe not a problem. In nbtree.h:
```
/*
* We need to be able to tell the difference between read and write
* requests for pages, in order to do locking correctly.
*/
#define BT_READ BUFFER_LOCK_SHARE
#define BT_WRITE BUFFER_LOCK_EXCLUSIVE
```

Can the new lock type BUFFER_LOCK_SHARE_EXCLUSIVE be used by nbt? Maybe =
implicitly upgrading from BT_READ to BUFFER_LOCK_SHARE_EXCLUSIVE is good =
enough?

12 - 0005 - heapam_visibility.c

After this commit, tuple hint bits may remain unset if we can=E2=80=99t =
obtain share-exclusive permission. That=E2=80=99s fine because hint bits =
are advisory and optional, but this is a behavior change. Would it make =
sense to mention this explicitly and briefly in the SetHintBitsExt() =
comment?

13 - 0005 - nbtutils.c
```
+				/*
+				 * If we're not able to set hint bits, =
there's no point
+				 * continuing.
+				 */
+				if (!killedsomething &&
+					!BufferBeginSetHintBits(buf))
+					goto unlock_page;
```
I like this code, because it ensures to only call BufferBeginSetHintBits =
once. But lately, I saw the same logic in gistget.c:
```
+		if (!killedsomething)
+		{
+			/*
+			 * Use hint bit infrastructure to be allowed to =
modify the page
+			 * without holding an exclusive lock.
+			 */
+			if (!BufferBeginSetHintBits(buffer))
+				goto unlock;
+		}
```
I just feel the gist version is easier to read, so maybe change the nbt =
one to be the same as the gist one. But I think the nbt one=E2=80=99s =
comment is better.

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