+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 <[email protected]> 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
