Hi Nitin, On Mon, 31 Aug 2026 at 18:20, Nitin Jadhav <[email protected]> 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
