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

On Mon, 17 Feb 2025 at 18:24, Andres Freund <andres(at)anarazel(dot)de> wrote:
> Why not do this only in the if (rlim.rlim_cur < nclients + 3) case?

Done, I also changed it to not bump to rlim_max, but only to nclients
+ 3. The rest of the patches I'll update later. But response below.

> I think it might be better to have a precursor commit that wraps system(),
> including the fflush(), pgstat_report_wait_start(), pgstat_report_wait_end().
>
> It seems a bit silly to have a dance of 5 function calls that are repeated in
> three places.

Fair enough, I'll update that in the next version.

> > +#ifdef HAVE_GETRLIMIT
> > +/*
> > + * Increases the open file limit (RLIMIT_NOFILE) by the requested amount.
> > + * Returns true if successful, false otherwise.
> > + */
> > +static bool
> > +IncreaseOpenFileLimit(int extra_files)
> > +{
>
> Why is this a relative instead of an absolute amount?

I don't feel strongly about this, but it seemed nicer to encapsulate
that. My callers (and I expect the io_uring one too) all had the
relative amount. Seemed a bit silly to always require the caller to
add custom_max_open_files.rlim_cur to that, especially since you might
want to call IncreaseOpenFileLimit from another file for your io_uring
stuff, which would mean also making custom_max_open_files.rlim_cur
non-static. Or of course you'd have to query getrlimit again.

> > + if (setrlimit(RLIMIT_NOFILE, &rlim) != 0)
> > + {
> > + ereport(WARNING, (errmsg("setrlimit failed: %m")));
> > + return false;
> > + }
>
> Is a WARNING really appropriate here? I guess it's just copied from elsewhere
> in the file, but still...

I mean this shouldn't ever fail. We checked beforehand if we are
allowed to increase it this far. So if setrlimit fails then there's a
bug somewhere. That made me think it was sensible to report a warning,
so at least people can see something strange is going on.

> > + if (setrlimit(RLIMIT_NOFILE, &original_max_open_files) != 0)
> > + {
> > + ereport(WARNING, (errmsg("setrlimit failed: %m")));
> > + }
> > +#endif
> > +}
>
> Hm. Does this actually work if we currently have more than
> original_max_open_files.rlim_cur files open?

So according to at least the Linux and FreeBSD manpages, lowering
below the currently open files is not a failure mode.
- https://linux.die.net/man/2/setrlimit
- https://man.freebsd.org/cgi/man.cgi?query=setrlimit

I also tried this out manually on my Linux machine, by starting
postgres with only a soft limit of 15 (lower than that and it wouldn't
start). I was then able to change the limits without issue when
calling setrlimit or system.

... Sadly that turned out not to be true when calling popen in
OpenPipeStream (ofcourse). Because that will actually open a file in
the postgres process too, and at that point you're already over your
file limit. I can see two ways around that:
1. Writing a custom version of popen, where we lower the limit just
before we call exec in the forked process.
2. Don't care about restoring the original limit for OpenPipeStream

My suggestion would be to go for 2: It doesn't seem worth the
complexity of having a custom popen implementation, just to be able to
handle buggy programs (i.e. ones that cannot handle more than 1024
files, while still trying to open more than that amount of files)

> It'd be fine to do the system() with more files open, because it'll internally
> do exec(), but of course setrlimit() doesn't know that ...

I'm not sure I understand what you mean here.

> All these ifdefs make this function rather hard to read. It was kinda bad
> before, but it does get even worse with this patch. Not really sure what to
> do about that though.

Agreed, I'll see if I can figure out a way to restructure it a bit.

> We were discussing calling set_max_fds() twice to deal with io_uring FDs
> requiring a higher FD limit than before. With the patch as-is, we'd overwrite
> our original original_max_open_files in that case...

That should be very easy to change if that's indeed how we want to
bump the rlimit for the io_uring file descriptors. But it didn't seem
like you were sure which of the two options you liked best.

> Hm. When is this second case here reachable and useful? Shouldn't we already
> have increased the limit before getting here? If so IncreaseOpenFileLimit()
> won't help, no?

While it's a very unlikely edge case, it's theoretically possible
(afaict) that the maximum number of open files is already open. So
highestfd will then be 0 (because it's the first loop), and we'll only
find out that we're at the limit due to getting ENFILE.

I'll add a code comment about this, because it's indeed not obvious.

> > @@ -1078,6 +1164,7 @@ set_max_safe_fds(void)
> > max_safe_fds, usable_fds, already_open);
> > }
> >
> > +
> > /*
> > * Open a file with BasicOpenFilePerm() and pass default file mode for the
> > * fileMode parameter.
>
> Unrelated change.

Ugh, yeah I guess I added the new functions there originally, but
moved them around and didn't clean up the added whitespace.

> If we want to reflect the value, shouldn't it show up as PGC_S_OVERRIDE or
> such?

Makes sense