From 7e95a3ef2f67d2e3312e7a5898bfa57a7716c8f2 Mon Sep 17 00:00:00 2001 From: Nazir Bilal Yavuz Date: Wed, 16 Sep 2026 10:53:05 +0300 Subject: [PATCH v2 5/8] aio: Allow AIO reopen callbacks to return errors Allow target reopen callbacks to return -errno for ordinary failures. IO workers pass these results through completion so the issuer can apply its error policy. Exceptions retain the existing worker recovery path, and relation read/write descriptor lookup keeps its existing error reporting. This prepares for asynchronous checkpoint fsyncs, where the issuer needs to distinguish potentially deleted files from other failures when deciding whether to retry or report a sync failure. Discussion: https://postgr.es/m/CAN55FZ0vLWJQNB%3DHuHXG2wabFjXJd6OWTa3%3DkRzwObdZD9poHQ%40mail.gmail.com --- src/backend/storage/aio/aio_target.c | 8 +- src/backend/storage/aio/method_worker.c | 99 ++++++++++++++----------- src/backend/storage/smgr/smgr.c | 20 +++-- src/include/storage/aio.h | 7 +- src/include/storage/aio_internal.h | 2 +- 5 files changed, 81 insertions(+), 55 deletions(-) diff --git a/src/backend/storage/aio/aio_target.c b/src/backend/storage/aio/aio_target.c index 82b24b0026d..1a0088fa382 100644 --- a/src/backend/storage/aio/aio_target.c +++ b/src/backend/storage/aio/aio_target.c @@ -136,11 +136,15 @@ pgaio_io_can_reopen(PgAioHandle *ioh) * IO has been staged in, the file descriptor has to be reopened - any FD * referenced in the IO itself, won't be valid in the separate process. */ -void +int pgaio_io_reopen(PgAioHandle *ioh) { + int result; + Assert(ioh->target > PGAIO_TID_INVALID && ioh->target < PGAIO_TID_COUNT); Assert(ioh->op > PGAIO_OP_INVALID && ioh->op < PGAIO_OP_COUNT); - pgaio_target_info[ioh->target]->reopen(ioh); + result = pgaio_target_info[ioh->target]->reopen(ioh); + + return result; } diff --git a/src/backend/storage/aio/method_worker.c b/src/backend/storage/aio/method_worker.c index 7639d093677..746d1be20e4 100644 --- a/src/backend/storage/aio/method_worker.c +++ b/src/backend/storage/aio/method_worker.c @@ -872,6 +872,7 @@ IoWorkerMain(const void *startup_data, size_t startup_data_len) if (io_index != -1) { PgAioHandle *ioh = NULL; + int reopen_result; /* Cancel timeout and update wakeup:work ratio. */ idle_timeout_abs = 0; @@ -897,62 +898,72 @@ IoWorkerMain(const void *startup_data, size_t startup_data_len) HOLD_INTERRUPTS(); /* - * It's very unlikely, but possible, that reopen fails. E.g. due - * to memory allocations failing or file permissions changing or - * such. In that case we need to fail the IO. - * - * There's not really a good errno we can report here. + * Ordinary reopen failures can return -errno below. If the + * callback instead raises an error, use the existing worker-exit + * recovery path, which has no reliable errno to report. */ error_errno = ENOENT; - pgaio_io_reopen(ioh); + reopen_result = pgaio_io_reopen(ioh); - /* - * To be able to exercise the reopen-fails path, allow injection - * points to trigger a failure at this point. - */ - INJECTION_POINT("aio-worker-after-reopen", ioh); + if (reopen_result < 0) + { + error_errno = 0; + error_ioh = NULL; + + START_CRIT_SECTION(); + pgaio_io_process_completion(ioh, reopen_result); + END_CRIT_SECTION(); + } + else + { + /* + * To be able to exercise the reopen-fails path, allow + * injection points to trigger a failure at this point. + */ + INJECTION_POINT("aio-worker-after-reopen", ioh); - error_errno = 0; - error_ioh = NULL; + error_errno = 0; + error_ioh = NULL; - /* - * As part of IO completion the buffer will be marked as NOACCESS, - * until the buffer is pinned again - which never happens in io - * workers. Therefore the next time there is IO for the same - * buffer, the memory will be considered inaccessible. To avoid - * that, explicitly allow access to the memory before reading data - * into it. - */ + /* + * As part of IO completion the buffer will be marked as + * NOACCESS, until the buffer is pinned again - which never + * happens in io workers. Therefore the next time there is IO + * for the same buffer, the memory will be considered + * inaccessible. To avoid that, explicitly allow access to the + * memory before reading data into it. + */ #ifdef USE_VALGRIND - { - struct iovec *iov; - uint16 iov_length = pgaio_io_get_iovec_length(ioh, &iov); + { + struct iovec *iov; + uint16 iov_length = pgaio_io_get_iovec_length(ioh, &iov); - for (int i = 0; i < iov_length; i++) - VALGRIND_MAKE_MEM_UNDEFINED(iov[i].iov_base, iov[i].iov_len); - } + for (int i = 0; i < iov_length; i++) + VALGRIND_MAKE_MEM_UNDEFINED(iov[i].iov_base, iov[i].iov_len); + } #endif #ifdef PGAIO_WORKER_SHOW_PS_INFO - { - char *description = pgaio_io_get_target_description(ioh); - - sprintf(cmd, "%d: [%s] %s", - MyIoWorkerId, - pgaio_io_get_op_name(ioh), - description); - pfree(description); - set_ps_display(cmd); - } + { + char *description = pgaio_io_get_target_description(ioh); + + sprintf(cmd, "%d: [%s] %s", + MyIoWorkerId, + pgaio_io_get_op_name(ioh), + description); + pfree(description); + set_ps_display(cmd); + } #endif - /* - * We don't expect this to ever fail with ERROR or FATAL, no need - * to keep error_ioh set to the IO. - * pgaio_io_perform_synchronously() contains a critical section to - * ensure we don't accidentally fail. - */ - pgaio_io_perform_synchronously(ioh); + /* + * We don't expect this to ever fail with ERROR or FATAL, no + * need to keep error_ioh set to the IO. + * pgaio_io_perform_synchronously() contains a critical + * section to ensure we don't accidentally fail. + */ + pgaio_io_perform_synchronously(ioh); + } RESUME_INTERRUPTS(); errcallback.arg = NULL; diff --git a/src/backend/storage/smgr/smgr.c b/src/backend/storage/smgr/smgr.c index 69e61ea1661..83417e1bcc2 100644 --- a/src/backend/storage/smgr/smgr.c +++ b/src/backend/storage/smgr/smgr.c @@ -165,7 +165,7 @@ static dlist_head unpinned_relns; static void smgrshutdown(int code, Datum arg); static void smgrdestroy(SMgrRelation reln); -static void smgr_aio_reopen(PgAioHandle *ioh); +static int smgr_aio_reopen(PgAioHandle *ioh); static char *smgr_aio_describe_identity(const PgAioTargetData *sd); @@ -1058,9 +1058,11 @@ pgaio_io_set_target_smgr(PgAioHandle *ioh, /* * Callback for the smgr AIO target, to reopen the file (e.g. because the IO - * is executed in a worker). + * is executed in a worker). Returns 0 on success. Failures can return + * -errno, but relation read/write descriptor lookup retains its existing + * error reporting and may raise an error instead. */ -static void +static int smgr_aio_reopen(PgAioHandle *ioh) { PgAioTargetData *sd = pgaio_io_get_target_data(ioh); @@ -1068,6 +1070,7 @@ smgr_aio_reopen(PgAioHandle *ioh) SMgrRelation reln; ProcNumber procno; uint32 off; + int fd; /* * The caller needs to prevent interrupts from being processed, otherwise @@ -1089,15 +1092,18 @@ smgr_aio_reopen(PgAioHandle *ioh) case PGAIO_OP_READV: od->read.fd = smgrfd(reln, sd->smgr.forkNum, sd->smgr.blockNum, &off); Assert(off == od->read.offset); - break; + return 0; case PGAIO_OP_WRITEV: od->write.fd = smgrfd(reln, sd->smgr.forkNum, sd->smgr.blockNum, &off); Assert(off == od->write.offset); - break; + return 0; case PGAIO_OP_FSYNC: - od->fsync.fd = smgrfd(reln, sd->smgr.forkNum, sd->smgr.blockNum, &off); - break; + fd = smgrfd(reln, sd->smgr.forkNum, sd->smgr.blockNum, &off); + od->fsync.fd = fd; + return 0; } + + pg_unreachable(); } /* diff --git a/src/include/storage/aio.h b/src/include/storage/aio.h index 428504c0472..7f056f43e78 100644 --- a/src/include/storage/aio.h +++ b/src/include/storage/aio.h @@ -168,8 +168,13 @@ struct PgAioTargetInfo /* * To support executing using worker processes, the file descriptor for an * IO may need to be reopened in a different process. + * + * Returns 0 on success. Callbacks can return -errno for an ordinary + * failure to reopen, allowing the issuer to handle the failure without + * terminating the worker. Callbacks that raise an error instead use the + * worker's exception-recovery path, which does not preserve errno. */ - void (*reopen) (PgAioHandle *ioh); + int (*reopen) (PgAioHandle *ioh); /* describe the target of the IO, used for log messages and views */ char *(*describe_identity) (const PgAioTargetData *sd); diff --git a/src/include/storage/aio_internal.h b/src/include/storage/aio_internal.h index 9ca4087aa7f..35d4f3f310d 100644 --- a/src/include/storage/aio_internal.h +++ b/src/include/storage/aio_internal.h @@ -359,7 +359,7 @@ extern int pgaio_io_get_iovec_length(PgAioHandle *ioh, struct iovec **iov); /* aio_target.c */ extern bool pgaio_io_can_reopen(PgAioHandle *ioh); -extern void pgaio_io_reopen(PgAioHandle *ioh); +extern int pgaio_io_reopen(PgAioHandle *ioh); extern const char *pgaio_io_get_target_name(PgAioHandle *ioh); -- 2.47.3