Content-Type: text/plain;
	charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3864.400.21\))
Subject: Re: Checkpointer write combining
From: Chao Li <li.evan.chao@gmail.com>
In-Reply-To: <377BC880-1616-4DEF-B9EF-5E297C358F7D@gmail.com>
Date: Thu, 26 Feb 2026 12:23:45 +0800
Cc: Nazir Bilal Yavuz <byavuz81@gmail.com>,
 PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>,
 Andres Freund <andres@anarazel.de>
Content-Transfer-Encoding: quoted-printable
Message-Id: <BF806DCB-D361-4626-9A68-95F50FD18CC8@gmail.com>
References: 
 <CAAKRu_bcWRvRwZUop_d9vzF9nHAiT+-uPzkJ=S3ShZ1GqeAYOw@mail.gmail.com>
 <CAAKRu_bQWv5hJH0ncbGw8BEufAxdwMw4KY0KYuYLaw6KA=fpCA@mail.gmail.com>
 <CAN55FZ3FOv+ubp4yyFKt=Rr+ceM0tfXRDVr6nKgqSyWXh8G8Gg@mail.gmail.com>
 <CAAKRu_Yurj0sabXZB5EVqO3fKgwN1vELe1H4e0s1zXV3MKbZjQ@mail.gmail.com>
 <CAAKRu_bv2=_DU-tYFdTQJj7ohKUyFnrJvETyCes3_NpFzvVt4Q@mail.gmail.com>
 <CAAKRu_bz=t1FGXhUytPx9SC648-ozb-UxBps4zWUmS2XMg4Z4A@mail.gmail.com>
 <2FA0BAC7-5413-4ABD-94CA-4398FE77750D@gmail.com>
 <CAAKRu_atZuU3gYY6hVVuvqDW+mfXM+fCWCyYODbZPesxyr=y6g@mail.gmail.com>
 <3FC2442D-1012-4079-A009-A0B8C45E092D@gmail.com>
 <CAAKRu_b3k+KbeFhFgDZ26PvJbvL9W5HYwKrn8bBS4CTU13khpw@mail.gmail.com>
 <6B520FA8-B805-4C0D-B8B9-202B3AF168A9@gmail.com>
 <CAAKRu_Z8GtkTPsbScp8QoNF=V1YT3cVT6mPf1sR-NAtSE+jzoQ@mail.gmail.com>
 <0198DBB9-4A76-49E4-87F8-43D46DD0FD76@gmail.com>
 <CAAKRu_ZfneK9hEQs-ht2696aJ3ZSJs1c_-rXzBmyKjPDYRnqgA@mail.gmail.com>
 <4ECED07F-75F2-46D4-8F71-0B0DFA350777@gmail.com>
 <BDACEACB-8066-4003-8B4F-0C8AA6D366E2@gmail.com>
 <CAAKRu_ZBzTp-o4pu1UwmpLWkFmAmP7dyGFo867HxMo-AF+=MDw@mail.gmail.com>
 <20E76846-4816-45BC-84F0-D8836D9AFA4C@gmail.com>
 <CAAKRu_ZiEpE_EHww3S3-E3iznybdnX8mXSO7Wsuru7=P9Y=czQ@mail.gmail.com>
 <CAAKRu_bO0AopZhdpYVec+x8GsNrRxE4rq22AiOouViVVyOHU=w@mail.gmail.com>
 <0D990493-9DF4-4136-B1A1-CA0112E809ED@gmail.com>
 <CAAKRu_b_e96kWChReBPNjsD7GJuGKoM+exdyodP+8Oa=-T=pZA@mail.gmail.com>
 <CAAKRu_Ygia_e2aVJcnkSksy85wuYwk_WattzwKJSJvcVj72Tuw@mail.gmail.com>
 <377BC880-1616-4DEF-B9EF-5E297C358F7D@gmail.com>
To: Melanie Plageman <melanieplageman@gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/BF806DCB-D361-4626-9A68-95F50FD18CC8%40gmail.com>
Precedence: bulk


