Re: Checkpointer write combining

Hi Melanie,

Thank you for the detailed clarifications which helped me understand
the constraints much more clearly. You are absolutely right regarding
the key points you specified. My initial concern came from trying to
avoid cases where strategy pin limits were unexpectedly small (0 or
negative) or global pin limits temporarily shrink due to memory issue
/ fast cycling of pins.

On Wed, Nov 19, 2025 at 12:00 AM Melanie Plageman
<melanieplageman(at)gmail(dot)com> wrote:
>
> On Wed, Nov 12, 2025 at 6:40 AM BharatDB <bharatdbpg(at)gmail(dot)com> wrote:
> >
> > Considering the CI failures in earlier patch versions around “max batch size”, upon my observation I found the failures arise either from boundary conditions when io_combine_limit (GUC) is set larger than the compile-time MAX_IO_COMBINE_LIMIT
>
> How could io_combine_limit be higher than MAX_IO_COMBINE_LIMIT? The
> GUC is capped to MAX_IO_COMBINE_LIMIT -- see guc_parameters.dat.

After revisiting guc_parameters.dat, Now I see that the GUC is
strictly capped at MAX_IO_COMBINE_LIMIT, so comparisons against larger
values are unnecessary. Thus my earlier concern came from assuming
some unbounded user-provided values are not applicable here.

> > or when pin limits return small/zero values due to which it produce out-of-range batch sizes or assertion failures in CI.
>
> This is true. But I think it can be solved with a single comparison to
> 1 as in the recent version.
ok

> > Comparing the approaches suggested in the thread, I think implementing (GUC + compile-time cap first, and then pin limits) could be more effective in avoiding CI failures and also we should consider the following logic conditions:
> >
> > Set io_combine_limit == 0 explicitly (fallback to 1 for forward progress).
>
> The io_combine_limit GUC has a minimum value of 1 (in guc_parameters.dat)
Noted.

> > Use explicit typed comparisons to avoid signed/unsigned pitfalls and add a final Assert() to capture assumptions in CI.
>
> I think my new version works.
>
> 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;
>
> GetAccessStrategyPinLimit() is the only function returning a signed
> value here -- and it should always return a positive value (while I
> wish we would use unsigned integers when a value will never be
> negative, the strategy->nbuffers struct member was added a long time
> ago). Then the Min() macro should work correctly and produce a value
> that fits in a uint32 because of integer promotion rules.

Th explanation about GetAccessStrategyPinLimit() (despite being int)
makes sense and I agree that with the Min() macro and integer
promotion rules, the outcome is always safe. Therefore, explicit typed
comparisons are indeed redundant. However, after reviewing the
existing code paths and your updated version,
max_write_batch_size = Max(1, max_write_batch_size);
=> It is clear that both GetAccessStrategyPinLimit() and GetPinLimit()
should always return sensible positive values and the fallback fully
covers the forward-progress requirement. And I agree that it is both
correct and sufficient for the CI failures we were seeing earlier.

Thank you for helping me understand the reasoning behind this design.
And this will be kept in mind for further work on implementing write
combining.
I appreciate your patience and the opportunity to refine my
assumptions. Looking forward to more suggestions and feedbacks.

Thanking you.

Regards,
Soumya