public inbox for [email protected]  

help / color / mirror / Atom feed

From: Heikki Linnakangas <[email protected]>
To: Jelte Fennema-Nio <[email protected]>
To: Andres Freund <[email protected]>
Cc: Tom Lane <[email protected]>
Cc: Tomas Vondra <[email protected]>
Cc: PostgreSQL-development <[email protected]>
Subject: Re: Bump soft open file limit (RLIMIT_NOFILE) to hard limit on startup
Date: Fri, 4 Apr 2025 20:34:21 +0300
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <CAGECzQQh6VSy3KG4pN1d=h9J=D1rStFCMR+t7yh_Kwj-g87aLQ@mail.gmail.com>
	<[email protected]>
	<[email protected]>
	<7u7dbn6s2i6bf3hjzkbqaexj2bpoblqxwbkffbetl4rjv6dcom@s2uickjc5z53>
	<[email protected]>
	<CAGECzQQqO0DKhyZtUWRKSL=WeBSYM=+gZ+dY58xQ2ZnypSONTw@mail.gmail.com>
	<lj5q3h4zxwhfusvv2azjnwva74yjwthkrrsvcxrac67f636tpi@ewtgldqjfwes>
	<CAGECzQQ1JnUim8KEFVrM+d4zfvxg=yVmzB8rLv4W5mKmnVnEuQ@mail.gmail.com>
	<4huky7iczrycvq3ptpjkkzrclsabqceu6jyppizotjafqywq5g@g4eynaqxjog5>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>

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)

view thread (4+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected], [email protected]
  Subject: Re: Bump soft open file limit (RLIMIT_NOFILE) to hard limit on startup
  In-Reply-To: <[email protected]>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox