Re: aio: worker: Free SMGR objects when idle - Mailing list pgsql-hackers
| From | Alexandre Felipe |
|---|---|
| Subject | Re: aio: worker: Free SMGR objects when idle |
| Date | |
| Msg-id | CAE8JnxOyYyv6m=Mz+NwQb=Qj6yFkXD2a+n=+Nes6mhmH6L5PoQ@mail.gmail.com Whole thread |
| In response to | Re: aio: worker: Free SMGR objects when idle (shihao zhong <zhong950419@gmail.com>) |
| List | pgsql-hackers |
+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);
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;
* 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
pgsql-hackers by date: