I wrote:
> Noah Misch <n...@leadboat.com> writes:
>> On Thu, May 01, 2014 at 12:13:28PM -0400, Tom Lane wrote:
>>> Is there any reason not to change this to just fflush(NULL)?  We dropped
>>> support for SunOS 4.1 quite some time ago ...

>> Modern systems have other fflush(NULL) problems:

>> http://www.nntp.perl.org/group/perl.perl5.porters/2013/09/msg207692.html
>> http://perl5.git.perl.org/metaconfig.git/blob/master:/U/perl/fflushall.U

> Fun.  I doubt that the postmaster's stdin would ever be a pipe, but
> maybe we'd better leave well enough alone.  Should update the comment
> though.

However ... after looking around I notice that fflush(NULL) is already
being used in parallel pg_dump and pg_upgrade; and at least in the latter
case I'm afraid to change that because it looks like there are probably
other stdio output files open in the process.

I think we should probably go ahead and switch over to using
fflush(NULL).  The capability is required by C89 for crissake;
are we really going to rely on hearsay evidence that there are
current platforms that are still broken?  Also, I found at least
one claim on the net that this has been fixed in Solaris for awhile:
http://grokbase.com/t/perl/perl5-porters/021hn4z9pq/fflush-null-on-solaris-9

Attached is a draft patch that I was working on before I decided that
making the indicated changes in pg_upgrade was probably a bad idea.
This is mainly to memorialize the places we should look at if we
change it.

BTW, while working on this I noticed that there are a boatload of places
where we use system() or popen() without a prior fflush.  I suspect most
of them are safe, or we'd have heard more complaints --- but shouldn't
we clamp down on that?

                        regards, tom lane

diff --git a/contrib/pg_upgrade/controldata.c b/contrib/pg_upgrade/controldata.c
index fa0a005..3bf9c6a 100644
*** a/contrib/pg_upgrade/controldata.c
--- b/contrib/pg_upgrade/controldata.c
*************** get_control_data(ClusterInfo *cluster, b
*** 114,119 ****
--- 113,120 ----
  			 cluster->bindir,
  			 live_check ? "pg_controldata\"" : "pg_resetxlog\" -n",
  			 cluster->pgdata);
+ 
+ 	/* Don't use fflush(NULL); see comments in fork_process.c */
  	fflush(stdout);
  	fflush(stderr);
  
