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