From: Chao Li <li.evan.chao@gmail.com>
To: Melanie Plageman <melanieplageman@gmail.com>
Cc: Nazir Bilal Yavuz <byavuz81@gmail.com>
Cc: Kirill Reshke <reshkekirill@gmail.com>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Cc: Andres Freund <andres@anarazel.de>
Subject: Re: Eagerly evict bulkwrite strategy ring
Date: Wed, 19 Nov 2025 09:45:59 +0800
Message-ID: <BA606005-DAFD-4470-BDCF-9CC5418B1412@gmail.com> (raw)
In-Reply-To: <CAAKRu_ZbdGfp9=QRWajNkC95T5U5m3m_J6029KFB80V9ai==8g@mail.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>
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:
>
> On Mon, Nov 3, 2025 at 3:06 PM Melanie Plageman
> <melanieplageman@gmail.com> wrote:
>>
>> I found an incorrect assert in CleanVictimBuffer() that was tripping
>> in CI. Attached v6 is updated with it removed.
>
> v7 rebased over recent changes in bufmgr.c
>
> - Melanie
> <v7-0001-Refactor-goto-into-for-loop-in-GetVictimBuffer.patch><v7-0002-Split-FlushBuffer-into-two-parts.patch><v7-0003-Eagerly-flush-bulkwrite-strategy-ring.patch>
0001 only changes “goto" to “for”, thus it's supposed no logic change.
In the old code:
```
if (strategy != NULL)
{
XLogRecPtr lsn;
/* Read the LSN while holding buffer header lock */
buf_state = LockBufHdr(buf_hdr);
lsn = 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 != BAS_BULKREAD)
return false;
/* Don't muck with behavior of normal buffer-replacement strategy */
if (!from_ring ||
strategy->buffers[strategy->current] != BufferDescriptorGetBuffer(buf))
return false;
LockBufHdr(buf);
lsn = 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/
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: li.evan.chao@gmail.com, melanieplageman@gmail.com, byavuz81@gmail.com, reshkekirill@gmail.com, pgsql-hackers@lists.postgresql.org, andres@anarazel.de
Subject: Re: Eagerly evict bulkwrite strategy ring
In-Reply-To: <BA606005-DAFD-4470-BDCF-9CC5418B1412@gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox