aio: worker: Free SMGR objects when idle

Hi Nazir,

Thanks for the patch. This is the same problem as BUG #19622, and
Alexandre has a sinval based patch for it in [1]/messages/by-id/CAE8JnxOivqBrwGz8jwnKnXJ5tBe6EU=71P31MBStY37MDZ=jng@mail.gmail.com.

I read v1, the placement looks good to me.

For the never idle case you named, the check could go after the
if/else, next to CHECK_FOR_INTERRUPTS(). Every iteration passes there,
and the SMGRRELEASE barrier already calls smgrreleaseall() at that
point, so it is a safe place. It only matters for a worker that sees
no empty queue for a whole checkpoint cycle, so it is a small thing.

For 18 the idle branch looks different, so the backpatch needs its own
version.

[1]: /messages/by-id/CAE8JnxOivqBrwGz8jwnKnXJ5tBe6EU=71P31MBStY37MDZ=jng@mail.gmail.com
/messages/by-id/CAE8JnxOivqBrwGz8jwnKnXJ5tBe6EU=71P31MBStY37MDZ=jng@mail.gmail.com

Thanks,
Shihao

Hi,

On 2026-09-21 10:25:12 -0400, shihao zhong wrote:

Thanks for the patch. This is the same problem as BUG #19622, and
Alexandre has a sinval based patch for it in [1].

I read v1, the placement looks good to me.

For the never idle case you named, the check could go after the
if/else, next to CHECK_FOR_INTERRUPTS().

I'm doubtful that that'd be a good idea. It'd add a lot of contention on the
CheckpointerShmem->ckpt_lck that's acquired as part of
FirstCallSinceLastCheckpoint. On a system with a lot of IO that'd probably
noticeable.

I don't think any approach that adds an acuisition of a central lock around
every IO is going to make sense here.

I think Alexandre's patch has the same issue, unfortunately.

I'm sure we could make FirstCallSinceLastCheckpoint() not require a
spinlock. But I wonder if that's quite the right design. Doing work for every
single iteration of various loops (bgwriter, io worker) doesn't really seem
right to me for something that's as rare as this.

Having aux processes participate in sinval doesn't really clearly seem like
the right thing either. There are a lot of messages that they never benefit
from. And manual filtering in the receive function like Alexandre added
doesn't really seem like a good way of addressing that.

Maybe we should just drop having SHAREDINVALSMGR_ID and only use
PROCSIGNAL_BARRIER_SMGRRELEASE?

Greetings,

Andres Freund

On Mon, Sep 21, 2026 at 4:09 PM Andres Freund <andres@anarazel.de> wrote:

I'm doubtful that that'd be a good idea. It'd add a lot of contention on the

CheckpointerShmem->ckpt_lck that's acquired as part of
FirstCallSinceLastCheckpoint. On a system with a lot of IO that'd probably
noticeable.

I always look forward to your comments Andres.
I don't see a reason to have a lock in FirstCallSinceLastCheckpoint()

diff --git a/src/backend/postmaster/checkpointer.c
b/src/backend/postmaster/checkpointer.c
index 580c7944119..c91c0350e78 100644
--- a/src/backend/postmaster/checkpointer.c
+++ b/src/backend/postmaster/checkpointer.c
@@ -1517,9 +1517,7 @@ FirstCallSinceLastCheckpoint(void)
        int                     new_done;
        bool            FirstCall = false;

- SpinLockAcquire(&CheckpointerShmem->ckpt_lck);
new_done = CheckpointerShmem->ckpt_done;
- SpinLockRelease(&CheckpointerShmem->ckpt_lck);

if (new_done != ckpt_done)
FirstCall = true;

I don't think any approach that adds an acuisition of a central lock around
every IO is going to make sense here.

I think Alexandre's patch has the same issue, unfortunately.

Unfortunately, yes. I didn't notice how complex ReceiveSharedInvalidMessages
is, SIGetDataEntries is hiding some LWLocks.

