Re: AIO v2.5

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.

b. Encapsulate the test in a PROCEDURE, so perhaps less happens between the
batch_start() and the procedure-managed COMMIT. Maybe less fragile than
(a), maybe more fragile.

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].

d. Have test_aio deduce whether these are set, probably by observing memory
contexts or DEBUG messages. Maybe have every postmaster startup print a
DEBUG message about these settings being enabled. Skip relevant parts of
test_aio. This sounds messy.

Each of those feels defensible to me. I'd probably do (a) or (b) to start.

[1] For that spec, an alternative expected output sufficed. Incidentally,
I'll soon fix that spec flaking on valgrind/skink.