Re: AIO v2.5

Hi,

I've pushed fixes for 1) and 2) and am working on 3).

On 2025-04-01 17:13:24 -0700, Noah Misch wrote:
> On Tue, Apr 01, 2025 at 06:25:28PM -0400, Andres Freund wrote:
> > On 2025-04-01 17:47:51 -0400, Andres Freund wrote:
> > > 3) Some subtests fail if RELCACHE_FORCE_RELEASE and CATCACHE_FORCE_RELEASE are defined:
> > >
> > > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=prion&dt=2025-04-01%2019%3A23%3A07
> > >
> > > # +++ tap check in src/test/modules/test_aio +++
> > >
> > > # Failed test 'worker: batch_start() leak & cleanup in implicit xact: expected stderr'
> > > # at t/001_aio.pl line 318.
> > > # 'psql:<stdin>:4: ERROR: starting batch while batch already in progress'
> > > # doesn't match '(?^:open AIO batch at end)'
> > >
> > >
> > > The problem is basically that the test intentionally forgets to exit batchmode
> > > - normally that would trigger an error at the end of the transaction, which
> > > the test verifies. However, with RELCACHE_FORCE_RELEASE and
> > > CATCACHE_FORCE_RELEASE defined, we get other code entering batchmode and
> > > erroring out because batchmode isn't allowed to be entered recursively.
>
> > > I don't really have a good idea how to deal with that yet.
> >
> > Hm. Making the query something like
> >
> > SELECT * FROM (VALUES (NULL), (batch_start()));
> >
> > avoids the wrong output, because the type lookup happens for the first row
> > already. But that's pretty magical and probably fragile.
>
> Hmm. Some options:
>
> a. VALUES() trick above. For test code, it's hard to argue with something
> that seems to solve it in practice.

I think I'll go for a slightly nicer version of that, namely
SELECT WHERE batch_start() IS NULL
I think that ends up the least verbose of the ideas we've been discussing.

> c. Move RELCACHE_FORCE_RELEASE and CATCACHE_FORCE_RELEASE to be
> GUC-controlled, like how CLOBBER_CACHE_ALWAYS changed into the
> debug_discard_caches GUC. Then disable them for relevant parts of
> test_aio. This feels best long-term, but it's bigger. I also wanted this
> in syscache-update-pruned.spec[1].

Yea, that'd probably be a good thing medium-term.

Greetings,

Andres Freund