Re: Bump soft open file limit (RLIMIT_NOFILE) to hard limit on startup

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