I'm sure we could make FirstCallSinceLastCheckpoint() not require a

spinlock.

race condition hehe

But I wonder if that's quite the right design. Doing work for every
single iteration of various loops (bgwriter, io worker) doesn't really seem
right to me for something that's as rare as this.

Well, from my previous read of Nazir's patch he will check only when the
worker
goes idle, at most once per checkpoint.

Having aux processes participate in sinval doesn't really clearly seem like
the right thing either. There are a lot of messages that they never benefit
from. And manual filtering in the receive function like Alexandre added
doesn't really seem like a good way of addressing that.

Noted

Maybe we should just drop having SHAREDINVALSMGR_ID and only use
PROCSIGNAL_BARRIER_SMGRRELEASE?

Closing all the files because one was dropped doesn't seem right either.

bool
ProcessBarrierSmgrRelease(void)
{
smgrreleaseall();
return true;
}

And, apparently, that is prone to the same issue as shihao pointed out
earlier.
smgrreleaseall() will turn a file leak into a slow memory memory leak.

Regards,
Alexandre

Is it a trending topic, or is it just that I didn't pay attention to this
before?

Another instance of stale file handles:
/messages/by-id/CALj2ACVwDuOXXDjj2cVdnTKoxsgTSLDin4XoL63AnM6aUgMQaA@mail.gmail.com

Hi,

Right, the per IO check puts the spinlock in the hot path.

On dropping SHAREDINVALSMGR_ID, one thing to check first. The barrier
is only emitted by DROP and ALTER DATABASE and by DROP TABLESPACE.
Plain DROP TABLE and TRUNCATE go through CacheInvalidateSmgr, from
smgrdounlinkall and smgrtruncate. With only the barrier left, a
backend would hold descriptors for dropped relations until someone
drops a database or a tablespace. That is a behavior change, not a
cleanup.

The barrier also only releases, it does not destroy, so it does not
fix what Nazir reported. The hash table in the IO worker keeps
growing either way. Nothing pins those entries in an IO worker, and
smgrdestroyall only zaps unpinned ones, so destroying them there
should be safe.

That suggests a version with no lock at all. Keep the cleanup where
Nazir put it, at a safe point in the worker loop, but trigger it on a
local condition, the number of unpinned entries being over a cap,
rather than on a checkpoint generation. No shared state, and it also
covers a worker that never goes idle.

What this does not answer is how an IO worker finds out that a
relation was unlinked, so it can close the descriptor without waiting
for its own eviction. That is the sinval question you raised, and it
looks separate from Nazir's patch to me.

Thanks,
Shihao

+Bharath, +Sami
Maybe this discussion is relevant for the
WAL segment file descriptor leak issue.

On Tue, Sep 22, 2026 at 1:12 AM shihao zhong <zhong950419@gmail.com> wrote:

Hi,

Right, the per IO check puts the spinlock in the hot path.

On dropping SHAREDINVALSMGR_ID, one thing to check first. The barrier
is only emitted by DROP and ALTER DATABASE and by DROP TABLESPACE.
Plain DROP TABLE and TRUNCATE go through CacheInvalidateSmgr, from
smgrdounlinkall and smgrtruncate. With only the barrier left, a
backend would hold descriptors for dropped relations until someone
drops a database or a tablespace. That is a behavior change, not a
cleanup.

The barrier also only releases, it does not destroy, so it does not
fix what Nazir reported. The hash table in the IO worker keeps
growing either way. Nothing pins those entries in an IO worker, and
smgrdestroyall only zaps unpinned ones, so destroying them there
should be safe.

That suggests a version with no lock at all. Keep the cleanup where
Nazir put it, at a safe point in the worker loop, but trigger it on a
local condition, the number of unpinned entries being over a cap,

rather than on a checkpoint generation. No shared state, and it also

covers a worker that never goes idle.

Do I/O workers use pins?

