Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtp (Exim 4.80) (envelope-from ) id 1Wfz0e-00084T-Dw for pgsql-hackers@arkaria.postgresql.org; Thu, 01 May 2014 21:59:16 +0000 Received: from localhost ([127.0.0.1] helo=postgresql.org) by malur.postgresql.org with smtp (Exim 4.80) (envelope-from ) id 1Wfz0d-0007dc-SW for pgsql-hackers@arkaria.postgresql.org; Thu, 01 May 2014 21:59:15 +0000 Received: from makus.postgresql.org ([2001:4800:7903:4::125]) by malur.postgresql.org with esmtp (Exim 4.80) (envelope-from ) id 1Wfz0c-0007bx-8F for pgsql-hackers@postgreSQL.org; Thu, 01 May 2014 21:59:14 +0000 Received: from sss.pgh.pa.us ([66.207.139.130]) by makus.postgresql.org with esmtp (Exim 4.80) (envelope-from ) id 1Wfz0Y-0005m3-F3 for pgsql-hackers@postgreSQL.org; Thu, 01 May 2014 21:59:13 +0000 Received: from sss1.sss.pgh.pa.us (localhost [127.0.0.1]) by sss.pgh.pa.us (8.14.4/8.14.4) with ESMTP id s41Lx8jL028166; Thu, 1 May 2014 17:59:09 -0400 From: Tom Lane To: Noah Misch cc: pgsql-hackers@postgreSQL.org Subject: Re: Obsolete coding in fork_process.c In-reply-to: <4013.1398971788@sss.pgh.pa.us> References: <30040.1398960808@sss.pgh.pa.us> <20140501183231.GA1190179@tornado.leadboat.com> <4013.1398971788@sss.pgh.pa.us> Comments: In-reply-to Tom Lane message dated "Thu, 01 May 2014 15:16:28 -0400" MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0" Content-ID: <28023.1398981149.0@sss.pgh.pa.us> Date: Thu, 01 May 2014 17:59:08 -0400 Message-ID: <28165.1398981548@sss.pgh.pa.us> X-Pg-Spam-Score: -2.6 (--) List-Archive: List-Help: List-ID: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: X-Mailing-List: pgsql-hackers Precedence: bulk Sender: pgsql-hackers-owner@postgresql.org ------- =_aaaaaaaaaa0 Content-Type: text/plain; charset="us-ascii" Content-ID: <28023.1398981149.1@sss.pgh.pa.us> I wrote: > Noah Misch 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 ------- =_aaaaaaaaaa0 Content-Type: text/x-diff; name="fflush-null-update.patch"; charset="us-ascii" Content-ID: <28023.1398981149.2@sss.pgh.pa.us> Content-Description: fflush-null-update.patch Content-Transfer-Encoding: quoted-printable diff --git a/contrib/pg_upgrade/controldata.c b/contrib/pg_upgrade/controld= ata.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); +=20 + /* Don't use fflush(NULL); see comments in fork_process.c */ fflush(stdout); fflush(stderr); =20=20 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++; =20=20 ! /* Ensure stdio state is quiesced before forking */ ! fflush(NULL); =20=20 #ifndef WIN32 child =3D fork(); --- 118,126 ---- /* set this before we start the job */ parallel_jobs++; =20=20 ! /* Don't use fflush(NULL); see comments in fork_process.c */ ! fflush(stdout); ! fflush(stderr); =20=20 #ifndef WIN32 child =3D fork(); *************** parallel_transfer_all_new_dbs(DbInfoArr=20 *** 227,234 **** /* set this before we start the job */ parallel_jobs++; =20=20 ! /* Ensure stdio state is quiesced before forking */ ! fflush(NULL); =20=20 #ifndef WIN32 child =3D fork(); --- 228,236 ---- /* set this before we start the job */ parallel_jobs++; =20=20 ! /* Don't use fflush(NULL); see comments in fork_process.c */ ! fflush(stdout); ! fflush(stderr); =20=20 #ifndef WIN32 child =3D 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; } =20=20 + /* Don't use fflush(NULL); see comments in fork_process.c */ + fflush(stdout); + fflush(stderr); +=20 th->pid =3D fork(); if (th->pid =3D=3D -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 **** =20=20 /* * Flush stdio channels just before fork, to avoid double-output problem= s. ! * 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 ---- =20=20 /* * Flush stdio channels just before fork, to avoid double-output problem= s. ! * ! * 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(); =20=20 TryAgain: + /* Don't use fflush(NULL); see comments in fork_process.c */ fflush(stdout); fflush(stderr); errno =3D 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; =20=20 + /* Don't use fflush(NULL); see comments in fork_process.c */ fflush(stdout); fflush(stderr); errno =3D 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=20 *** 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); +=20 bgchild =3D fork(); if (bgchild =3D=3D 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 **** =20=20 Assert(AH->public.numWorkers > 0); =20=20 - /* Ensure stdio state is quiesced before forking */ - fflush(NULL); -=20 pstate =3D (ParallelState *) pg_malloc(sizeof(ParallelState)); =20=20 pstate->numWorkers =3D AH->public.numWorkers; --- 489,494 ---- *************** ParallelBackupStart(ArchiveHandle *AH, R *** 539,544 **** --- 536,542 ---- pstate->parallelSlot[i].args =3D (ParallelArgs *) pg_malloc(sizeof(Para= llelArgs)); pstate->parallelSlot[i].args->AH =3D NULL; pstate->parallelSlot[i].args->te =3D NULL; +=20 #ifdef WIN32 /* Allocate a new structure for every worker */ wi =3D (WorkerInfo *) pg_malloc(sizeof(WorkerInfo)); *************** ParallelBackupStart(ArchiveHandle *AH, R *** 553,558 **** --- 551,561 ---- wi, 0, &(pstate->parallelSlot[i].threadId)); pstate->parallelSlot[i].hThread =3D handle; #else +=20 + /* Don't use fflush(NULL); see comments in fork_process.c */ + fflush(stdout); + fflush(stderr); +=20 pid =3D fork(); if (pid =3D=3D 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); =20=20 + /* Don't use fflush(NULL); see comments in fork_process.c */ fflush(stdout); fflush(stderr); =20=20 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 =3D 0; *************** do_copy(const char *args) *** 307,318 **** { if (options->program) { fflush(stdout); fflush(stderr); - errno =3D 0; #ifndef WIN32 pqsignal(SIGPIPE, SIG_IGN); #endif copystream =3D 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 =3D 0; copystream =3D 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; =20=20 ! /* flush output buffers in case popen does not... */ fflush(stdout); fflush(stderr); =20=20 --- 350,356 ---- #ifndef WIN32 FILE *pgver; =20=20 ! /* Don't use fflush(NULL); see comments in fork_process.c */ fflush(stdout); fflush(stderr); =20=20 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; =20=20 - /* On Windows, system() seems not to force fflush, so... */ - fflush(stdout); - fflush(stderr); -=20 snprintf(buf, sizeof(buf), SYSTEMQUOTE "\"%s/pg_ctl\" stop -D \"%s/data\" -s -m fast" SYSTEMQUO= TE, bindir, temp_install); r =3D system(buf); if (r !=3D 0) { --- 288,301 ---- char buf[MAXPGPATH * 2]; int r; =20=20 snprintf(buf, sizeof(buf), SYSTEMQUOTE "\"%s/pg_ctl\" stop -D \"%s/data\" -s -m fast" SYSTEMQUO= TE, bindir, temp_install); +=20 + /* Don't use fflush(NULL); see comments in fork_process.c */ + fflush(stdout); + fflush(stderr); +=20 r =3D system(buf); if (r !=3D 0) { *************** spawn_process(const char *cmdline) *** 929,938 **** #ifndef WIN32 pid_t pid; =20=20 ! /* ! * 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; =20=20 ! /* Don't use fflush(NULL); see comments in fork_process.c */ fflush(stdout); fflush(stderr); if (logfile) ------- =_aaaaaaaaaa0 Content-Type: text/plain Content-Disposition: inline Content-Transfer-Encoding: 8bit MIME-Version: 1.0 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers ------- =_aaaaaaaaaa0--