Re: AIO v2.5

On Wed, Mar 12, 2025 at 01:06:03PM -0400, Andres Freund wrote:
> On 2025-03-11 20:57:43 -0700, Noah Misch wrote:
> > - Like you say, "redefine max_files_per_process to be about the number of
> > files each *backend* will additionally open". It will become normal that
> > each backend's actual FD list length is max_files_per_process + MaxBackends
> > if io_method=io_uring. Outcome is not unlike
> > v6-0002-Bump-postmaster-soft-open-file-limit-RLIMIT_NOFIL.patch +
> > v6-0003-Reflect-the-value-of-max_safe_fds-in-max_files_pe.patch but we don't
> > mutate max_files_per_process. Benchmark results should not change beyond
> > the inter-major-version noise level unless one sets io_method=io_uring. I'm
> > feeling best about this one, but I've not been thinking about it long.
>
> Yea, I think that's something probably worth doing separately from Jelte's
> patch. I do think that it'd be rather helpful to have jelte's patch to
> increase NOFILE in addition though.

Agreed.

> > > > > +static void
> > > > > +maybe_adjust_io_workers(void)
> > > >
> > > > This also restarts workers that exit, so perhaps name it
> > > > start_io_workers_if_missing().
> > >
> > > But it also stops IO workers if necessary?
> >
> > Good point. Maybe just add a comment like "start or stop IO workers to close
> > the gap between the running count and the configured count intent".
>
> It's now
> /*
> * Start or stop IO workers, to close the gap between the number of running
> * workers and the number of configured workers. Used to respond to change of
> * the io_workers GUC (by increasing and decreasing the number of workers), as
> * well as workers terminating in response to errors (by starting
> * "replacement" workers).
> */

Excellent.

