Hi Jelte, Haibo (cc'ed) and I reviewed this as part of a patch review workshop.
We agree with the basic policy Andres suggested: PostgreSQL should try to raise the soft RLIMIT_NOFILE to cover the file descriptors already in use plus max_files_per_process, rather than simply raising it to the hard limit. The current patch is generally moving in the right direction on that part. Our main concern is the mechanism introduced to preserve the original limit for external commands. The patch adds its own pg_system()/pg_popen() implementation based on fork(), performs PostgreSQL-specific work in the child, restores the saved startup-time resource limit, adjusts signals and file descriptors, and then calls exec(). This does not seem like a good long-term abstraction if we want to consider a threaded PostgreSQL architecture. In that architecture, backend sessions would be threads in the same server process. Forking such a process is fundamentally different from fork_process() in today's process-per-backend architecture: only the calling thread survives in the child, while locks held by other threads may remain inherited. The code between fork() and exec() therefore needs to be extremely small and restricted to operations that are safe in this environment. fork_process() is not really a counterexample. It is infrastructure designed specifically for PostgreSQL's current process architecture and will necessarily have to be redesigned if backends become threads. We should avoid introducing new infrastructure today that carries the same assumption, particularly when threading is itself being used as a motivation for the redesign. The FD-limit policy and the external-process spawning mechanism also look like two separate design questions. Raising RLIMIT_NOFILE becomes even more useful in a threaded architecture because client sockets, relation files, AIO descriptors, etc. would all share one process-wide FD limit. But that does not imply that the current fork()-based pg_system()/pg_popen() design is suitable for that architecture. To make this forward-looking, a better direction would be a narrowly defined process-spawning abstraction, using posix_spawn() where appropriate or a minimal fork/exec implementation. The child-side path should only perform the operations strictly required before exec(), such as descriptor setup, signal setup, and resource-limit adjustment, while allocation, logging, error handling, and other PostgreSQL work remain in the parent. So we support the basic RLIMIT_NOFILE policy change, but we are not convinced that reimplementing system() and popen() this way is the right abstraction for it. A few things from reading the patch: 1. Is there any supported way for a session to affect the soft limit after startup, such as calling setrlimit() from a PL/pgSQL (or other procedural language) function? If the answer is "no", it would be good to state that assumption explicitly. 2. In IncreaseOpenFileLimit(), rlim_cur + extra_files would overflow if rlim_cur were RLIM_INFINITY, and Min() would then pick the wrapped value and lower the limit drastically. That is unreachable today, but only via the earlier "rlim_cur == original_max_open_files.rlim_max" test combined with the rlim_cur <= rlim_max guarantee. A comment or an explicit RLIM_INFINITY check would make it less fragile. 3. Nit: SaveOriginalOpenFileLimit() returns void, but its comment says "Returns true if successful, false otherwise." Regards, Surya Poondla
