Re: Cleaning up historical portability baggage - Mailing list pgsql-hackers

Here's another bit of baggage handling: fixing up the places that
were afraid to use fflush(NULL).  We could doubtless have done
this years ago (indeed, I found several places already using it)
but as long as we're making a push to get rid of obsolete code,
doing it now seems appropriate.

One thing that's not clear to me is what the appropriate rules
should be for popen().  POSIX makes clear that you shouldn't
expect popen() to include an fflush() itself, but we seem quite
haphazard about whether to do one or not before popen().  In
the case of popen(..., "r") we can expect that the child can't
write on our stdout, but stderr could be a problem anyway.

Likewise, there are some places that fflush before system(),
but they are a minority.  Again it seems like the main risk
is duplicated or mis-ordered stderr output.

I'm inclined to add fflush(NULL) before any popen() or system()
that hasn't got one already, but did not do that in the attached.

Thoughts?

            regards, tom lane

diff --git a/src/backend/postmaster/fork_process.c b/src/backend/postmaster/fork_process.c
index c75be03d2c..ec67761487 100644
--- a/src/backend/postmaster/fork_process.c
+++ b/src/backend/postmaster/fork_process.c
@@ -37,13 +37,8 @@ fork_process(void)

     /*
      * 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.
      */
-    fflush(stdout);
-    fflush(stderr);
+    fflush(NULL);

 #ifdef LINUX_PROFILE

diff --git a/src/backend/storage/file/fd.c b/src/backend/storage/file/fd.c
index e3b19ca1ed..1aaab6c554 100644
--- a/src/backend/storage/file/fd.c
+++ b/src/backend/storage/file/fd.c
@@ -2503,8 +2503,7 @@ OpenPipeStream(const char *command, const char *mode)
     ReleaseLruFiles();

 TryAgain:
-    fflush(stdout);
-    fflush(stderr);
+    fflush(NULL);
     pqsignal(SIGPIPE, SIG_DFL);
     errno = 0;
     file = popen(command, mode);
diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c
index 95f32de4e2..cb3c289889 100644
--- a/src/backend/utils/error/elog.c
+++ b/src/backend/utils/error/elog.c
@@ -643,8 +643,7 @@ errfinish(const char *filename, int lineno, const char *funcname)
          * Any other code you might be tempted to add here should probably be
          * in an on_proc_exit or on_shmem_exit callback instead.
          */
-        fflush(stdout);
-        fflush(stderr);
+        fflush(NULL);

         /*
          * Let the cumulative stats system know. Only mark the session as
@@ -670,8 +669,7 @@ errfinish(const char *filename, int lineno, const char *funcname)
          * XXX: what if we are *in* the postmaster?  abort() won't kill our
          * children...
          */
-        fflush(stdout);
-        fflush(stderr);
+        fflush(NULL);
         abort();
     }

diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c
index 29c28b7315..8567c875fe 100644
--- a/src/bin/initdb/initdb.c
+++ b/src/bin/initdb/initdb.c
@@ -489,8 +489,7 @@ popen_check(const char *command, const char *mode)
 {
     FILE       *cmdfd;

-    fflush(stdout);
-    fflush(stderr);
+    fflush(NULL);
     errno = 0;
     cmdfd = popen(command, mode);
     if (cmdfd == NULL)
diff --git a/src/bin/pg_ctl/pg_ctl.c b/src/bin/pg_ctl/pg_ctl.c
index 73e20081d1..be2af9f261 100644
--- a/src/bin/pg_ctl/pg_ctl.c
+++ b/src/bin/pg_ctl/pg_ctl.c
@@ -448,8 +448,7 @@ start_postmaster(void)
     pgpid_t        pm_pid;

     /* Flush stdio channels just before fork, to avoid double-output problems */
-    fflush(stdout);
-    fflush(stderr);
+    fflush(NULL);

 #ifdef EXEC_BACKEND
     pg_disable_aslr();
diff --git a/src/bin/pg_dump/pg_dumpall.c b/src/bin/pg_dump/pg_dumpall.c
index 68c455f84b..d665b257c9 100644
--- a/src/bin/pg_dump/pg_dumpall.c
+++ b/src/bin/pg_dump/pg_dumpall.c
@@ -1578,8 +1578,7 @@ runPgDump(const char *dbname, const char *create_opts)

     pg_log_info("running \"%s\"", cmd->data);

-    fflush(stdout);
-    fflush(stderr);
+    fflush(NULL);

     ret = system(cmd->data);

diff --git a/src/bin/pg_upgrade/controldata.c b/src/bin/pg_upgrade/controldata.c
index e2086a07de..018cd310f7 100644
--- a/src/bin/pg_upgrade/controldata.c
+++ b/src/bin/pg_upgrade/controldata.c
@@ -123,8 +123,7 @@ get_control_data(ClusterInfo *cluster, bool live_check)
         /* only pg_controldata outputs the cluster state */
         snprintf(cmd, sizeof(cmd), "\"%s/pg_controldata\" \"%s\"",
                  cluster->bindir, cluster->pgdata);
-        fflush(stdout);
-        fflush(stderr);
+        fflush(NULL);

         if ((output = popen(cmd, "r")) == NULL)
             pg_fatal("could not get control data using %s: %s",
@@ -191,8 +190,7 @@ get_control_data(ClusterInfo *cluster, bool live_check)
              cluster->bindir,
              live_check ? "pg_controldata\"" : resetwal_bin,
              cluster->pgdata);
-    fflush(stdout);
-    fflush(stderr);
+    fflush(NULL);

     if ((output = popen(cmd, "r")) == NULL)
         pg_fatal("could not get control data using %s: %s",
diff --git a/src/bin/psql/copy.c b/src/bin/psql/copy.c
index c181682a13..0f66ebc2ed 100644
--- a/src/bin/psql/copy.c
+++ b/src/bin/psql/copy.c
@@ -288,8 +288,7 @@ do_copy(const char *args)
         {
             if (options->program)
             {
-                fflush(stdout);
-                fflush(stderr);
+                fflush(NULL);
                 errno = 0;
                 copystream = popen(options->file, PG_BINARY_R);
             }
@@ -307,10 +306,9 @@ do_copy(const char *args)
         {
             if (options->program)
             {
-                fflush(stdout);
-                fflush(stderr);
-                errno = 0;
+                fflush(NULL);
                 disable_sigpipe_trap();
+                errno = 0;
                 copystream = popen(options->file, PG_BINARY_W);
             }
             else
diff --git a/src/common/exec.c b/src/common/exec.c
index fdcad0ee8c..81ff86dce6 100644
--- a/src/common/exec.c
+++ b/src/common/exec.c
@@ -389,8 +389,7 @@ pipe_read_line(char *cmd, char *line, int maxsize)
     FILE       *pgver;

     /* flush output buffers in case popen does not... */
-    fflush(stdout);
-    fflush(stderr);
+    fflush(NULL);

     errno = 0;
     if ((pgver = popen(cmd, "r")) == NULL)
diff --git a/src/test/regress/pg_regress.c b/src/test/regress/pg_regress.c
index a803355f8e..3c92aa8836 100644
--- a/src/test/regress/pg_regress.c
+++ b/src/test/regress/pg_regress.c
@@ -264,8 +264,7 @@ stop_postmaster(void)
         int            r;

         /* On Windows, system() seems not to force fflush, so... */
-        fflush(stdout);
-        fflush(stderr);
+        fflush(NULL);

         snprintf(buf, sizeof(buf),
                  "\"%s%spg_ctl\" stop -D \"%s/data\" -s",
@@ -1063,13 +1062,9 @@ spawn_process(const char *cmdline)
     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?
+     * Must flush I/O buffers before fork.
      */
-    fflush(stdout);
-    fflush(stderr);
-    if (logfile)
-        fflush(logfile);
+    fflush(NULL);

 #ifdef EXEC_BACKEND
     pg_disable_aslr();