> On Feb 25, 2026, at 14:52, Chao Li <li.evan.chao@gmail.com> wrote:
>=20
>=20
>=20
>> On Feb 20, 2026, at 07:41, Melanie Plageman =
<melanieplageman@gmail.com> wrote:
>>=20
>> On Wed, Jan 14, 2026 at 6:35=E2=80=AFPM Melanie Plageman
>> <melanieplageman@gmail.com> wrote:
>>>=20
>>> Thanks! v13 attached.
>>=20
>> I've added background writer write combining and then spent some time
>> refactoring and benchmarking this over the last few weeks, and I've
>> realized I won't be able to get it in shape for Postgres 19. I've
>> attached version 14 here with my WIP code so I can come back to it in
>> the next release.
>>=20
>> 0012 is an invasive refactor of GetVictimBuffer() and all the
>> functions it calls that needs to be split apart into smaller commits
>> and redistributed throughout the other patches in the set. Other than
>> that, there are a number of todos both in the code and from a
>> benchmarking perspective:
>>=20
>> - Probe for preceding as well as following blocks when looking for
>> blocks to combine
>> - Experiment with different batch sizes
>> - Enable and test bulkread and vacuum write combining
>>=20
>> I also periodically get this troubling warning: "WARNING:  invalid
>> page in block 2 of relation "base/16384/2608_fsm"; zeroing out page".
>> So clearly there's at least one bug somewhere.
>>=20
>> FWIW benchmarks showed a large improvement from backend and bgwriter
>> write combining, but I'll provide more detailed output from =
benchmarks
>> when I post a more reviewable patch set.
>>=20
>> 0001 and 0002 are completely independent cleanup that should be =
pushed
>> anyway, so I might do that, but I didn't want to do so without having
>> posted on the list at least once.
>>=20
>> - Melanie
>> =
<v14-0001-Make-FlushUnlockedBuffer-use-its-parameters.patch><v14-0002-Make=
-ScheduleBufferTagForWriteback-static.patch><v14-0003-Streamline-buffer-re=
jection-for-bulkreads-of-unl.patch><v14-0004-Allow-PinBuffer-to-skip-incre=
asing-usage-count.patch><v14-0005-Eagerly-flush-bulkwrite-strategy-ring.pa=
tch><v14-0006-Write-combining-for-BAS_BULKWRITE.patch><v14-0007-Add-databa=
se-Oid-to-CkptSortItem.patch><v14-0008-Implement-checkpointer-data-write-c=
ombining.patch><v14-0009-Remove-SyncOneBuffer-and-refactor-BgBufferSync.pa=
tch><v14-0010-Normal-backend-write-combining.patch><v14-0011-Add-write-com=
bining-for-background-writer.patch><v14-0012-WIP-refactor.patch>
>=20
> A few comments for v14:
>=20
> 1 - 0001
> ```
> - FlushBuffer(buf, reln, IOOBJECT_RELATION, IOCONTEXT_NORMAL);
> + FlushBuffer(buf, reln, io_object, io_context);
> ```
>=20
> This changes the hardcode IOOBJECT_RELATION to io_object when calling =
FlushBuffer(). But FlushBuffer() itself still uses IOOBJECT_RELATION =
instead of io_object, so the change will actually not take effective:
> ```
>    pgstat_count_io_op_time(IOOBJECT_RELATION, io_context,
>                                              IOOP_WRITE, io_start, 1, =
BLCKSZ);
> ```
>=20
> So, an update in FlushBuffer() is also needed.
>=20
> 2 - 0003 - freelist.c
> ```
> bool
> StrategyRejectBuffer(BufferAccessStrategy strategy, BufferDesc *buf, =
bool from_ring)
> {
> + Assert(strategy);
> +
> /* We only do this in bulkread mode */
> if (strategy->btype !=3D BAS_BULKREAD)
> return false;
> @@ -795,8 +800,14 @@ StrategyRejectBuffer(BufferAccessStrategy =
strategy, BufferDesc *buf, bool from_r
> strategy->buffers[strategy->current] !=3D =
BufferDescriptorGetBuffer(buf))
> return false;
>=20
> + Assert(BufferIsLockedByMe(BufferDescriptorGetBuffer(buf)));
> + Assert(!(pg_atomic_read_u64(&buf->state) & BM_LOCKED));
> ```
>=20
> I don=E2=80=99t quite understand =
Assert(!(pg_atomic_read_u64(&buf->state) & BM_LOCKED));
>=20
> The comment says =E2=80=9Cthe buffer header spinlock must not be =
held,=E2=80=9D but I doubt this Assert actually ensures that. Since =
BM_LOCKED is just a bit in a shared state, isn't it possible for a =
concurrent backend to grab the lock right when we=E2=80=99re checking =
it?
>=20
> If a concurrent process locks the header a split-second before the =
Assert, we=E2=80=99ll get a false-positive crash in a dev environment =
even though the code is fine. If they lock it a split-second after, then =
the Assert didn't really catch anything. It feels like a race condition =
either way.
>=20
> I think the only way to truly ensure a spinlock is "not held" is to =
actually acquire it. If we=E2=80=99re just checking the bit like this, =
it=E2=80=99s just a snapshot that might be stale by the time the =
instruction finishes. What was the specific worry here=E2=80=94a =
self-deadlock, or something else?
>=20
> 3 - 0004
> ```
> +typedef enum BufferUsageCountChange
> +{
> + BUC_ZERO,
> + BUC_MAX_ONE,
> + BUC_ONE,
> +} BufferUsageCountChange;
> ```
>=20
> Can we add some brief comments to explain every enum item? I feel hard =
to guess the meaning from the names without further reading the code.
>=20
> 4 - 0005 - bufmgr.c
> ```
> +/*
> + * Prepare bufdesc for eager flushing.
> + *
> + * Given bufnum, return the buffer descriptor of the buffer to =
eagerly flush,
> + * pinned and locked and with BM_IO_IN_PROGRESS set, or NULL if this =
buffer
> + * does not contain a block that should be flushed.
> + *
> + * If returning a buffer, also return its LSN.
> + */
> +static BufferDesc *
> +PrepareOrRejectEagerFlushBuffer(Buffer bufnum)
> ```
>=20
> This header comment looks stale.
>=20
> * It says =E2=80=9Cand with BM_IO_IN_PROGRESS set=E2=80=9D, but I =
don=E2=80=99t see where BM_IO_IN_PROGRESS is set in this function.
> * It says =E2=80=9Calso return its LSN=E2=80=9D, but I don=E2=80=99t =
see LSN is returned.
>=20
> 5 - 0006
> ```
> + /* Start IO on the first buffer */
> + if (!StartBufferIO(buf_hdr, false, false))
> + goto clean;
> ```
>=20
> This failure branch feels like leaking the content lock. The buffer =
was already share-locked via BufferLockConditional() earlier, and I =
don=E2=80=99t see the =E2=80=9Cclean" path unlock that content lock =
before returning.
>=20
> 6 - 0009
> ```
> + FlushUnlockedBuffer(bufHdr, NULL, IOOBJECT_RELATION, =
IOCONTEXT_NORMAL);
> + UnpinBuffer(bufHdr);
> +
> + ScheduleBufferTagForWriteback(wb_context, IOCONTEXT_NORMAL, =
&bufHdr->tag);
> ```
>=20
> bufHdr is unlocked and unpinned, it might be unsafe to still use =
bufHdr->tag. I think we can copy bufHdr->tag to a local variable before =
unlock the buffer.
>=20
> ~~ My brain is stuck, I will continue 0010 tomorrow ~~
>=20

