Re: aio: worker: Free SMGR objects when idle

+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(at)gmail(dot)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