Re: AIO v2.5 - Mailing list pgsql-hackers

commit 247ce06b wrote:
> +            pgaio_io_reopen(ioh);
> +
> +            /*
> +             * To be able to exercise the reopen-fails path, allow injection
> +             * points to trigger a failure at this point.
> +             */
> +            pgaio_io_call_inj(ioh, "AIO_WORKER_AFTER_REOPEN");
> +
> +            error_errno = 0;
> +            error_ioh = NULL;
> +
> +            /*
> +             * We don't expect this to ever fail with ERROR or FATAL, no need
> +             * to keep error_ioh set to the IO.
> +             * pgaio_io_perform_synchronously() contains a critical section to
> +             * ensure we don't accidentally fail.
> +             */
> +            pgaio_io_perform_synchronously(ioh);

A CHECK_FOR_INTERRUPTS() could close() the FD that pgaio_io_reopen() callee
smgr_aio_reopen() stores.  Hence, I think smgrfd() should assert that
interrupts are held instead of doing its own HOLD_INTERRUPTS(), and a
HOLD_INTERRUPTS() should surround the above region of code.  It's likely hard
to reproduce a problem, because pgaio_io_call_inj() does nothing in many
builds, and pgaio_io_perform_synchronously() starts by entering a critical
section.

On Thu, Mar 20, 2025 at 09:58:37PM -0400, Andres Freund wrote:
> Attached v2.11

> Subject: [PATCH v2.11 06/27] aio: Implement support for reads in smgr/md/fd

