Date: Sun, 23 Mar 2025 08:55:29 -0700
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>
Subject: Re: AIO v2.5
Message-ID: <20250323155529.ab.nmisch@google.com>
References: <20250312035743.f5.nmisch@google.com>
 <w6uiicyou7hzq47mbyejubtcyb2rngkkf45fk4q7inue5kfbeo@bbfad3qyubvs>
 <4b3f32ug3cayekysqlgspz2qjmeb7lca3gvazayglxr2m3d4dv@il33accgsji7>
 <17906.1741863183@localhost>
 <3yxd5r23zly5bytvgyktbxtxq2r3gbpi7xd4dugevh3h4w4q6c@lu6oatjjpltz>
 <oav52ytakcvfnk52w5o5wvxdjavxth2tgptxt25gnfmh2pdcpk@omp3xukdu5if>
 <zp7obmaqdd6b5u2vhpw27hovpjirtqvjakkbkaz4vju7tohyul@4kfobe7ipgxm>
 <6ak556uyqiptdwjaci4kbi5eykwkmzqgkbtkyaosjnopjhncrc@2v4ac2jwyz22>
 <20250323002056.95.nmisch@google.com>
 <wd7zzn6kks7otnkkb3olekvaqfnm4meuh5yugn4ghkaiw5opph@zki247ivvrsx>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <wd7zzn6kks7otnkkb3olekvaqfnm4meuh5yugn4ghkaiw5opph@zki247ivvrsx>
User-Agent: Mutt/2.2.12 (2023-09-09)
Archived-At: 
 <https://www.postgresql.org/message-id/20250323155529.ab.nmisch%40google.com>
Precedence: bulk

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.