Hi Nazir, Thanks for the updated patch set. I reviewed v2. The changes address my earlier comments about fsync timing and reopen-error handling. In particular, wait time is now accounted for when the issuer actually blocks for completion, and ordinary reopen failures preserve the errno without terminating the I/O worker.
I found two further points, and the worker-side SLRU test-coverage question from v1 still appears applicable. An I/O worker can skip a required fsync after fsync is enabled. As Yuhang noted, there appears to be a race when reloading fsync from off to on. pgaio_io_start_fsync() makes the dispatch decision using the issuing process's enableFsync. Therefore, once the checkpointer has processed the new configuration and sees fsync=on, it can submit the operation to an I/O worker. However, worker execution eventually calls pg_fsync() or pg_fdatasync(). Those functions check the worker process's own enableFsync. An I/O worker can consume and execute a queued request before processing its pending configuration reload. If the worker still has the old fsync=off value, pg_fsync() returns success without issuing the syscall. The checkpointer then treats the operation as successfully completed even though the required synchronization did not occur. The unsafe direction is specifically off to on; the reverse direction can cause an unnecessary fsync but does not lose durability. Could the decision to perform the fsync be captured in the AIO operation when it is submitted, with worker execution using a syscall helper that does not re-evaluate the worker-local enableFsync value? Alternatively, the worker would need to guarantee that it has applied the relevant configuration generation before executing the request. I think this should be treated as blocking because it can allow a checkpoint to complete without executing a required fsync. Worker SMGR cleanup appears dependent on the worker becoming idle. Patch 0002 calls smgrdestroyall() only after the worker finds that no request is available. This means a continuously busy worker may never perform the cleanup. Worker-side relation reopening calls smgropen(), and those unpinned SMgrRelation objects remain in the worker's SMGR hash until smgrdestroyall() is called. With sustained I/O over many distinct relations, a worker whose queue never becomes empty could therefore retain an increasing number of SMGR entries, including entries for relations that have since been dropped. Would it be safer to check FirstCallSinceLastCheckpoint() at a safe point after completing each request, before consuming the next request, rather than only on the idle path? At that point any descriptor reopened for the completed operation has already been released. The worker-side SLRU path does not appear to have targeted test coverage. The test_slru_page_sync() still registers the test SLRU with SYNC_HANDLER_NONE. Consequently, SlruSyncFileTag() selects PGAIO_TID_SYNC, which has no reopen callback and is executed synchronously in the submitting process under io_method=worker. Best Regards, Nitin Jadhav Azure Database for PostgreSQL Microsoft