> +int
> +FileStartReadV(PgAioHandle *ioh, File file,
> +               int iovcnt, off_t offset,
> +               uint32 wait_event_info)
> +{
> +    int            returnCode;
> +    Vfd           *vfdP;
> +
> +    Assert(FileIsValid(file));
> +
> +    DO_DB(elog(LOG, "FileStartReadV: %d (%s) " INT64_FORMAT " %d",
> +               file, VfdCache[file].fileName,
> +               (int64) offset,
> +               iovcnt));
> +
> +    returnCode = FileAccess(file);
> +    if (returnCode < 0)
> +        return returnCode;
> +
> +    vfdP = &VfdCache[file];
> +
> +    pgaio_io_prep_readv(ioh, vfdP->fd, iovcnt, offset);

FileStartReadV() and pgaio_io_prep_readv() advance the IO to PGAIO_HS_STAGED
w/ batch mode, PGAIO_HS_SUBMITTED w/o batch mode.  I didn't expect that from
functions so named.  The "start" verb sounds to me like unconditional
PGAIO_HS_SUBMITTED, and the "prep" verb sounds like PGAIO_HS_DEFINED.  I like
the "stage" verb, because it matches PGAIO_HS_STAGED, and the comment at
PGAIO_HS_STAGED succinctly covers what to expect.  Hence, I recommend names
FileStageReadV, pgaio_io_stage_readv, mdstagereadv, and smgrstageread.  How do
you see it?

> +/*
> + * AIO error reporting callback for mdstartreadv().
> + *
> + * Errors are encoded as follows:
> + * - PgAioResult.error_data != 0 encodes IO that failed with errno != 0

I recommend replacing "errno != 0" with either "that errno" or "errno ==
error_data".


> Subject: [PATCH v2.11 07/27] aio: Add README.md explaining higher level design

Ready for commit apart from some trivia:

> +if (ioret.result.status == PGAIO_RS_ERROR)
> +    pgaio_result_report(aio_ret.result, &aio_ret.target_data, ERROR);

I think ioret and aio_ret are supposed to be the same object.  If that's
right, change one of the names.  Likewise elsewhere in this file.

> +The central API piece for postgres' AIO abstraction are AIO handles. To
> +execute an IO one first has to acquire an IO handle (`pgaio_io_acquire()`) and
> +then "defined", i.e. associate an IO operation with the handle.

s/"defined"/"define" it/ or similar

> +The "solution" to this the ability to associate multiple completion callbacks

s/this the/this is the/


> Subject: [PATCH v2.11 08/27] localbuf: Track pincount in BufferDesc as well

> @@ -5350,6 +5350,18 @@ ConditionalLockBufferForCleanup(Buffer buffer)
>          Assert(refcount > 0);
>          if (refcount != 1)
>              return false;
> +
> +        /*
> +         * Check that the AIO subsystem doesn't have a pin. Likely not
> +         * possible today, but better safe than sorry.
> +         */
> +        bufHdr = GetLocalBufferDescriptor(-buffer - 1);
> +        buf_state = pg_atomic_read_u32(&bufHdr->state);
> +        refcount = BUF_STATE_GET_REFCOUNT(buf_state);
> +        Assert(refcount > 0);
> +        if (refcount != 1)
> +            return false;
> +

LockBufferForCleanup() should get code like this
ConditionalLockBufferForCleanup() code, either now or when "not possible
today" ends.  Currently, it just assumes all local buffers are
cleanup-lockable:

    /* Nobody else to wait for */
    if (BufferIsLocal(buffer))
        return;

> @@ -570,7 +577,13 @@ InvalidateLocalBuffer(BufferDesc *bufHdr, bool check_unreferenced)
>  
>      buf_state = pg_atomic_read_u32(&bufHdr->state);
>  
> -    if (check_unreferenced && LocalRefCount[bufid] != 0)
> +    /*
> +     * We need to test not just LocalRefCount[bufid] but also the BufferDesc
> +     * itself, as the latter is used to represent a pin by the AIO subsystem.
> +     * This can happen if AIO is initiated and then the query errors out.
> +     */
> +    if (check_unreferenced &&
> +        (LocalRefCount[bufid] != 0 || BUF_STATE_GET_REFCOUNT(buf_state) != 0))
>          elog(ERROR, "block %u of %s is still referenced (local %u)",

I didn't write a test to prove it, but I'm suspecting we'll reach the above
ERROR with this sequence:

  CREATE TEMP TABLE foo ...;
  [some command that starts reading a block of foo into local buffers, then ERROR with IO ongoing]
  DROP TABLE foo;

DropRelationAllLocalBuffers() calls InvalidateLocalBuffer(bufHdr, true).  I
think we'd need to do like pgaio_shutdown() and finish all IOs (or all IOs for
the particular rel) before InvalidateLocalBuffer().  Or use something like the
logic near elog(ERROR, "buffer is pinned in InvalidateBuffer") in
corresponding bufmgr code.  I think that bufmgr ERROR is unreachable, since
only a private refcnt triggers that bufmgr ERROR.  Is there something
preventing the localbuf error from being a problem?  (This wouldn't require
changes to the current patch; responsibility would fall in a bufmgr AIO
patch.)


> Subject: [PATCH v2.11 09/27] bufmgr: Implement AIO read support

(Still reviewing this and later patches, but incidental observations follow.)

> +buffer_readv_complete_one(uint8 buf_off, Buffer buffer, uint8 flags,
> +                          bool failed, bool is_temp)
> +{
...
> +    PgAioResult result;
...
> +    result.status = PGAIO_RS_OK;
...
> +    return result;

gcc 14.2.0 -Werror gives me:

  bufmgr.c:7297:16: error: ‘result’ may be used uninitialized [-Werror=maybe-uninitialized]

Zeroing the unset fields silenced it:

--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -7221,3 +7221,3 @@ buffer_readv_complete_one(uint8 buf_off, Buffer buffer, uint8 flags,
     char       *bufdata = BufferGetBlock(buffer);
-    PgAioResult result;
+    PgAioResult result = { .status = PGAIO_RS_OK };
     uint32        set_flag_bits;
@@ -7238,4 +7238,2 @@ buffer_readv_complete_one(uint8 buf_off, Buffer buffer, uint8 flags,
 
-    result.status = PGAIO_RS_OK;
-
     /* check for garbage data */


> Subject: [PATCH v2.11 13/27] aio: Basic read_stream adjustments for real AIO

> @@ -416,6 +418,13 @@ read_stream_start_pending_read(ReadStream *stream)
>  static void
>  read_stream_look_ahead(ReadStream *stream)
>  {
> +    /*
> +     * Allow amortizing the cost of submitting IO over multiple IOs. This
> +     * requires that we don't do any operations that could lead to a deadlock
> +     * with staged-but-unsubmitted IO.
> +     */
> +    pgaio_enter_batchmode();

We call read_stream_get_block() while in batchmode, so the stream callback
needs to be ready for that.  A complicated case is
collect_corrupt_items_read_stream_next_block(), which may do its own buffer
I/O to read in a vmbuffer for VM_ALL_FROZEN().  That's feeling to me like a
recipe for corner cases reaching ERROR "starting batch while batch already in
progress".  Are there mitigating factors?


> Subject: [PATCH v2.11 17/27] aio: Add test_aio module

> +    # verify that page verification errors are detected even as part of a
> +    # shortened multi-block read (tbl_corr, block 1 is tbl_corred)

Is "tbl_corred" a typo of something?

> --- /dev/null
> +++ b/src/test/modules/test_aio/test_aio.c
> @@ -0,0 +1,657 @@
> +/*-------------------------------------------------------------------------
> + *
> + * delay_execution.c
> + *        Test module to allow delay between parsing and execution of a query.
> + *
> + * The delay is implemented by taking and immediately releasing a specified
> + * advisory lock.  If another process has previously taken that lock, the
> + * current process will be blocked until the lock is released; otherwise,
> + * there's no effect.  This allows an isolationtester script to reliably
> + * test behaviors where some specified action happens in another backend
> + * between parsing and execution of any desired query.
> + *
> + * Copyright (c) 2020-2025, PostgreSQL Global Development Group
> + *
> + * IDENTIFICATION
> + *      src/test/modules/delay_execution/delay_execution.c

Header comment is surviving from copy-paste of delay_execution.c.

> +     * Tor tests we don't want the resowner release preventing us from

s/Tor/For/