From: Chao Li <li.evan.chao@gmail.com>
To: Melanie Plageman <melanieplageman@gmail.com>
Cc: Nazir Bilal Yavuz <byavuz81@gmail.com>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Cc: Andres Freund <andres@anarazel.de>
Subject: Re: Checkpointer write combining
Date: Fri, 12 Sep 2025 11:33:17 +0800
Message-ID: <3FC2442D-1012-4079-A009-A0B8C45E092D@gmail.com> (raw)
In-Reply-To: <CAAKRu_atZuU3gYY6hVVuvqDW+mfXM+fCWCyYODbZPesxyr=y6g@mail.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>
> On Sep 12, 2025, at 07:11, Melanie Plageman <melanieplageman@gmail.com> wrote:
>
> On Wed, Sep 10, 2025 at 4:24 AM Chao Li <li.evan.chao@gmail.com> wrote:
>>
>
> Thanks for the review!
>
> For any of your feedback that I simply implemented, I omitted an
> inline comment about it. Those changes are included in the attached
> v6. My inline replies below are only for feedback requiring more
> discussion.
>
>> On Sep 10, 2025, at 01:55, Melanie Plageman <melanieplageman@gmail.com> wrote:
>>
>> 2 - 0001
>> ```
>> --- a/src/backend/storage/buffer/freelist.c
>> +++ b/src/backend/storage/buffer/freelist.c
>>
>> + if (XLogNeedsFlush(lsn))
>> + {
>> + /*
>> + * Remove the dirty buffer from the ring; necessary to prevent an
>> + * infinite loop if all ring members are dirty.
>> + */
>> + strategy->buffers[strategy->current] = InvalidBuffer;
>> + return true;
>> + }
>>
>> - return true;
>> + return false;
>> }
>> ```
>>
>> We can do:
>> ```
>> If (!XLogNeedsFlush(lan))
>> Return false
>>
>> /* Remove the dirty buffer ….
>> */
>> Return true;
>> }
>> ```
>
> This would make the order of evaluation the same as master but I
> actually prefer it this way because then we only take the buffer
> header spinlock if there is a chance we will reject the buffer (e.g.
> we don't need to examine it for strategies except BAS_BULKREAD)
>
>
I don’t understand why the two versions are different:
if (XLogNeedsFlush(lsn))
{
/*
* Remove the dirty buffer from the ring; necessary to prevent an
* infinite loop if all ring members are dirty.
*/
strategy->buffers[strategy->current] = InvalidBuffer;
return true;
}
return false;
VS
if (XLogNeedsFlush(lsn))
return false;
/*
* Remove the dirty buffer from the ring; necessary to prevent an
* infinite loop if all ring members are dirty.
*/
strategy->buffers[strategy->current] = InvalidBuffer;
return true;
>
>> 10 - 0004
>> ```
>> --- a/src/backend/storage/buffer/bufmgr.c
>> +++ b/src/backend/storage/buffer/bufmgr.c
>>
>> + limit = Min(max_batch_size, limit);
>> ```
>>
>> Do we need to check max_batch_size should be less than (MAX_IO_COMBINE_LIMIT-1)? Because BufWriteBatch.bufdescs is defined with length of MAX_IO_COMBINE_LIMIT, and the first place has been used to store “start”.
>
> I assert that in StrategyMaxWriteBatchSize(). io_combine_limit is not
> allowed to exceed MAX_IO_COMBINE_LIMIT, so it shouldn't happen anyway,
> since we are capping ourselves at io_combine_limit. Or is that your
> point?
>
Please ignore comment 10. I think I cross-line it in my original email. I added the comment, then lately I found you have checked MAX_IO_COMBINE_LIMIT in the other function, so tried to delete it by cross-lining the comment.
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, pgsql-hackers@lists.postgresql.org, andres@anarazel.de
Subject: Re: Checkpointer write combining
In-Reply-To: <3FC2442D-1012-4079-A009-A0B8C45E092D@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