Content-Type: text/plain;
	charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3826.700.81\))
Subject: Re: Eagerly evict bulkwrite strategy ring
From: Chao Li <li.evan.chao@gmail.com>
In-Reply-To: 
 <CAAKRu_ZbdGfp9=QRWajNkC95T5U5m3m_J6029KFB80V9ai==8g@mail.gmail.com>
Date: Wed, 19 Nov 2025 09:45:59 +0800
Cc: Nazir Bilal Yavuz <byavuz81@gmail.com>,
 Kirill Reshke <reshkekirill@gmail.com>,
 PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>,
 Andres Freund <andres@anarazel.de>
Content-Transfer-Encoding: quoted-printable
Message-Id: <BA606005-DAFD-4470-BDCF-9CC5418B1412@gmail.com>
References: 
 <CAAKRu_Yjn4mvN9NBxtmsCQSGwup45CoA4e05nhR7ADP-v0WCig@mail.gmail.com>
 <CALdSSPhcZbof=6GGmAzeqYbEbDDXJhAbrFZ=3NepxiorF8fBOg@mail.gmail.com>
 <CAAKRu_bwy5=Rz-rizSc-LhHP08nkwSQ_Eix2b256RDmgRj4wSw@mail.gmail.com>
 <CAAKRu_a=+jwQDUYkoC=nWz6Ky6CGgE-ZW1XzfYaeFrQEC6+iaw@mail.gmail.com>
 <CAN55FZ0r=0OjUawtt6Q9Gc15Ue=1i-VSMxBxAzTxDHv+tTr4zA@mail.gmail.com>
 <CAAKRu_ahRXCNbPY5i3YSzwZJzDK60GhAqgL76svJgZxjXugxoQ@mail.gmail.com>
 <CAN55FZ0CTPnDdNszwDKpfdACHDhY12ArybH4smyD_QEJ+msQZg@mail.gmail.com>
 <CAAKRu_Y9WeWb5fu7Aibh-qGZuFYq3=57Y4ptSrzLYCnRWDHCow@mail.gmail.com>
 <CAAKRu_ZdU2VwDXF_kpnfojNzzXsJOdLDr_9GN6ZOnGE-QDKUCA@mail.gmail.com>
 <CAAKRu_ZbdGfp9=QRWajNkC95T5U5m3m_J6029KFB80V9ai==8g@mail.gmail.com>
To: Melanie Plageman <melanieplageman@gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/BA606005-DAFD-4470-BDCF-9CC5418B1412%40gmail.com>
Precedence: bulk

Hi Melanie,

I remember I ever reviewed this patch. But when I revisit v7, I just got =
a confusion to clarify.

> On Nov 19, 2025, at 03:13, Melanie Plageman =
<melanieplageman@gmail.com> wrote:
>=20
> On Mon, Nov 3, 2025 at 3:06=E2=80=AFPM Melanie Plageman
> <melanieplageman@gmail.com> wrote:
>>=20
>> I found an incorrect assert in CleanVictimBuffer() that was tripping
>> in CI. Attached v6 is updated with it removed.
>=20
> v7 rebased over recent changes in bufmgr.c
>=20
> - Melanie
> =
<v7-0001-Refactor-goto-into-for-loop-in-GetVictimBuffer.patch><v7-0002-Spl=
it-FlushBuffer-into-two-parts.patch><v7-0003-Eagerly-flush-bulkwrite-strat=
egy-ring.patch>

0001 only changes =E2=80=9Cgoto" to =E2=80=9Cfor=E2=80=9D, thus it's =
supposed no logic change.

In the old code:
```
		if (strategy !=3D NULL)
		{
			XLogRecPtr	lsn;

			/* Read the LSN while holding buffer header lock =
*/
			buf_state =3D LockBufHdr(buf_hdr);
			lsn =3D BufferGetLSN(buf_hdr);
			UnlockBufHdr(buf_hdr);

			if (XLogNeedsFlush(lsn)
				&& StrategyRejectBuffer(strategy, =
buf_hdr, from_ring))
			{
				LWLockRelease(content_lock);
				UnpinBuffer(buf_hdr);
				goto again;
			}
		}
```
It only retries when XLogNeedsFlush(lsn) is true.

In the patch, everything are merged into StrategyRejectBuffer():
```
			if (StrategyRejectBuffer(strategy, buf_hdr, =
from_ring))
			{
				LWLockRelease(content_lock);
				UnpinBuffer(buf_hdr);
				continue;
			}
```
When StrategyRejectBuffer(strategy, buf_hdr, from_ring) is true, retry =
happens. However, look into the function:
```
bool
StrategyRejectBuffer(BufferAccessStrategy strategy, BufferDesc *buf, =
bool from_ring)
{
	XLogRecPtr	lsn;

	if (!strategy)
		return false;

	/* We only do this in bulkread mode */
	if (strategy->btype !=3D BAS_BULKREAD)
		return false;

	/* Don't muck with behavior of normal buffer-replacement =
strategy */
	if (!from_ring ||
		strategy->buffers[strategy->current] !=3D =
BufferDescriptorGetBuffer(buf))
		return false;

	LockBufHdr(buf);
	lsn =3D BufferGetLSN(buf);
	UnlockBufHdr(buf);

	if (XLogNeedsFlush(lsn))
		return false;
```

When XLogNeedsFlush(lsn) is true, StrategyRejectBuffer returns false, =
thus no retry will happen, which is different from the old logic, is =
that an intentional change?

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