Re: Allow io_combine_limit up to 1MB

Hi,

On 2025-02-12 15:24:21 +1300, Thomas Munro wrote:
> On Wed, Feb 12, 2025 at 3:22 PM Andres Freund <andres(at)anarazel(dot)de> wrote:
> > On 2025-02-12 13:59:21 +1300, Thomas Munro wrote:
> > > How about just maintaining it in a new variable
> > > effective_io_combine_limit, whenever either of them is assigned?
> >
> > Yea, that's probably the least bad way.
> >
> > I wonder if we should just name that variable io_combine_limit and have the
> > GUC be _raw or _guc or such? There's gonna be a fair number of references to
> > the variable in code...
>
> Alternatively, we could compute that as stream->io_combine_limit and
> use that. That has the advantage that it's fixed for the life of the
> stream, even if you change it (eg between fetches from a CURSOR that
> has streams). Pretty sure it won't break anything today, but it might
> just run out of queue space limiting concurrency arbitrarily if you
> increase it, which is a bit weird now that I focus on that. Capturing
> the value we'll use up front seems better on that front.

> On the other hand, other future code might also have to remember to compute
> that too (write streams, ...), a tiny bit of duplication.

Yep, was also "worried" about that.

> Or ... I guess we could do both things?

Maybe that'd be the best approach? Not sure.

> From 8cfa23a370a4564a0369991e2b0068b48983a0f6 Mon Sep 17 00:00:00 2001
> From: Thomas Munro <thomas(dot)munro(at)gmail(dot)com>
> Date: Wed, 12 Feb 2025 13:52:22 +1300
> Subject: [PATCH v2] Introduce max_io_combine_limit.
>
> The existing io_combine_limit parameter can be set by users. The new
> max_io_combine_limit parameter can be set only at server startup time.
> Code that combines I/Os should respect both of them by taking the
> smaller value.
>
> This allows the administrator to cap the total I/O size system-wide, but
> also provides a way for proposed patches to know what the maximum could
> possibly be in cases where it is multiplied by other GUCs to allocate
> shared memory, without having to assume that it's as high as the
> compile-time MAX_IO_COMBINE_LIMIT value.
>
> The read_stream.c code is changed to compute the minimum value up front
> as stream->io_combine_limit instead of using io_combine_limit directly.
> That has the extra benefit of remaining stable throughout the lifetime
> of the stream even if the user changes it (eg while consuming from a
> CURSOR). As previously coded, an mid-stream increase could limit
> concurrency artificially just because we run out of queue space too
> soon.
> ---
> doc/src/sgml/config.sgml | 23 ++++++++++++++-
> src/backend/commands/variable.c | 1 -
> src/backend/storage/aio/read_stream.c | 29 ++++++++++++-------
> src/backend/storage/buffer/bufmgr.c | 5 ++--
> src/backend/utils/misc/postgresql.conf.sample | 2 ++
> src/include/storage/bufmgr.h | 1 +
> 6 files changed, 47 insertions(+), 14 deletions(-)
>
> diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
> index 3b557ecabfb..c6de8b9e236 100644
> --- a/doc/src/sgml/config.sgml
> +++ b/doc/src/sgml/config.sgml
> @@ -2605,6 +2605,24 @@ include_dir 'conf.d'
> </listitem>
> </varlistentry>
>
> + <varlistentry id="guc-max-io-combine-limit" xreflabel="max_io_combine_limit">
> + <term><varname>max_io_combine_limit</varname> (<type>integer</type>)
> + <indexterm>
> + <primary><varname>max_io_combine_limit</varname> configuration parameter</primary>
> + </indexterm>
> + </term>
> + <listitem>
> + <para>
> + Controls the largest I/O size in operations that combine I/O, and silently
> + limits the user-settable parameter <varname>io_combine_limit</varname>.
> + This parameter can only be set in
> + the <filename>postgresql.conf</filename> file or on the server
> + command line.
> + The default is 128kB.
> + </para>
> + </listitem>
> + </varlistentry>

I can see an argument for having the max be slightly higher than the default,
but still less than MAX_IO_COMBINE_LIMIT. But I think just about anything is
fine for now.

> --- a/src/backend/commands/variable.c
> +++ b/src/backend/commands/variable.c
> @@ -1156,7 +1156,6 @@ assign_maintenance_io_concurrency(int newval, void *extra)
> #endif
> }
>
> -
> /*
> * These show hooks just exist because we want to show the values in octal.
> */

Bogus hunk?

> @@ -402,6 +403,7 @@ read_stream_begin_impl(int flags,
> size_t per_buffer_data_size)
> {
> ReadStream *stream;
> + int effective_io_combine_limit;
> size_t size;
> int16 queue_size;
> int max_ios;
> @@ -409,6 +411,12 @@ read_stream_begin_impl(int flags,
> uint32 max_pinned_buffers;
> Oid tablespace_id;
>
> + /*
> + * Respect both the system-wide limit and the user-settable limit on I/O
> + * combining size.
> + */
> + effective_io_combine_limit = Min(max_io_combine_limit, io_combine_limit);
> +
> /*
> * Decide how many I/Os we will allow to run at the same time. That
> * currently means advice to the kernel to tell it that we will soon read.

Heh, somehow effective_* now gives me hives almost immediately :)

Greetings,

Andres Freund