Re: aio: Async fsyncs for crash recovery and checkpointer

Hi Nitin,

On Mon, 31 Aug 2026 at 18:20, Nitin Jadhav
<nitinjadhavpostgres(at)gmail(dot)com> wrote:
>
> Thanks for working on this. The overall direction looks promising and
> the patch splits are easy to follow. I reviewed the v1 series and
> found a few issues/questions.

Thank you for looking into this!

> pg_stat_io.fsync_time no longer measures fsync wait time. In
> mdsyncfiletag(), the time attributed to IOOP_FSYNC is measured around
> FileStartSync(). With io_method=worker or io_method=io_uring,
> FileStartSync() generally returns after submitting the operation,
> before the fsync has completed. As a result, pg_stat_io.fsync_time
> records mostly submission overhead rather than time spent waiting for
> the fsync. This appears inconsistent with the documented meaning of
> fsync_time, which is "time spent waiting for fsync operations." It
> also makes the statistic method-dependent - synchronous execution
> includes the fsync itself, while worker and io_uring execution
> generally do not. A synchronous fallback under the worker method would
> change the semantics. Should the timing instead be accounted when
> completion is observed or the request is reaped? Alternatively, if
> asynchronous fsyncs require different accounting semantics, should
> those semantics be explicitly defined and documented?

Yes, you are right. We can copy the timing calculation used for async
read I/Os. So, it means that total time of fsyncs will be submission
of I/O + wait time for pgaio_wref_wait() in the drain() functions. If
we do that, we don't need to change documentation because this will
represent the total "time spent waiting for fsync operations."
regardless of io_method.

> SLRU reopen errors terminate I/O workers and lose the original errno.
> sync_aio_reopen() raises ERROR if an I/O worker cannot reopen an
> SLRU segment. The worker’s exception-recovery path completes every
> reopen failure as ENOENT. An ENOENT here can be an expected race -
> the SLRU segment may have been truncated after the fsync was submitted
> but before the worker reopened it. ProcessSyncRequests() already has
> cancellation and FILE_POSSIBLY_DELETED() handling for this case.
> However, before that logic sees the result, the I/O worker has emitted
> an error and exited. Repeated SLRU truncation concurrent with
> checkpoints could therefore cause unnecessary logging and worker
> churn. There is also an errno-preservation issue. Failures such as
> EACCES, EMFILE, or EIO are delivered to the issuing process as ENOENT.
> The worker’s initial error report may contain the original %m , but
> the AIO completion result does not. The checkpointer may consequently
> treat the first failure as a possible deletion race and retry it, then
> eventually report a misleading “No such file or directory” error.
> Could the reopen interface return an errno for ordinary open failures
> instead of raising ERROR ? The worker could complete the request with
> -errno and continue processing. That would preserve the actual error,
> allow the existing cancellation/retry logic to handle ENOENT, and
> reserve worker termination for genuinely unexpected failures.

Yes, this is a valid problem. Thank you for the detailed explanation.
I will change the reopen interface so that ordinary failures return
-errno. The I/O worker can then complete the request and continue
processing, while errors raised for unexpected failures will retain
the existing worker-termination path.

> The new worker-side SLRU target does not appear to be covered by the
> test. The updated test_slru_page_sync() test uses ".sync_handler =
> SYNC_HANDLER_NONE". That causes SlruSyncFileTag() to select
> PGAIO_TID_SYNC rather than PGAIO_TID_SYNC_FILETAG. Since
> PGAIO_TID_SYNC has no reopen callback, io_method=worker executes
> this request synchronously in the submitting process instead of
> handing it to an I/O worker. The test therefore covers the refactored
> local fsync path, but not the main path introduced by patch 0004.
> Could we add a TAP test to cover this?

You are right. However, I think what needs to be done is having a
general AIO fsync tests (which will cover the path you mentioned too)
in the test_aio test suite. I will try to implement it in the v2.

--
Regards,
Nazir Bilal Yavuz
Microsoft