On Wed, Nov 12, 2025 at 6:40 AM BharatDB <[email protected]> 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.

> 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.

> 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)

> 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.

- Melanie


Reply via email to