From: Tom Lane <tgl@sss.pgh.pa.us>
To: Noah Misch <noah@leadboat.com>
Cc: pgsql-hackers@postgreSQL.org
Subject: Re: Obsolete coding in fork_process.c
Date: Thu, 01 May 2014 17:59:08 -0400
Message-ID: <28165.1398981548@sss.pgh.pa.us> (raw)
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>
List-Unsubscribe: <mailto:majordomo@postgresql.org?body=unsub%20pgsql-hackers>
I wrote:
> Noah Misch <noah@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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackersAttachments:
[text/x-diff] fflush-null-update.patch (9.0K, ../28165.1398981548@sss.pgh.pa.us/2-fflush-null-update.patch)
download | inline diff:diff --git a/contrib/pg_upgrade/controldata.c b/contrib/pg_upgrade/controldata.cindex 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.cindex 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.cindex 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.cindex 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.cindex 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.cindex 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.cindex 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.cindex 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.cindex 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.cindex 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.cindex 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.cindex 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)
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: pgsql-hackers@postgresql.org
Cc: tgl@sss.pgh.pa.us, noah@leadboat.com
Subject: Re: Obsolete coding in fork_process.c
In-Reply-To: <28165.1398981548@sss.pgh.pa.us>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox