Re: Checkpointer write combining - Mailing list pgsql-hackers

> On Nov 4, 2025, at 07:34, Melanie Plageman <melanieplageman@gmail.com> wrote:
>
> Thanks for continuing to review! I've revised the patches to
> incorporate all of your feedback except for where I mention below.
>
> There were failures in CI due to issues with max batch size, so
> attached v8 also seeks to fix those.
>
> - Melanie
>
> On Thu, Oct 16, 2025 at 12:25 AM Chao Li <li.evan.chao@gmail.com> wrote:
>>
>> 3 - 0003
>> ```
>> +/*
>> + * Return the next buffer in the ring or InvalidBuffer if the current sweep is
>> + * over.
>> + */
>> +Buffer
>> +StrategySweepNextBuffer(BufferAccessStrategy strategy, int *sweep_cursor)
>> +{
>> +       if (++(*sweep_cursor) >= strategy->nbuffers)
>> +               *sweep_cursor = 0;
>> +
>> +       return strategy->buffers[*sweep_cursor];
>> +}
>> ```
>>
>> Feels the function comment is a bit confusing, because the function code doesn’t really perform sweep, the function
isjust a getter. InvalidBuffer just implies the current sweep is over. 
>>
>> Maybe rephrase to something like: “Return the next buffer in the range. If InvalidBuffer is returned, that implies
thecurrent sweep is done." 
>
> Yes, actually I think having these helpers mention the sweep is more
> confusing than anything else. I've revised them to be named more
> generically and updated the comments accordingly.
>
>> 5 - 0004
>> ```
>> +uint32
>> +StrategyMaxWriteBatchSize(BufferAccessStrategy strategy)
>> +{
>> +       uint32          max_possible_buffer_limit;
>> +       uint32          max_write_batch_size;
>> +       int                     strategy_pin_limit;
>> +
>> +       max_write_batch_size = io_combine_limit;
>> +
>> +       strategy_pin_limit = GetAccessStrategyPinLimit(strategy);
>> +       max_possible_buffer_limit = GetPinLimit();
>> +
>> +       max_write_batch_size = Min(strategy_pin_limit, max_write_batch_size);
>> +       max_write_batch_size = Min(max_possible_buffer_limit, max_write_batch_size);
>> +       max_write_batch_size = Max(1, max_write_batch_size);
>> +       max_write_batch_size = Min(max_write_batch_size, io_combine_limit);
>> +       Assert(max_write_batch_size < MAX_IO_COMBINE_LIMIT);
>> +       return max_write_batch_size;
>> +}
>> ```
>>
>> This implementation is hard to understand. I tried to simplify it:
>> ```
>> uint32
>> StrategyMaxWriteBatchSize(BufferAccessStrategy strategy)
>> {
>>        int strategy_pin_limit = GetAccessStrategyPinLimit(strategy);
>>        uint32 max_write_batch_size = Min(GetPinLimit(), (uint32)strategy_pin_limit);
>>
>>        /* Clamp to io_combine_limit and enforce minimum of 1 */
>>        if (max_write_batch_size > io_combine_limit)
>>                max_write_batch_size = io_combine_limit;
>>        if (max_write_batch_size == 0)
>>                max_write_batch_size = 1;
>>
>>        Assert(max_write_batch_size < MAX_IO_COMBINE_LIMIT);
>>        return max_write_batch_size;
>> }
>> ```
>
> I agree that the implementation was hard to understand. I've not quite
> gone with your version but I have rewritten it like this:
>
> uint32
> StrategyMaxWriteBatchSize(BufferAccessStrategy strategy)
> {
>    uint32        max_write_batch_size = Min(io_combine_limit,
> MAX_IO_COMBINE_LIMIT);
>    int            strategy_pin_limit = GetAccessStrategyPinLimit(strategy);
>    uint32        max_possible_buffer_limit = GetPinLimit();
>
>    /* Identify the minimum of the above */
>    max_write_batch_size = Min(strategy_pin_limit, max_write_batch_size);
>    max_write_batch_size = Min(max_possible_buffer_limit, max_write_batch_size);
>
>    /* Must allow at least 1 IO for forward progress */
>    max_write_batch_size = Max(1, max_write_batch_size);
>
>    return max_write_batch_size;
> }
>
> Is this better?

Yes, I think your version is safer because it enforces the max limit at runtime instead of only asserting it in debug
builds.

>
> - Melanie
>
<v8-0001-Refactor-goto-into-for-loop-in-GetVictimBuffer.patch><v8-0002-Split-FlushBuffer-into-two-parts.patch><v8-0003-Eagerly-flush-bulkwrite-strategy-ring.patch><v8-0004-Write-combining-for-BAS_BULKWRITE.patch><v8-0005-Add-database-Oid-to-CkptSortItem.patch><v8-0006-Implement-checkpointer-data-write-combining.patch><v8-0007-WIP-Refactor-SyncOneBuffer-for-bgwriter-only.patch>

I quickly went through 0001-0006, looks good to me now. As 0007 has WIP in the subject, I skipped it.

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