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)