diff --git a/contrib/pg_upgrade/parallel.c b/contrib/pg_upgrade/parallel.c
index f4201e1..7807687 100644
*** a/contrib/pg_upgrade/parallel.c
--- b/contrib/pg_upgrade/parallel.c
*************** parallel_exec_prog(const char *log_file,
*** 118,125 ****
  		/* set this before we start the job */
  		parallel_jobs++;
  
! 		/* Ensure stdio state is quiesced before forking */
! 		fflush(NULL);
  
  #ifndef WIN32
  		child = fork();
--- 118,126 ----
  		/* set this before we start the job */
  		parallel_jobs++;
  
! 		/* Don't use fflush(NULL); see comments in fork_process.c */
! 		fflush(stdout);
! 		fflush(stderr);
  
  #ifndef WIN32
  		child = fork();
*************** parallel_transfer_all_new_dbs(DbInfoArr 
*** 227,234 ****
  		/* set this before we start the job */
  		parallel_jobs++;
  
! 		/* Ensure stdio state is quiesced before forking */
! 		fflush(NULL);
  
  #ifndef WIN32
  		child = fork();
--- 228,236 ----
  		/* set this before we start the job */
  		parallel_jobs++;
  
! 		/* Don't use fflush(NULL); see comments in fork_process.c */
! 		fflush(stdout);
! 		fflush(stderr);
  
  #ifndef WIN32
  		child = fork();
diff --git a/contrib/pgbench/pgbench.c b/contrib/pgbench/pgbench.c
index 7c1e59e..cf84fcf 100644
*** a/contrib/pgbench/pgbench.c
--- b/contrib/pgbench/pgbench.c
*************** pthread_create(pthread_t *thread,
*** 3328,3333 ****
--- 3328,3337 ----
  		return errno;
  	}
  
+ 	/* Don't use fflush(NULL); see comments in fork_process.c */
+ 	fflush(stdout);
+ 	fflush(stderr);
+ 
  	th->pid = fork();
  	if (th->pid == -1)			/* error */
  	{
diff --git a/src/backend/postmaster/fork_process.c b/src/backend/postmaster/fork_process.c
index 3e2acdd..28f4ca7 100644
*** a/src/backend/postmaster/fork_process.c
--- b/src/backend/postmaster/fork_process.c
*************** fork_process(void)
*** 38,45 ****
  
  	/*
  	 * Flush stdio channels just before fork, to avoid double-output problems.
! 	 * Ideally we'd use fflush(NULL) here, but there are still a few non-ANSI
! 	 * stdio libraries out there (like SunOS 4.1.x) that coredump if we do.
  	 * Presently stdout and stderr are the only stdio output channels used by
  	 * the postmaster, so fflush'ing them should be sufficient.
  	 */
--- 38,50 ----
  
  	/*
  	 * Flush stdio channels just before fork, to avoid double-output problems.
! 	 *
! 	 * Ideally we'd use fflush(NULL) here, but that has portability issues ---
! 	 * notably, it's reported that current Solaris as of 2013 still has some
! 	 * odd behaviors in fflush(NULL), such as closing stdin if it's a pipe.
! 	 * While it's not clear that that would affect the postmaster, it still
! 	 * seems best to stay away from fflush(NULL).
! 	 *
  	 * Presently stdout and stderr are the only stdio output channels used by
  	 * the postmaster, so fflush'ing them should be sufficient.
  	 */
diff --git a/src/backend/storage/file/fd.c b/src/backend/storage/file/fd.c
index 0560bf9..268584a 100644
*** a/src/backend/storage/file/fd.c
--- b/src/backend/storage/file/fd.c
*************** OpenPipeStream(const char *command, cons
*** 1741,1746 ****
--- 1741,1747 ----
  	ReleaseLruFiles();
  
  TryAgain:
+ 	/* Don't use fflush(NULL); see comments in fork_process.c */
  	fflush(stdout);
  	fflush(stderr);
  	errno = 0;
diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c
index b53fa8b..55e8c71 100644
*** a/src/bin/initdb/initdb.c
--- b/src/bin/initdb/initdb.c
*************** popen_check(const char *command, const c
*** 692,697 ****
--- 692,698 ----
  {
  	FILE	   *cmdfd;
  
+ 	/* Don't use fflush(NULL); see comments in fork_process.c */
  	fflush(stdout);
  	fflush(stderr);
  	errno = 0;
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index 1a468fa..73a2428 100644
*** a/src/bin/pg_basebackup/pg_basebackup.c
--- b/src/bin/pg_basebackup/pg_basebackup.c
*************** StartLogStreamer(char *startpos, uint32 
*** 437,442 ****
--- 437,446 ----
  	 * a fork(). On Windows, we create a thread.
  	 */
  #ifndef WIN32
+ 	/* Don't use fflush(NULL); see comments in fork_process.c */
+ 	fflush(stdout);
+ 	fflush(stderr);
+ 
  	bgchild = fork();
  	if (bgchild == 0)
  	{
diff --git a/src/bin/pg_dump/parallel.c b/src/bin/pg_dump/parallel.c
index afe941f..1f35ab9 100644
*** a/src/bin/pg_dump/parallel.c
--- b/src/bin/pg_dump/parallel.c
*************** ParallelBackupStart(ArchiveHandle *AH, R
*** 489,497 ****
  
  	Assert(AH->public.numWorkers > 0);
  
- 	/* Ensure stdio state is quiesced before forking */
- 	fflush(NULL);
- 
  	pstate = (ParallelState *) pg_malloc(sizeof(ParallelState));
  
  	pstate->numWorkers = AH->public.numWorkers;
--- 489,494 ----
*************** ParallelBackupStart(ArchiveHandle *AH, R
*** 539,544 ****
--- 536,542 ----
  		pstate->parallelSlot[i].args = (ParallelArgs *) pg_malloc(sizeof(ParallelArgs));
  		pstate->parallelSlot[i].args->AH = NULL;
  		pstate->parallelSlot[i].args->te = NULL;
+ 
  #ifdef WIN32
  		/* Allocate a new structure for every worker */
  		wi = (WorkerInfo *) pg_malloc(sizeof(WorkerInfo));
*************** ParallelBackupStart(ArchiveHandle *AH, R
*** 553,558 ****
--- 551,561 ----
  								wi, 0, &(pstate->parallelSlot[i].threadId));
  		pstate->parallelSlot[i].hThread = handle;
  #else
+ 
+ 		/* Don't use fflush(NULL); see comments in fork_process.c */
+ 		fflush(stdout);
+ 		fflush(stderr);
+ 
  		pid = fork();
  		if (pid == 0)
  		{
diff --git a/src/bin/pg_dump/pg_dumpall.c b/src/bin/pg_dump/pg_dumpall.c
index 47fe6cc..fbb3822 100644
*** a/src/bin/pg_dump/pg_dumpall.c
--- b/src/bin/pg_dump/pg_dumpall.c
*************** runPgDump(const char *dbname)
*** 1692,1697 ****
--- 1692,1698 ----
  	if (verbose)
  		fprintf(stderr, _("%s: running \"%s\"\n"), progname, cmd->data);
  
+ 	/* Don't use fflush(NULL); see comments in fork_process.c */
  	fflush(stdout);
  	fflush(stderr);
  
diff --git a/src/bin/psql/copy.c b/src/bin/psql/copy.c
index d706206..09b2021 100644
*** a/src/bin/psql/copy.c
--- b/src/bin/psql/copy.c
*************** do_copy(const char *args)
*** 288,293 ****
--- 288,294 ----
  		{
  			if (options->program)
  			{
+ 				/* Don't use fflush(NULL); see comments in fork_process.c */
  				fflush(stdout);
  				fflush(stderr);
  				errno = 0;
*************** do_copy(const char *args)
*** 307,318 ****
  		{
  			if (options->program)
  			{
  				fflush(stdout);
  				fflush(stderr);
- 				errno = 0;
  #ifndef WIN32
  				pqsignal(SIGPIPE, SIG_IGN);
  #endif
  				copystream = popen(options->file, PG_BINARY_W);
  			}
  			else
--- 308,320 ----
  		{
  			if (options->program)
  			{
+ 				/* Don't use fflush(NULL); see comments in fork_process.c */
  				fflush(stdout);
  				fflush(stderr);
  #ifndef WIN32
  				pqsignal(SIGPIPE, SIG_IGN);
  #endif
+ 				errno = 0;
  				copystream = popen(options->file, PG_BINARY_W);
  			}
  			else
diff --git a/src/common/exec.c b/src/common/exec.c
index 037bef2..6547896 100644
*** a/src/common/exec.c
--- b/src/common/exec.c
*************** pipe_read_line(char *cmd, char *line, in
*** 350,356 ****
  #ifndef WIN32
  	FILE	   *pgver;
  
! 	/* flush output buffers in case popen does not... */
  	fflush(stdout);
  	fflush(stderr);
  
--- 350,356 ----
  #ifndef WIN32
  	FILE	   *pgver;
  
! 	/* Don't use fflush(NULL); see comments in fork_process.c */
  	fflush(stdout);
  	fflush(stderr);
  
diff --git a/src/test/regress/pg_regress.c b/src/test/regress/pg_regress.c
index 07dd803..65b69a8 100644
*** a/src/test/regress/pg_regress.c
--- b/src/test/regress/pg_regress.c
*************** stop_postmaster(void)
*** 288,300 ****
  		char		buf[MAXPGPATH * 2];
  		int			r;
  
- 		/* On Windows, system() seems not to force fflush, so... */
- 		fflush(stdout);
- 		fflush(stderr);
- 
  		snprintf(buf, sizeof(buf),
  				 SYSTEMQUOTE "\"%s/pg_ctl\" stop -D \"%s/data\" -s -m fast" SYSTEMQUOTE,
  				 bindir, temp_install);
  		r = system(buf);
  		if (r != 0)
  		{
--- 288,301 ----
  		char		buf[MAXPGPATH * 2];
  		int			r;
  
  		snprintf(buf, sizeof(buf),
  				 SYSTEMQUOTE "\"%s/pg_ctl\" stop -D \"%s/data\" -s -m fast" SYSTEMQUOTE,
  				 bindir, temp_install);
+ 
+ 		/* Don't use fflush(NULL); see comments in fork_process.c */
+ 		fflush(stdout);
+ 		fflush(stderr);
+ 
  		r = system(buf);
  		if (r != 0)
  		{
*************** spawn_process(const char *cmdline)
*** 929,938 ****
  #ifndef WIN32
  	pid_t		pid;
  
! 	/*
! 	 * Must flush I/O buffers before fork.	Ideally we'd use fflush(NULL) here
! 	 * ... does anyone still care about systems where that doesn't work?
! 	 */
  	fflush(stdout);
  	fflush(stderr);
  	if (logfile)
--- 930,936 ----
  #ifndef WIN32
  	pid_t		pid;
  
! 	/* Don't use fflush(NULL); see comments in fork_process.c */
  	fflush(stdout);
  	fflush(stderr);
  	if (logfile)
-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Reply via email to