I wrote:
> Noah Misch <[email protected]> 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 ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
