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

Hi Jelte,

Apologies for the delay.

> I agree with this. But I feel like that's exactly what this patchset
> is currently doing. The newly introduced fork_for_shell_command makes
> sure almost nothing is happening on the child-side of the fork. I
> don't understand how the thing you describe differs from my currently
> proposed implementation. Could you clarify?
>
> I did some more digging and here is what I saw a place where it reaches
indirectly:
pid = fork();
if (pid == 0)
{
UseOriginalOpenFileLimit();
pqsignal(SIGINT, PG_SIG_DFL);
...
}

UseOriginalOpenFileLimit() reports a setrlimit() failure with
ereport(WARNING, (errmsg("setrlimit failed: %m"))). So the child body reads
as four statements, but one of them enters the error-reporting subsystem:
errstart() switches the current memory context to ErrorContext, errmsg()
runs the format through dgettext() and builds the result with
initStringInfo()/pstrdup(), and errfinish() emits it via EmitErrorReport()
through the configured log destinations, which can include syslog(). That
is what I meant by "PostgreSQL work in the child", and it is not visible at
the call site.

I should also correct how I characterised fork_process() in my previous
mail.
I set it aside as not being a counterexample, when it is actually the
comparison that makes my point: its child-side body is limited to getpid(),
getenv(), open(), write(), close() and a no-op init call. Not all of those
are formally in POSIX's async-signal-safe set either, but none of them
allocates, takes a lock, or enters a PostgreSQL subsystem. That property is
what I care about, rather than list membership as such.

In the same spirit I should be upfront that setrlimit() is itself not in
POSIX's async-signal-safe set. I don't think that is worth changing: it is
a direct syscall in every libc we target, and takes no locks and allocates
nothing. ereport() is a different category.

To be clear about why I think this is worth addressing even though it
cannot deadlock on today's process-per-backend architecture:
- POSIX restricts what may be called between fork() and exec() in a process
that has multiple threads, and ereport() is well outside that.
- We build against glibc, musl, macOS, the BSDs, Solaris and Windows, so I
would rather not depend on any one of them being forgiving here.
- ereport() reaches our own subsystems, not just libc. Once backends are
threads those acquire their own synchronization, and no libc behaviour
helps with that.
- Keeping ereport() safe on this path in perpetuity, across future changes
to elog.c, memory contexts, log destinations and translation, seems like an
awkward invariant to have to maintain.

There is also a smaller present-day consequence: fork_for_shell_command()
does not set MyProcPid the way fork_process() does, so if that ereport()
ever fires the line is emitted with the parent's PID and session context
attached, which would be misleading output.

To be clear, the WARNING in IncreaseOpenFileLimit() seems fine to me: that
one runs during set_max_safe_fds() at startup, not in a child, and I take
your earlier point that a failure there indicates a bug worth surfacing. It
is only the copy reached from the child path that I am asking about.

So, concretely:
1. Report the setrlimit() failure from the parent, drop it, or emit it with
a plain write(2), so that the child path contains only syscall-level work.
2. Add a comment on fork_for_shell_command() stating the constraint. This
is new infrastructure that will have more additions, and the hazard is
invisible from the call site.
3. In pg_popen()'s child, dup2()'s return value isn't checked. On failure
the child would exec with the parent's descriptor still in place, so the
command's output would go elsewhere and the parent would see immediate EOF
with no error.

I am no longer suggesting posix_spawn(), by the way: there is no standard
way to set a resource limit through it, which is the whole point here. So a
small fork/exec does look like the only workable approach, which is why the
two items above are what I am asking for rather than a different
abstraction.

> Nothing is explicitly exposed to make it easy to do so, but changing
> the soft limit after startup should not cause any problems (except
> that if you lower it you can open fewer files ofcourse).
>
>
Maybe it is worth a comment then: max_safe_fds is computed once during
startup, so a session that lowers rlim_cur afterwards leaves fd.c's
accounting stale. Today that stays within one backend. Once backends are
threads the limit is process-wide, so one session lowering it would
affect every
other session.

Regards,
Surya Poondla