Re: Allow io_combine_limit up to 1MB - Mailing list pgsql-hackers

Hi,

On 2025-02-12 15:24:21 +1300, Thomas Munro wrote:
> On Wed, Feb 12, 2025 at 3:22 PM Andres Freund <andres@anarazel.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.munro@gmail.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