Hi,
Thanks for your comments. Let me address them to the best of my ability.
> I believe this needs some more attention as this ring fd leak is going to
> cause harder outages in larger shops (single crash is going to cause leak,
> but then double allocation of the rings due to ulimit causing errno/EMFILE)
This is actually how we encountered the bug in the first place, except we were
hitting `ulimit -l` since our servers have a very high `ulimit -n`:
```
FATAL: could not setup io_uring queue: Cannot allocate memory
LOG: database system is shut down
```
> v2 LGTM to me, but does not apply due to AioShmemInit() taking some
> args now and there is this confusion (v2 preferred and less prefered v3),
> so maybe v4 should be sent to avoid confusion (v2 just rebased)?
I seem to have created some confusion with my versions, which I apologise
for. Let me try to correct that:
- v1: My initial naive implementation which adds the on_shmem_exit()
callback directly in pgaio_uring_shmem_init().
- v2: Adds the shmem_cleanup callback to IoMethodOps.
- v3: Rebases v2 because of the new API introduced in 58a1573.
So the rebase you ask for is v3, sent on the 17th of April. To settle the
numbering confusion I have attached v4, which I describe below.
> Could adding shmem_cleanup to IoMethodOps introduce an ABI break?
> If so, the v1 approach seems preferable, at least for a backpatch to v18?
Since aio_internal.h is installed as a server header and the new field is
added in the middle of the struct, v3 introduces an ABI break with PG 18
if backpatched.
As PG 18 is affected, I think this is worth backpatching. However, the new
shmem allocation functions aren't in PG 18, so the backpatch would need to
be v2-shaped and move `shmem_cleanup` to the end of the struct.
Since the backpatch needs a separate patch either way, master isn't constrained
here, so v4 keeps `shmem_cleanup` next to `shmem_callbacks`. I am happy to
send a backpatchable version if there is agreement on backpatching.
Alternatively, if maintaining a separate backpatch variant is undesirable,
v1 is still available. It doesn't change the `IoMethodOps` struct at all,
so the ABI question disappears.
> Just some nit: shouldn't we return / Assert() if not IsUnderPostmaster in
> pgaio_uring_shmem_cleanup()?
v4 adds an `Assert(!IsUnderPostmaster)` at the start of
`pgaio_uring_shmem_cleanup`.
> The CF entry is currently Ready for Committer, but the latest review
> prefers v2 while noting that it no longer applies after the
> AioShmemInit() change. It also leaves the IsUnderPostmaster check open.
>
> Could you post a single vNext with the intended design, rebased to the
> current API, and address that check? I have moved the entry to Waiting
> on Author for now.
v4 addresses all these items. I have moved the CF entry to "Needs review".
Thanks again for all your feedback.
Lucas.
From 204ce0e349395417ce5d5d910032f937e175ae98 Mon Sep 17 00:00:00 2001
From: Lucas DRAESCHER <[email protected]>
Date: Tue, 17 Mar 2026 17:26:11 +0100
Subject: [PATCH v4] Release io_uring resources on shmem exit
io_uring_queue_init() allocates resources for each io_uring
instance, but pgaio_uring_shmem_init() never registers a
cleanup callback to free them.
Add a shmem_cleanup callback to IoMethodOps registered in
AioShmemInit().
Implement the shmem_cleanup for method_io_uring.c as
pgaio_uring_shmem_cleanup() which calls
io_uring_queue_exit().
---
src/backend/storage/aio/aio_init.c | 18 ++++++++++++++++++
src/backend/storage/aio/method_io_uring.c | 20 ++++++++++++++++++++
src/include/storage/aio_internal.h | 6 ++++++
3 files changed, 44 insertions(+)
diff --git a/src/backend/storage/aio/aio_init.c b/src/backend/storage/aio/aio_init.c
index de50e6a8a31..60a8404204d 100644
--- a/src/backend/storage/aio/aio_init.c
+++ b/src/backend/storage/aio/aio_init.c
@@ -172,6 +172,20 @@ AioShmemRequest(void *arg)
pgaio_method_ops->shmem_callbacks.request_fn(pgaio_method_ops->shmem_callbacks.opaque_arg);
}
+/*
+ * Wrapper around pgaio_method_ops->shmem_cleanup to satisfy the
+ * on_shmem_exit() callback signature.
+ */
+static void
+pgaio_shmem_cleanup(int code, Datum arg)
+{
+ /*
+ * No null check needed here; AioShmemInit only registers this callback
+ * when shmem_cleanup is non-null.
+ */
+ pgaio_method_ops->shmem_cleanup();
+}
+
/*
* Initialize AIO shared memory during postmaster startup.
*/
@@ -225,6 +239,10 @@ AioShmemInit(void *arg)
if (pgaio_method_ops->shmem_callbacks.init_fn)
pgaio_method_ops->shmem_callbacks.init_fn(pgaio_method_ops->shmem_callbacks.opaque_arg);
+
+ /* Register callback to release any resources allocated above. */
+ if (pgaio_method_ops->shmem_cleanup)
+ on_shmem_exit(pgaio_shmem_cleanup, 0);
}
static void
diff --git a/src/backend/storage/aio/method_io_uring.c b/src/backend/storage/aio/method_io_uring.c
index 3ffe5061a20..63bb38a60eb 100644
--- a/src/backend/storage/aio/method_io_uring.c
+++ b/src/backend/storage/aio/method_io_uring.c
@@ -51,6 +51,7 @@
/* Entry points for IoMethodOps. */
static void pgaio_uring_shmem_request(void *arg);
static void pgaio_uring_shmem_init(void *arg);
+static void pgaio_uring_shmem_cleanup(void);
static void pgaio_uring_init_backend(void);
static int pgaio_uring_submit(uint16 num_staged_ios, PgAioHandle **staged_ios);
static void pgaio_uring_wait_one(PgAioHandle *ioh, uint64 ref_generation);
@@ -72,6 +73,7 @@ const IoMethodOps pgaio_uring_ops = {
.shmem_callbacks.request_fn = pgaio_uring_shmem_request,
.shmem_callbacks.init_fn = pgaio_uring_shmem_init,
+ .shmem_cleanup = pgaio_uring_shmem_cleanup,
.init_backend = pgaio_uring_init_backend,
.submit = pgaio_uring_submit,
@@ -403,6 +405,24 @@ pgaio_uring_shmem_init(void *arg)
}
}
+static void
+pgaio_uring_shmem_cleanup(void)
+{
+ Assert(!IsUnderPostmaster);
+
+ if (pgaio_uring_contexts != NULL)
+ {
+ int TotalProcs = pgaio_uring_procs();
+
+ elog(DEBUG1, "cleaning up %d io_uring processes", TotalProcs);
+
+ for (int i = 0; i < TotalProcs; i++)
+ io_uring_queue_exit(&pgaio_uring_contexts[i].io_uring_ring);
+
+ pgaio_uring_contexts = NULL;
+ }
+}
+
static void
pgaio_uring_init_backend(void)
{
diff --git a/src/include/storage/aio_internal.h b/src/include/storage/aio_internal.h
index 9ca4087aa7f..96a3f86ea97 100644
--- a/src/include/storage/aio_internal.h
+++ b/src/include/storage/aio_internal.h
@@ -272,6 +272,12 @@ typedef struct IoMethodOps
/* global initialization */
ShmemCallbacks shmem_callbacks;
+ /*
+ * Clean up shared memory resources before shutdown. Called during shmem
+ * exit. Optional.
+ */
+ void (*shmem_cleanup) (void);
+
/*
* Per-backend initialization. Optional.
*/
--
2.55.0
