> On Nov 4, 2025, at 07:34, Melanie Plageman <[email protected]> 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 <[email protected]> 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 is just 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 the current 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/