> > > > > +{
> > > > ...
> > > > > + /* Try to launch one. */
> > > > > + child = StartChildProcess(B_IO_WORKER);
> > > > > + if (child != NULL)
> > > > > + {
> > > > > + io_worker_children[id] = child;
> > > > > + ++io_worker_count;
> > > > > + }
> > > > > + else
> > > > > + break; /* XXX try again soon? */
> > > >
> > > > Can LaunchMissingBackgroundProcesses() become the sole caller of this
> > > > function, replacing the current mix of callers? That would be more conducive
> > > > to promptly doing the right thing after launch failure.
> > >
> > > I'm not sure that'd be a good idea - right now IO workers are started before
> > > the startup process, as the startup process might need to perform IO. If we
> > > started it only later in ServerLoop() we'd potentially do a fair bit of work,
> > > including starting checkpointer, bgwriter, bgworkers before we started IO
> > > workers. That shouldn't actively break anything, but it would likely make
> > > things slower.
> >
> > I missed that. How about keeping the two calls associated with PM_STARTUP but
> > replacing the assign_io_workers() and process_pm_child_exit() calls with one
> > in LaunchMissingBackgroundProcesses()?
>
> I think replacing the call in assign_io_workers() is a good idea, that way we
> don't need assign_io_workers().
>
> Less convinced it's a good idea to do the same for process_pm_child_exit() -
> if IO workers errored out we'll launch backends etc before we get to
> LaunchMissingBackgroundProcesses(). That's not a fundamental problem, but
> seems a bit odd.

Works for me.

> I think LaunchMissingBackgroundProcesses() should be split into one that
> starts aux processes and one that starts bgworkers. The one maintaining aux
> processes should be called before we start backends, the latter not.

That makes sense, though I've not thought about it much.

> > > > > + /*
> > > > > + * It's very unlikely, but possible, that reopen fails. E.g. due
> > > > > + * to memory allocations failing or file permissions changing or
> > > > > + * such. In that case we need to fail the IO.
> > > > > + *
> > > > > + * There's not really a good errno we can report here.
> > > > > + */
> > > > > + error_errno = ENOENT;
> > > >
> > > > Agreed there's not a good errno, but let's use a fake errno that we're mighty
> > > > unlikely to confuse with an actual case of libc returning that errno. Like
> > > > one of EBADF or EOWNERDEAD.
> > >
> > > Can we rely on that to be present on all platforms, including windows?
> >
> > I expect EBADF is universal. EBADF would be fine.
>
> Hm, that's actually an error that could happen for other reasons, and IMO
> would be more confusing than ENOENT. The latter describes the issue to a
> reasonable extent, EBADFD seems like it would be more confusing.
>
> I'm not sure it's worth investing time in this - it really shouldn't happen,
> and we probably have bigger problems than the error code if it does. But if we
> do want to do something, I think I can see a way to report a dedicated error
> message for this.

I agree it's not worth much investment. Let's leave that one as-is. We can
always change it further if the not-really-good errno shows up too much.

> > https://github.com/coreutils/gnulib/blob/master/doc/posix-headers/errno.texi
> > lists some OSs not having it, the newest of which looks like NetBSD 9.3
> > (2022). We could use it and add a #define for platforms lacking it.
>
> What would we define it as? I guess we could just pick a high value, but...

Some second-best value, but I withdraw that idea.

On Wed, Mar 12, 2025 at 07:23:47PM -0400, Andres Freund wrote:
> Attached is v2.7, with the following changes:

> Unresolved:
>
> - Whether to continue starting new workers in process_pm_child_exit()

I'm fine with that continuing. It's hurting ~nothing.

> - What to name the view (currently pg_aios). I'm inclined to go for
> pg_io_handles right now.

I like pg_aios mildly better than pg_io_handles, since "handle" sounds
implementation-centric.

On Fri, Mar 14, 2025 at 03:43:15PM -0400, Andres Freund wrote:
> Attached is v2.8 with the following changes:

> - In parallel: Find a way to deal with the set_max_safe_fds() issue that we've
> been discussing on this thread recently. As that only affects io_uring, it
> doesn't have to block other patches going in.

As above, I like the "redefine" option.

> - Right now effective_io_concurrency cannot be set > 0 on Windows and other
> platforms that lack posix_fadvise. But with AIO we can read ahead without
> posix_fadvise().
>
> It'd not really make anything worse than today to not remove the limit, but
> it'd be pretty weird to prevent windows etc from benefiting from AIO. Need
> to look around and see whether it would require anything other than doc
> changes.

Worth changing, but non-blocking.

On Fri, Mar 14, 2025 at 03:58:43PM -0400, Andres Freund wrote:
> - Should the docs for debug_io_direct be rephrased and if so, how?

> Perhaps it's worth going from
>
> <para>
> Currently this feature reduces performance, and is intended for
> developer testing only.
> </para>
> to
> <para>
> Currently this feature reduces performance in many workloads, and is
> intended for testing only.
> </para>
>
> I.e. qualify the downside with "many workloads" and widen the audience ever so
> slightly?

Yes, that's good.

Other than the smgr patch review sent on its own thread, I've not yet reviewed
any of these patches comprehensively. Given the speed of change, I felt it
was time to flush comments buffered since 2025-03-11:

commit 0284401 wrote:
> aio: Basic subsystem initialization

> @@ -465,6 +466,7 @@ AutoVacLauncherMain(const void *startup_data, size_t startup_data_len)
> */
> LWLockReleaseAll();
> pgstat_report_wait_end();
> + pgaio_error_cleanup();

AutoVacLauncherMain(), BackgroundWriterMain(), CheckpointerMain(), and
WalWriterMain() call AtEOXact_Buffers() but not AtEOXact_Aio(). Is that
proper? They do call pgaio_error_cleanup() as seen here, so the only loss is
some asserts. (The load-bearing part does get done.)

commit da72269 wrote:
> aio: Add core asynchronous I/O infrastructure

> + * This could be in aio_internal.h, as it is not pubicly referenced, but

typo -> publicly

commit 55b454d wrote:
> aio: Infrastructure for io_method=worker

> + /* Try to launch one. */
> + child = StartChildProcess(B_IO_WORKER);
> + if (child != NULL)
> + {
> + io_worker_children[id] = child;
> + ++io_worker_count;
> + }
> + else
> + break; /* XXX try again soon? */

I'd change the comment to something like one of:

retry after DetermineSleepTime()
next LaunchMissingBackgroundProcesses() will retry in <60s

On Tue, Mar 18, 2025 at 04:12:18PM -0400, Andres Freund wrote:
> - Decide what to do about the smgr interrupt issue

Replied on that thread. It's essentially ready.

> Questions / Unresolved:
>
> - Write support isn't going to land in 18, but there is a tiny bit of code
> regarding writes in the code for bufmgr IO. I guess I could move that to a
> later commit?
>
> I'm inclined to leave it, the structure of the code only really makes
> knowing that it's going to be shared between reads & writes.

Fine to leave it.

> - pg_aios view name

Covered above.

> Subject: [PATCH v2.10 08/28] bufmgr: Implement AIO read support

Some comments about BM_IO_IN_PROGRESS may need updates. This paragraph:

* The BM_IO_IN_PROGRESS flag acts as a kind of lock, used to wait for I/O on a
buffer to complete (and in releases before 14, it was accompanied by a
per-buffer LWLock). The process doing a read or write sets the flag for the
duration, and processes that need to wait for it to be cleared sleep on a
condition variable.

And these individual lines from "git grep BM_IO_IN_PROGRESS":
* I/O already in progress. We already hold BM_IO_IN_PROGRESS for the
* only one process at a time can set the BM_IO_IN_PROGRESS bit.
* only one process at a time can set the BM_IO_IN_PROGRESS bit.
* i.e at most one BM_IO_IN_PROGRESS bit is set per proc.

The last especially. For the other three lines and the paragraph, the notion
of a process "holding" BM_IO_IN_PROGRESS or being the process to "set" it or
being the process "doing a read" becomes less significant when one process
starts the IO and another completes it.

> + /* we better have ensured the buffer is present until now */
> + Assert(BUF_STATE_GET_REFCOUNT(buf_state) >= 1);

I'd delete that comment; to me, the assertion alone is clearer.

> + ereport(LOG,
> + (errcode(ERRCODE_DATA_CORRUPTED),
> + errmsg("invalid page in block %u of relation %s; zeroing out page",

This is changing level s/WARNING/LOG/. That seems orthogonal to the patch's
goals; is it needed? If so, I recommend splitting it out as a preliminary
patch, to highlight the behavior change for release notes.

> +/*
> + * Perform completion handling of a single AIO read. This read may cover
> + * multiple blocks / buffers.
> + *
> + * Shared between shared and local buffers, to reduce code duplication.
> + */
> +static pg_attribute_always_inline PgAioResult
> +buffer_readv_complete(PgAioHandle *ioh, PgAioResult prior_result,
> + uint8 cb_data, bool is_temp)
> +{
> + PgAioResult result = prior_result;
> + PgAioTargetData *td = pgaio_io_get_target_data(ioh);
> + uint64 *io_data;
> + uint8 handle_data_len;
> +
> + if (is_temp)
> + {
> + Assert(td->smgr.is_temp);
> + Assert(pgaio_io_get_owner(ioh) == MyProcNumber);
> + }
> + else
> + Assert(!td->smgr.is_temp);
> +
> + /*
> + * Iterate over all the buffers affected by this IO and call appropriate
> + * per-buffer completion function for each buffer.
> + */
> + io_data = pgaio_io_get_handle_data(ioh, &handle_data_len);
> + for (uint8 buf_off = 0; buf_off < handle_data_len; buf_off++)
> + {
> + Buffer buf = io_data[buf_off];
> + PgAioResult buf_result;
> + bool failed;
> +
> + Assert(BufferIsValid(buf));
> +
> + /*
> + * If the entire failed on a lower-level, each buffer needs to be

Missing word, probably fix like:
s,entire failed on a lower-level,entire I/O failed on a lower level,

> + * marked as failed. In case of a partial read, some buffers may be
> + * ok.
> + */
> + failed =
> + prior_result.status == ARS_ERROR
> + || prior_result.result <= buf_off;

I didn't run an experiment to check the following, but I think this should be
s/<=/</. Suppose we requested two blocks and read some amount of bytes
[1*BLCKSZ, 2*BLSCKSZ - 1]. md_readv_complete will store result=1. buf_off==0
should compute failed=false here, but buf_off==1 should compute failed=true.

I see this relies on md_readv_complete having converted "result" to blocks.
Was there some win from doing that as opposed to doing the division here?
Division here ("blocks_read = prior_result.result / BLCKSZ") would feel easier
to follow, to me.

> +
> + buf_result = buffer_readv_complete_one(buf_off, buf, cb_data, failed,
> + is_temp);
> +
> + /*
> + * If there wasn't any prior error and the IO for this page failed in
> + * some form, set the whole IO's to the page's result.

s/the IO for this page/page verification/
s/IO's/IO's result/

> + */
> + if (result.status != ARS_ERROR && buf_result.status != ARS_OK)
> + {
> + result = buf_result;
> + pgaio_result_report(result, td, LOG);
> + }
> + }
> +
> + return result;
> +}