> On Sep 12, 2025, at 07:11, Melanie Plageman <[email protected]> wrote:
>
> On Wed, Sep 10, 2025 at 4:24 AM Chao Li <[email protected]> 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 <[email protected]>
>> 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/
