Re: aio: Async fsyncs for crash recovery and checkpointer

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