Horton

Re: AIO v2.5

Noah Misch <noah@leadboat.com>

View thread
From: Noah Misch <noah@leadboat.com>
To: Andres Freund <andres@anarazel.de>
Cc: Antonin Houska <ah@cybertec.at>, pgsql-hackers@postgresql.org, Thomas Munro <thomas.munro@gmail.com>, Heikki Linnakangas <hlinnaka@iki.fi>, Robert Haas <robertmhaas@gmail.com>, Jakub Wartak <jakub.wartak@enterprisedb.com>, Jelte Fennema-Nio <postgres@jeltef.nl>
Date: 2025-03-23T15:55:29Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. aio: Fix assertion, clarify README

  2. aio: Fix reference to outdated name

  3. aio: Fix possible state confusions due to interrupt processing

  4. aio: Improve debug logging around waiting for IOs

  5. aio: Fix crash potential for pg_aios views due to late state update

  6. Increase BAS_BULKREAD based on effective_io_concurrency

  7. localbuf: Add Valgrind buffer access instrumentation

  8. aio: Make AIO more compatible with valgrind

  9. aio: Avoid spurious coverity warning

  10. tests: Fix incompatibility of test_aio with *_FORCE_RELEASE

  11. tests: Cope with WARNINGs during failed CREATE DB on windows

  12. aio: Add errcontext for processing I/Os for another backend

  13. aio: Add README.md explaining higher level design

  14. aio: Minor comment improvements

  15. aio: Add test_aio module

  16. aio: Add pg_aios view

  17. docs: Add acronym and glossary entries for I/O and AIO

  18. Enable IO concurrency on all systems

  19. read_stream: Introduce and use optional batchmode support

  20. docs: Reframe track_io_timing related docs as wait time

  21. bufmgr: Use AIO in StartReadBuffers()

  22. bufmgr: Implement AIO read support

  23. aio: Add WARNING result status

  24. Let caller of PageIsVerified() control ignore_checksum_failure

  25. pgstat: Allow checksum errors to be reported in critical sections

  26. Add errhint_internal()

  27. localbuf: Track pincount in BufferDesc as well

  28. aio, bufmgr: Comment fixes/improvements

  29. Fix mis-attribution of checksum failure stats to the wrong database

  30. aio: Implement support for reads in smgr/md/fd

  31. aio: Add io_method=io_uring

  32. aio: Add liburing dependency

  33. aio: Rename pgaio_io_prep_* to pgaio_io_start_*

  34. aio: Pass result of local callbacks to ->report_return

  35. aio: Be more paranoid about interrupts

  36. Redefine max_files_per_process to control additionally opened files

  37. aio: Change prefix of PgAioResultStatus values to PGAIO_RS_

  38. bufmgr: Improve stats when a buffer is read in concurrently

  39. aio: Add io_method=worker

  40. aio: Infrastructure for io_method=worker

  41. aio: Add core asynchronous I/O infrastructure

  42. aio: Basic subsystem initialization

  43. tests: Expand temp table tests to some pin related matters

  44. localbuf: Introduce FlushLocalBuffer()

  45. localbuf: Introduce TerminateLocalBufferIO()

  46. localbuf: Fix dangerous coding pattern in GetLocalVictimBuffer()

  47. localbuf: Introduce StartLocalBufferIO()

  48. localbuf: Introduce InvalidateLocalBuffer()

  49. Allow lwlocks to be disowned

  50. Make jsonb casts to scalar types translate JSON null to SQL NULL.

  51. bufmgr/smgr: Don't cross segment boundaries in StartReadBuffers()

  52. Use aux process resource owner in walsender

  53. bufmgr: Return early in ScheduleBufferTagForWriteback() if fsync=off

