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

On 18/03/2025 01:08, Jelte Fennema-Nio wrote:
> On Mon Feb 24, 2025 at 12:01 PM CET, Jelte Fennema-Nio wrote:
>> Right after pressing send I realized I could remove two more useless
>> lines...
>
> Rebased patchset attached (trivial conflict against pg_noreturn
> changes).

v7-0001-Adds-a-helper-for-places-that-shell-out-to-system.patch:

> /*
> * A custom wrapper around the system() function that calls the necessary
> * functions pre/post-fork.
> */
> int
> System(const char *command, uint32 wait_event_info)
> {
> int rc;
>
> fflush(NULL);
> pgstat_report_wait_start(wait_event_info);
>
> if (wait_event_info == WAIT_EVENT_RESTORE_COMMAND)
> {
> /*
> * Set in_restore_command to tell the signal handler that we should
> * exit right away on SIGTERM. This is done for the duration of the
> * system() call because there isn't a good way to break out while it
> * is executing. Since we might call proc_exit() in a signal handler,
> * it is best to put any additional logic outside of this section
> * where in_restore_command is set to true.
> */
> in_restore_command = true;
>
> /*
> * Also check if we had already received the signal, so that we don't
> * miss a shutdown request received just before this.
> */
> if (shutdown_requested)
> proc_exit(1);
> }
>
> rc = system(command);
>
> if (wait_event_info == WAIT_EVENT_RESTORE_COMMAND)
> in_restore_command = false;
>
> pgstat_report_wait_end();
> return rc;
> }

Let's move that 'in_restore_command' business to the caller. It's weird
modularity for the function to implicitly behave differently for some
callers. And 'wait_event_info' should only affect pgstat reporting, not
actual behavior.

I don't feel good about the function name. How about pg_system() or
something? postmaster/startup.c also seems like a weird place for it;
not sure where it belongs but probably not there. Maybe next to
OpenPipeStream() in fd.c, or move both to a new file.

v7-0002-Bump-postmaster-soft-open-file-limit-RLIMIT_NOFIL.patch:

> @@ -274,6 +275,7 @@ System(const char *command, uint32 wait_event_info)
> {
> int rc;
>
> + RestoreOriginalOpenFileLimit();
> fflush(NULL);
> pgstat_report_wait_start(wait_event_info);
>
> @@ -303,6 +305,7 @@ System(const char *command, uint32 wait_event_info)
> in_restore_command = false;
>
> pgstat_report_wait_end();
> + RestoreCustomOpenFileLimit();
> return rc;
> }
>

Looks a bit funny that both functions are called Restore<something>().
Not sure what to suggest instead though. Maybe RaiseOpenFileLimit() and
LowerOpenFileLimit().

> @@ -2724,6 +2873,19 @@ OpenPipeStream(const char *command, const char *mode)
> ReleaseLruFiles();
>
> TryAgain:
> +
> + /*
> + * It would be great if we could call RestoreOriginalOpenFileLimit here,
> + * but since popen() also opens a file in the current process (this side
> + * of the pipe) we cannot do so safely. Because we might already have many
> + * more files open than the original limit.
> + *
> + * The only way to address this would be implementing a custom popen()
> + * that calls RestoreOriginalOpenFileLimit only around the actual fork
> + * call, but that seems too much effort to handle the corner case where
> + * this external command uses both select() and tries to open more files
> + * than select() allows for.
> + */
> fflush(NULL);
> pqsignal(SIGPIPE, SIG_DFL);
> errno = 0;

What would it take to re-implement popen() with fork+exec? Genuine
question; I have a feeling it might be complicated to do correctly on
all platforms, but I don't know.

--
Heikki Linnakangas
Neon (https://neon.tech)