7 - 0010
```
- * batch_limit is the largest batch we are allowed to construct given =
the
- * remaining blocks in the table, the number of available pins, and the
- * current configuration.
+ * max_batch_size is the maximum number of blocks that can be combined =
into a
+ * single write in general. This function, based on the block number of =
start,
+ * will determine the maximum IO size for this particular write given =
how much
+ * of the file remains. max_batch_size is provided by the caller so it =
doesn't
+ * have to be recalculated for each write.
```

I don=E2=80=99t see the function FindStrategyFlushAdjacents() has a =
parameter named max_batch_size, but batch_limit is still there.

8 - 0010
```
 static void
+FindFlushAdjacents(BufferDesc *batch_start,
+				   uint32 batch_limit,
+				   BufferWriteBatch *batch)
+{
+	BufferTag	require;		/* requested block's =
buffer tag */
+	uint32		newHash;		/* hash value for =
require */
+	LWLock	   *newPartitionLock;	/* buffer partition lock for it =
*/
+	int			buf_id;
+
+	/* create a tag so we can lookup the buffers */
+	InitBufferTag(&require, &batch->reln->smgr_rlocator.locator,
+				  batch->forkno, InvalidBlockNumber);
+
+	for (; batch->n < batch_limit; batch->n++)
+	{
+		XLogRecPtr	lsn;
+
+		require.blockNum =3D batch->start + batch->n;
+
+		Assert(BlockNumberIsValid(require.blockNum));
```

When I first time read the Assert, I was confused how it can assume the =
block to be valid, then I realized that WriteBatchInit() ensures a safe =
=E2=80=9Climit=E2=80=9D. Maybe add a brief comment for the Assert.

9 - 0010
```
+		batch->bufdescs[batch->n] =3D
+			PrepareOrRejectEagerFlushBuffer(buf_id + 1,
+										=
	&require,
+										=
	newPartitionLock,
+										=
	&lsn);
+		if (lsn > batch->max_lsn)
+			batch->max_lsn =3D lsn;
+
+		if (batch->bufdescs[batch->n] =3D=3D NULL)
+			break;
```

I think we can move if (batch->bufdescs[batch->n] =3D=3D NULL) to before =
if (lsn > batch->max_lsn).

This is a correctness issue, because PrepareOrRejectEagerFlushBuffer =
will set lsn to InvalidXLogRecPtr when returns NULL, but doing the NULL =
check early just feels more reasonable.

10 - 0010
```
+ * max_lsn may be updated if the provided buffer LSN exceeds the =
current max
+ * LSN.
  */
 static BufferDesc *
 PrepareOrRejectEagerFlushBuffer(Buffer bufnum,
 								=
BufferTag *require,
+								LWLock =
*buftable_lock,
 								=
XLogRecPtr *lsn)
```

The function doesn=E2=80=99t have a parameter named max_lsn, is it =
=E2=80=9Clsn=E2=80=9D?

11 - 0010
```
+FindFlushAdjacents(BufferDesc *batch_start,
+				   uint32 batch_limit,
+				   BufferWriteBatch *batch)
```

Looks like batch_start is not used at all in this function.

12 - 0011
```
+ * Callers specify if and by how much they want to bump the buffer's =
usage
+ * count.
```

I don=E2=80=99t get what this comment means.

~~ As 0012 is WIP, I didn't review it. ~~

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