Re: aio: Async fsyncs for crash recovery and checkpointer

Hi Nazir!

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.

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?

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.

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?

Best Regards,
Nitin Jadhav
Azure Database for PostgreSQL
Microsoft