There is already `max_files_per_process` (default 1000), after
which it forces closing the least recently used files.
And that goes exactly where a cap on the number of files should
be, checking when a file is about to be open, but maybe not in all
paths.

ReleaseLruFiles calls ReleaseLruFiles repeatedly until
nfile + numAllocatedDescs + numExternalFDs < max_safe_fds
Called fom
PathNameOpenFilePerm, OpenTransientFilePerm

We RleaseLruFiles at the top and on error, ReleaseLruFile and TryAgain
pattern in
AllocateFile, AllocateDir, AllocatePipe

BasicOpenFilePerm has only the ReleaseLruFile on error, but the caller's do
ReleaseLruFiles, except for BasicOpenFile.

So, I see one way to violate max_files_per_process: By repeatedly calling
BasicOpenFile.

BasicOpenFile is not called by fd.c, or anything in src/backend/storage/

Grepping it on the codebase.
src/backend/access/transam/xlogutils.c:837: state->seg.ws_file =
BasicOpenFile(path, O_RDONLY | PG_BINARY);
src/backend/access/transam/xlogrecovery.c:4267: fd = BasicOpenFile(path,
O_RDONLY | PG_BINARY);
src/backend/access/transam/xlog.c:3323: fd = BasicOpenFile(path, O_RDWR |
PG_BINARY | O_CLOEXEC |
src/backend/access/transam/xlog.c:3351: fd = BasicOpenFile(tmppath,
open_flags);
src/backend/access/transam/xlog.c:3505: fd = BasicOpenFile(path, O_RDWR |
PG_BINARY | O_CLOEXEC |
src/backend/access/transam/xlog.c:3737: fd = BasicOpenFile(path, O_RDWR |
PG_BINARY | O_CLOEXEC |
src/backend/access/transam/xlog.c:4432: fd =
BasicOpenFile(XLOG_CONTROL_FILE,
src/backend/access/transam/xlog.c:4480: fd =
BasicOpenFile(XLOG_CONTROL_FILE,
src/backend/postmaster/walsummarizer.c:1608: state->seg.ws_file
= BasicOpenFile(path, O_RDONLY | PG_BINARY);
src/backend/utils/misc/guc.c:4742: Tmpfd =
BasicOpenFile(AutoConfTmpFileName,
src/backend/storage/file/fd.c:1090:BasicOpenFile(const char *fileName, int
fileFlags)
src/backend/replication/walsender.c:3347: state->seg.ws_file =
BasicOpenFile(path, O_RDONLY | PG_BINARY);
src/common/controldata_utils.c:223: if ((fd =
BasicOpenFile(ControlFilePath, O_RDWR | PG_BINARY)) < 0);

This made me wonder whether the solution for the WAL segment descriptor leak
isn't just a matter of calling `PathNameOpenFilePerm` instead of `
BasicOpenFile`
in xlog.c (this is a long shot). If we have a file management
infrastructure it
is probably better to use that.

Also is this showing up in any logs?
/*
* Close the file. We aren't expecting this to fail; if it does, better
* to leak the FD than to mess up our internal state.
*/
if (close(vfdP->fd) != 0)
elog(vfdP->fdstate & FD_TEMP_FILE_LIMIT ? LOG : data_sync_elevel(LOG),
"could not close file \"%s\": %m", vfdP->fileName);
vfdP->fd = VFD_CLOSED;
--nfile;

Closing unused in LRU (not only capping number of open files)
=====================

On the ReleaseLru we could put a limit to the number of I/O not touching
the file (N).
Just keep a per-worker I/O counter, every I/O check how many operations
have been
performed since we last used the LRU.
And the number of unnecessary open(): the worst case is to open
`max_files_per_process`
then read N times from a single file, then read from each file immediately
after they
close. This gives (N + max_files_per_process - 1) operations, and
(max_files_per_process - 1)
additional open(), N / (max_files_per_process - 1) + 1.

I think this is cheap enough to go after starting an I/O, and we will
close at
most one file per iteration (because no two files have the same last I/O
stamp).
And compared to the existing LRU limit, has the advantage that it would
close both forgotten files, files that are used for an infrequent query, or
descriptors for open relations. And avoid destroying all descriptors at
once.

An approach like this is easy to be added to SMGR functions so that it
benefits not only the I/O workers.

What this does not answer is how an IO worker finds out that a
relation was unlinked, so it can close the descriptor without waiting
for its own eviction. That is the sinval question you raised, and it
looks separate from Nazir's patch to me.

The above seems a good compromise if we can't use inval in the worker.
It will only close files that are open but not used for a while. And if we
To cover the case where the server is not too busy and will take too long
to reach the I/O count that will trigger its closure, we could keep a
timestamp
of the last used in addition to the I/O count.

Regards,
Alexandre

Hi,

Thank you for looking into this all!

On Tue, 22 Sept 2026 at 03:12, shihao zhong <zhong950419@gmail.com> wrote:

Right, the per IO check puts the spinlock in the hot path.

The barrier also only releases, it does not destroy, so it does not
fix what Nazir reported. The hash table in the IO worker keeps
growing either way. Nothing pins those entries in an IO worker, and
smgrdestroyall only zaps unpinned ones, so destroying them there
should be safe.

Correct.

That suggests a version with no lock at all. Keep the cleanup where
Nazir put it, at a safe point in the worker loop, but trigger it on a
local condition, the number of unpinned entries being over a cap,
rather than on a checkpoint generation. No shared state, and it also
covers a worker that never goes idle.

We need to make a function call to hash_get_num_entries() for each IO
to check number of unpinned entries, which doesn't sound good to me.
This is especially true for the partitioned case.

For now, I used the number of IOs (capped at 1024) since the last smgr
cleanup. This solution removes CheckpointerShmem->ckpt_lck contention
and worker goes idle problem.

--
Regards,
Nazir Bilal Yavuz
Microsoft

Hi Nazir,

On the entry count. Nothing is pinned in an IO worker, only the
relcache calls smgrpin(), which also answers Alexandre's question. So
the unpinned count is just hash_get_num_entries(), and that only reads
a counter. SMgrRelationHash is local and not partitioned. The IO
counter is fine with me too though.

One gap in v2. v1 cleaned up when the worker went idle, v2 does not.
A worker that does fewer than 1024 IOs and then sleeps keeps those
entries and their descriptors until it gets busy again. #19622 has
that case, workers idle for 51 minutes still holding descriptors of
dropped tables. Maybe also clean up in the idle branch when
ios_since_smgr_cleanup > 0. That needs no lock either.

Thanks,
Shihao

Hi,

On Wed, 23 Sept 2026 at 15:18, shihao zhong <zhong950419@gmail.com> wrote:

On the entry count. Nothing is pinned in an IO worker, only the
relcache calls smgrpin(), which also answers Alexandre's question. So
the unpinned count is just hash_get_num_entries(), and that only reads
a counter. SMgrRelationHash is local and not partitioned. The IO
counter is fine with me too though.

You are right that SMgrRelationHash is local and not partitioned, I
missed that. Then, I think using the 'number of unpinned entries'
makes more sense because the IO count won't work well when multiple
IOs use the same relation. I introduced the smgrnumentries() function
in v3 to get the number of cached SMgrRelation objects.

One gap in v2. v1 cleaned up when the worker went idle, v2 does not.
A worker that does fewer than 1024 IOs and then sleeps keeps those
entries and their descriptors until it gets busy again. #19622 has
that case, workers idle for 51 minutes still holding descriptors of
dropped tables. Maybe also clean up in the idle branch when
ios_since_smgr_cleanup > 0. That needs no lock either.

Done.

One thing remains: if the cache doesn't grow enough and the worker
doesn't go idle (i.e. all IOs process less than
PGAIO_WORKER_SMGR_CLEANUP_THRESHOLD relation), we don't clear the SMGR
objects but I think this is a correct behavior.

--
Regards,
Nazir Bilal Yavuz
Microsoft

Hi Nazir,

Andres, Shihao -- please check this is flawed.

You already proposed to use PGAIO_WORKER_SMGR_CLEANUP_INTERVAL and I think
that is better than PGAIO_WORKER_SMGR_CLEANUP_THRESHOLD from v3.

And I think that instead of trying to cover corner cases we have to design
a solution that has clear operation conditions (invariants).

Here I outline one solution, if you want to implement yourself.

Invariants (unless the process is sleeping):
Any open file was used at least once during the last two checkpoints.
Any open file was used at least once over the last OPEN_FILE_EXPIRE_COUNT
operations.

Actions:
- On a checkpoint destroy files that were not used since the previous
checkpoint
less destructive than destroy all.
- After an I/O closes the LRU file if it was not used during the last
OPEN_FILE_EXPIRE_COUNT

All you need to add is an I/O stamp on each entry.

This should keep the files that are actively used always open,
while setting a clear limit on how long a file can remain open.
io_count wraps safely,

Here I present a pseudo-code, suggesting the loop, but it could be called
from
the I/O functions themselves, integrating with the LRU update and protecting
not only I/O workers, but any process that uses this API.

Since we are only closing old files it is unlikely that we have to wait.

ckpt_io = 0
io_count = 0
loop (io) {
++io_count;
newest = get_file(io.rlocator);
newest.last_use = io_count;
lru_tounch_file(newest); // move to the top of the list.
oldest = lru.tail;
cleanup(cutoff);
}

cleanup(cutoff) {
// O(1) amortized, always check at most one surviving
// file per call, and the number destroyed files
// is less than the number of I/O.
// Closes at most one file except when crossing a checkpoint.

// entries not used over the last operations
cutoff = io_count - OPEN_FILE_EXPIRE_COUNT;
if FirstCallSinceLastCheckpoint() {
// destroys entries not used since the previous checkpoint.
cutoff = max(cutoff, ckpt_io - 1);
ckpt_io = io_count;
}

for(;;) {
oldest = lru.tail;
if (lru.tail == NULL) never
// wrap-safe comparison
if (lru.last_use - cutoff >= 0)
break; // always true for the last file used.
destroy(oldest);
}
}

I would remove BasicOpenFile from fd.c, that function is dangerous.

And of course remove the spin lock inside FirstCallSinceLastCheckpoint()

Regards,
Alexandre

Hi Alexandre,

I don't think this patch needs the LRU. Backends already call
smgrdestroyall() at transaction end, and bgwriter and checkpointer do it
after each checkpoint, so the IO worker is the one process that never
cleans up, and v3 bounds it with the entry cap and the idle cleanup.

Touching an LRU and calling FirstCallSinceLastCheckpoint() on every IO
puts work back in the hot path, which is what Andres objected to as well.

Thanks,
Shihao

On Sun, Sep 27, 2026 at 5:23 PM shihao zhong <zhong950419@gmail.com> wrote:

Touching an LRU and calling FirstCallSinceLastCheckpoint() on every IO
puts work back in the hot path, which is what Andres objected to as well.

I am aware of those objections, this proposal is very different type of
work.

No inval subscription as in my v1 (that involves shared memory, LWLock's,
iterating and filtering irrelevant messages). And also no global spin lock
as
Nazir's v1.

About touching LRU, I thought that the I/O worker already updated the LRU
list every operation, does it go through a different path?

Regards,
Alexandre

Hi Alexandre,

About touching LRU, I thought that the I/O worker already updated the
LRU list every operation, does it go through a different path?

You are right, FileAccess() in fd.c moves the file to the head of its
LRU ring on every read and write, so that part costs nothing new. I
was wrong there.

I still think v3 is enough for this patch, but I will leave it to
Nazir and Andres.

Thanks,
Shihao