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
