Re: Allow io_combine_limit up to 1MB

Hi,

On 2025-03-18 16:18:17 +1300, Thomas Munro wrote:
> Here's a new version that also adjusts the code that just landed in
> da722699:

Something isn't quite right with this code. If I just add -c
io_combine_limit=32 to the options and do a seqscan, I get odd
failures. Mostly assertion failures about buffers not being valid in
CheckReadBuffersOperation().

Sure glad I added CheckReadBuffersOperation(), that'd have been much nastier
to figure out without those assertion failures.

A bit of debugging later: Ie figured out that this is because io_combine_limit
is bigger than io_max_combine_limit, so the iovecs of one IO overlap with
another. With predictably hilarious results.

I think it might be a small thing:
Since our GUC system doesn't support dependencies or cross-checks
between GUCs, the user-settable one now assigns a "raw" value to
io_combine_limit_guc, and the lower of io_combine_limit_guc and
io_max_combine_limit is maintained in io_combine_limit.

However, the IO combine limit GUC still references io_combine_limit:

{
{"io_combine_limit",
PGC_USERSET,
RESOURCES_IO,
gettext_noop("Limit on the size of data reads and writes."),
NULL,
GUC_UNIT_BLOCKS
},
&io_combine_limit,
DEFAULT_IO_COMBINE_LIMIT,
1, MAX_IO_COMBINE_LIMIT,
NULL, assign_io_combine_limit, NULL
},

Therefore the GUC machinery undoes the work of io_combine_limit done in
assign_io_combine_limit:

/*
* GUC assign hooks that recompute io_combine_limit whenever
* io_combine_limit_guc and io_max_combine_limit are changed. These are needed
* because the GUC subsystem doesn't support dependencies between GUCs, and
* they may be assigned in either order.
*/
void
assign_io_max_combine_limit(int newval, void *extra)
{
io_max_combine_limit = newval;
io_combine_limit = Min(io_max_combine_limit, io_combine_limit_guc);
}
void
assign_io_combine_limit(int newval, void *extra)
{
io_combine_limit_guc = newval;
io_combine_limit = Min(io_max_combine_limit, io_combine_limit_guc);
}

So we end up with a larger io_combine_limit than
io_max_combine_limit. Hilarity ensues.

Besides the obvious fix, I think we should also add
Assert(len <= io_max_combine_limit);

to pgaio_io_set_handle_data_32/64, that'd have made the bug much more obvious.

Greetings,

Andres Freund