On Sun, Mar 23, 2025 at 11:11:53AM -0400, Andres Freund wrote:
> On 2025-03-22 17:20:56 -0700, Noah Misch wrote:
> > On Thu, Mar 20, 2025 at 09:58:37PM -0400, Andres Freund wrote:
> > >   Not sure yet how to best disable testing io_uring in this case. We can't
> > >   just query EXEC_BACKEND from pg_config.h unfortunately.  I guess making the
> > >   initdb not fail and checking the error log would work, but that doesn't work
> > >   nicely with Cluster.pm.
> > 
> > How about "postgres -c io_method=io_uring -C <anything>":
> > 
> > --- a/src/test/modules/test_aio/t/001_aio.pl
> > +++ b/src/test/modules/test_aio/t/001_aio.pl
> > @@ -29,7 +29,13 @@ $node_worker->stop();
> >  # Test io_method=io_uring
> >  ###
> >  
> > -if ($ENV{with_liburing} eq 'yes')
> > +sub have_io_uring
> > +{
> > +	local %ENV = $node_worker->_get_env();  # any node works
> > +	return run_log [qw(postgres -c io_method=io_uring -C io_method)];
> > +}
> > +
> > +if (have_io_uring())
> >  {
> >  	my $node_uring = create_node('io_uring');
> >  	$node_uring->start();
> 
> Yea, that's a good idea.
> 
> One thing that doesn't seem great is that it requires a prior node - what if
> we do -c io_method=invalid' that would report the list of valid GUC options,
> so we could just grep for io_uring?

Works for me.

> > One idea so far is to comment on valid states after some IoMethodOps
> > callbacks:

> I think these are a good idea. I added those to the copy-edit patch, with a
> few more tweaks:

The tweaks made it better.

> > > Subject: [PATCH v2.11 04/27] aio: Add liburing dependency

> > > +              [AC_DEFINE([USE_LIBURING], 1, [Define to build with io_uring support. (--with-liburing)])])
> > > +AC_MSG_RESULT([$with_liburing])
> > > +AC_SUBST(with_liburing)
> > >  
> > >  #
> > >  # UUID library
> > > @@ -1463,6 +1471,9 @@ elif test "$with_uuid" = ossp ; then
> > >  fi
> > >  AC_SUBST(UUID_LIBS)
> > >  
> > > +if test "$with_liburing" = yes; then
> > > +  PKG_CHECK_MODULES(LIBURING, liburing)
> > > +fi
> > 
> > We usually put this right after the AC_MSG_CHECKING ... AC_SUBST block.
> 
> We don't really seem to do that for "dependency checks" in general, e.g.
> PGAC_CHECK_PERL_CONFIGS, PGAC_CHECK_PYTHON_EMBED_SETUP, PGAC_CHECK_READLINE,
> dependency dependent AC_CHECK_LIB calls, .. later in configure.ac than the
> defnition of the option.

AC_CHECK_LIB stays far away, yes.

> But you're right that the PKG_CHECK_MODULES calls are closer-by. And I'm happy
> to move towards having the code for each dep all in one place, so moved.
> 
> 
> A related thing: We seem to have no order of the $with_ checks that I can
> discern. Should the liburing check be at a different place?

No opinion on that one.  It's fine.

> > This currently has unrelated stuff separating them.  Also, with the
> > exception of icu, we follow PKG_CHECK_MODULES uses by absorbing flags from
> > pkg-config and use AC_CHECK_LIB to add the actual "-l".
> 
> I think for liburing I was trying to follow ICU's example - injecting CFLAGS
> and LIBS just in the parts of the build dir that needs them.
> 
> For LIBS I think I did so:
> 
> diff --git a/src/backend/Makefile b/src/backend/Makefile
> ...
> +# The backend conditionally needs libraries that most executables don't need.
> +LIBS += $(LDAP_LIBS_BE) $(ICU_LIBS) $(LIBURING_LIBS)
> 
> But ugh, for some reason I didn't do that for LIBURING_CFLAGS. In the v1.x
> version of aio I had
> aio:src/backend/storage/aio/Makefile:override CPPFLAGS += $(LIBURING_CFLAGS)
> 
> but somehow lost that somewhere along the way to v2.x
> 
> 
> I think I like targetting where ${LIB}_LIBS and ${LIB}_CFLAGS are applied more
> narrowly better than just adding to the global CFLAGS, CPPFLAGS, LDFLAGS.

Agreed.

> somewhat inclined to add it LIBURING_CFLAGS in src/backend rather than
> src/backend/storage/aio/ though.
> 
> But I'm also willing to do it entirely differently.

The CPPFLAGS addition, located wherever makes sense, resolves that point.

> > > --- a/doc/src/sgml/installation.sgml
> > > +++ b/doc/src/sgml/installation.sgml
> > 
> > lz4 and other deps have a mention in <sect1 id="install-requirements">, in
> > addition to sections edited here.
> 
> Good point.
> 
> Although once more I feel defeated by the ordering used :)
> 
> Hm, that list is rather incomplete. At least libxml, libxslt, selinux, curl,
> uuid, systemd, selinux and bonjour aren't listed.
> 
> Not sure if it makes sense to add liburing, given that?

That's a lot of preexisting incompleteness.  I withdraw the point about <sect1
id="install-requirements">.


Unrelated to the above, another question about io_uring:

commit da722699 wrote:
> +/*
> + * Need to submit staged but not yet submitted IOs using the fd, otherwise
> + * the IO would end up targeting something bogus.
> + */
> +void
> +pgaio_closing_fd(int fd)

An IO in PGAIO_HS_STAGED clearly blocks closing the IO's FD, and an IO in
PGAIO_HS_COMPLETED_IO clearly doesn't block that close.  For io_method=worker,
closing in PGAIO_HS_SUBMITTED is okay.  For io_method=io_uring, is there a
reference about it being okay to close during PGAIO_HS_SUBMITTED?  I looked
awhile for an authoritative view on that, but I didn't find one.  If we can
rely on io_uring_submit() returning only after the kernel has given the
io_uring its own reference to all applicable file descriptors, I expect it's
okay to close the process's FD.  If the io_uring acquires its reference later
than that, I expect we shouldn't close before that later time.