From 7fed801135bae14d63b11ee4a10f6083767046d8 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Mon, 29 Aug 2022 13:55:38 -0400 Subject: [PATCH] Clean up inconsistent use of fflush(). More than twenty years ago (79fcde48b), we hacked the postmaster to avoid a core-dump on systems that didn't support fflush(NULL). We've mostly, though not completely, hewed to that rule ever since. But such systems are surely gone in the wild, so in the spirit of cleaning out no-longer-needed portability hacks let's get rid of multiple per-file fflush() calls in favor of using fflush(NULL). Also, we were fairly inconsistent about whether to fflush() before popen() and system() calls. While we've received no bug reports about that, it seems likely that at least some of these call sites are at risk of odd behavior, such as error messages appearing in an unexpected order. Rather than expend a lot of brain cells figuring out which places are at hazard, let's just establish a uniform coding rule that we should fflush(NULL) before these calls. A no-op fflush() is surely of trivial cost compared to launching a sub-process via a shell; while if it's not a no-op then we likely need it. Discussion: https://postgr.es/m/2923412.1661722825@sss.pgh.pa.us --- src/backend/access/transam/xlogarchive.c | 2 ++ src/backend/postmaster/fork_process.c | 7 +------ src/backend/postmaster/shell_archive.c | 1 + src/backend/storage/file/fd.c | 3 +-- src/backend/utils/error/elog.c | 6 ++---- src/bin/initdb/initdb.c | 5 +++-- src/bin/pg_ctl/pg_ctl.c | 5 +++-- src/bin/pg_dump/pg_dumpall.c | 3 +-- src/bin/pg_rewind/pg_rewind.c | 1 + src/bin/pg_upgrade/controldata.c | 6 ++---- src/bin/pg_upgrade/exec.c | 7 +++++++ src/bin/pg_upgrade/option.c | 1 + src/bin/pg_verifybackup/pg_verifybackup.c | 1 + src/bin/pgbench/pgbench.c | 2 ++ src/bin/psql/command.c | 4 ++++ src/bin/psql/common.c | 1 + src/bin/psql/copy.c | 8 +++----- src/bin/psql/prompt.c | 4 +++- src/bin/psql/psqlscanslash.l | 1 + src/common/exec.c | 4 +--- src/fe_utils/archive.c | 1 + src/fe_utils/print.c | 1 + src/interfaces/libpq/fe-print.c | 1 + src/test/regress/pg_regress.c | 18 ++++++++---------- 24 files changed, 52 insertions(+), 41 deletions(-) diff --git a/src/backend/access/transam/xlogarchive.c b/src/backend/access/transam/xlogarchive.c index 4101a30e374..e2b7176f2f6 100644 --- a/src/backend/access/transam/xlogarchive.c +++ b/src/backend/access/transam/xlogarchive.c @@ -169,6 +169,7 @@ RestoreArchivedFile(char *path, const char *xlogfname, /* * Copy xlog from archival storage to XLOGDIR */ + fflush(NULL); pgstat_report_wait_start(WAIT_EVENT_RESTORE_COMMAND); rc = system(xlogRestoreCmd); pgstat_report_wait_end(); @@ -358,6 +359,7 @@ ExecuteRecoveryCommand(const char *command, const char *commandName, /* * execute the constructed command */ + fflush(NULL); pgstat_report_wait_start(wait_event_info); rc = system(xlogRecoveryCmd); pgstat_report_wait_end(); diff --git a/src/backend/postmaster/fork_process.c b/src/backend/postmaster/fork_process.c index c75be03d2c3..ec677614870 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/postmaster/shell_archive.c b/src/backend/postmaster/shell_archive.c index 19e240c2053..8a54d02e7bb 100644 --- a/src/backend/postmaster/shell_archive.c +++ b/src/backend/postmaster/shell_archive.c @@ -99,6 +99,7 @@ shell_archive_file(const char *file, const char *path) (errmsg_internal("executing archive command \"%s\"", xlogarchcmd))); + fflush(NULL); pgstat_report_wait_start(WAIT_EVENT_ARCHIVE_COMMAND); rc = system(xlogarchcmd); pgstat_report_wait_end(); diff --git a/src/backend/storage/file/fd.c b/src/backend/storage/file/fd.c index e3b19ca1ed4..1aaab6c5542 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 95f32de4e29..cb3c2898898 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 29c28b73153..e00837ecacf 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) @@ -914,6 +913,7 @@ test_config_settings(void) test_conns, test_buffs, dynamic_shared_memory_type, DEVNULL, DEVNULL); + fflush(NULL); status = system(cmd); if (status == 0) { @@ -950,6 +950,7 @@ test_config_settings(void) n_connections, test_buffs, dynamic_shared_memory_type, DEVNULL, DEVNULL); + fflush(NULL); status = system(cmd); if (status == 0) break; diff --git a/src/bin/pg_ctl/pg_ctl.c b/src/bin/pg_ctl/pg_ctl.c index 73e20081d1d..d7ff3902aa7 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(); @@ -916,6 +915,7 @@ do_init(void) cmd = psprintf("\"%s\" %s%s > \"%s\"", exec_path, pgdata_opt, post_opts, DEVNULL); + fflush(NULL); if (system(cmd) != 0) { write_stderr(_("%s: database system initialization failed\n"), progname); @@ -2222,6 +2222,7 @@ adjust_data_dir(void) my_exec_path, pgdata_opt ? pgdata_opt : "", post_opts ? post_opts : ""); + fflush(NULL); fd = popen(cmd, "r"); if (fd == NULL || fgets(filename, sizeof(filename), fd) == NULL) diff --git a/src/bin/pg_dump/pg_dumpall.c b/src/bin/pg_dump/pg_dumpall.c index 68c455f84b2..d665b257c93 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_rewind/pg_rewind.c b/src/bin/pg_rewind/pg_rewind.c index 1ff8da16764..3cd77c09b1a 100644 --- a/src/bin/pg_rewind/pg_rewind.c +++ b/src/bin/pg_rewind/pg_rewind.c @@ -1151,6 +1151,7 @@ ensureCleanShutdown(const char *argv0) appendPQExpBufferStr(postgres_cmd, " template1 < "); appendShellString(postgres_cmd, DEVNULL); + fflush(NULL); if (system(postgres_cmd->data) != 0) { pg_log_error("postgres single-user mode in target cluster failed"); diff --git a/src/bin/pg_upgrade/controldata.c b/src/bin/pg_upgrade/controldata.c index e2086a07dea..018cd310f7c 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/pg_upgrade/exec.c b/src/bin/pg_upgrade/exec.c index aa3d2f88ed8..60f4b5443e6 100644 --- a/src/bin/pg_upgrade/exec.c +++ b/src/bin/pg_upgrade/exec.c @@ -39,6 +39,7 @@ get_bin_version(ClusterInfo *cluster) v2 = 0; snprintf(cmd, sizeof(cmd), "\"%s/pg_ctl\" --version", cluster->bindir); + fflush(NULL); if ((output = popen(cmd, "r")) == NULL || fgets(cmd_output, sizeof(cmd_output), output) == NULL) @@ -125,7 +126,10 @@ exec_prog(const char *log_filename, const char *opt_log_file, * the file do not see to help. */ if (mainThreadId != GetCurrentThreadId()) + { + fflush(NULL); result = system(cmd); + } #endif log = fopen(log_file, "a"); @@ -174,7 +178,10 @@ exec_prog(const char *log_filename, const char *opt_log_file, /* see comment above */ if (mainThreadId == GetCurrentThreadId()) #endif + { + fflush(NULL); result = system(cmd); + } if (result != 0 && report_error) { diff --git a/src/bin/pg_upgrade/option.c b/src/bin/pg_upgrade/option.c index fbab1c4fb72..3f719f1762c 100644 --- a/src/bin/pg_upgrade/option.c +++ b/src/bin/pg_upgrade/option.c @@ -416,6 +416,7 @@ adjust_data_dir(ClusterInfo *cluster) */ snprintf(cmd, sizeof(cmd), "\"%s/postgres\" -D \"%s\" -C data_directory", cluster->bindir, cluster->pgconfig); + fflush(NULL); if ((output = popen(cmd, "r")) == NULL || fgets(cmd_output, sizeof(cmd_output), output) == NULL) diff --git a/src/bin/pg_verifybackup/pg_verifybackup.c b/src/bin/pg_verifybackup/pg_verifybackup.c index bd18b4491d5..63d02719a61 100644 --- a/src/bin/pg_verifybackup/pg_verifybackup.c +++ b/src/bin/pg_verifybackup/pg_verifybackup.c @@ -811,6 +811,7 @@ parse_required_wal(verifier_context *context, char *pg_waldump_path, pg_waldump_path, wal_directory, this_wal_range->tli, LSN_FORMAT_ARGS(this_wal_range->start_lsn), LSN_FORMAT_ARGS(this_wal_range->end_lsn)); + fflush(NULL); if (system(pg_waldump_cmd) != 0) report_backup_error(context, "WAL parsing failed for timeline %u", diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c index 07b7e0cf37c..9aadcaad71e 100644 --- a/src/bin/pgbench/pgbench.c +++ b/src/bin/pgbench/pgbench.c @@ -2973,6 +2973,8 @@ runShellCommand(Variables *variables, char *variable, char **argv, int argc) command[len] = '\0'; + fflush(NULL); /* needed before either system() or popen() */ + /* Fast path for non-assignment case */ if (variable == NULL) { diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c index a81bd3307b4..61188d96f2a 100644 --- a/src/bin/psql/command.c +++ b/src/bin/psql/command.c @@ -2661,6 +2661,7 @@ exec_command_write(PsqlScanState scan_state, bool active_branch, if (fname[0] == '|') { is_pipe = true; + fflush(NULL); disable_sigpipe_trap(); fd = popen(&fname[1], "w"); } @@ -3834,6 +3835,7 @@ editFile(const char *fname, int lineno) sys = psprintf("\"%s\" \"%s\"", editorName, fname); #endif + fflush(NULL); result = system(sys); if (result == -1) pg_log_error("could not start editor \"%s\"", editorName); @@ -4956,6 +4958,7 @@ do_shell(const char *command) { int result; + fflush(NULL); if (!command) { char *sys; @@ -5065,6 +5068,7 @@ do_watch(PQExpBuffer query_buf, double sleep) #endif if (pagerprog && myopt.topt.pager) { + fflush(NULL); disable_sigpipe_trap(); pagerpipe = popen(pagerprog, "w"); diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c index 9f95869eca6..e611e3266d0 100644 --- a/src/bin/psql/common.c +++ b/src/bin/psql/common.c @@ -59,6 +59,7 @@ openQueryOutputFile(const char *fname, FILE **fout, bool *is_pipe) } else if (*fname == '|') { + fflush(NULL); *fout = popen(fname + 1, "w"); *is_pipe = true; } diff --git a/src/bin/psql/copy.c b/src/bin/psql/copy.c index c181682a132..0f66ebc2edf 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/bin/psql/prompt.c b/src/bin/psql/prompt.c index 509e6422b7e..3955d72206c 100644 --- a/src/bin/psql/prompt.c +++ b/src/bin/psql/prompt.c @@ -266,8 +266,10 @@ get_prompt(promptStatus_t status, ConditionalStack cstack) { int cmdend = strcspn(p + 1, "`"); char *file = pnstrdup(p + 1, cmdend); - FILE *fd = popen(file, "r"); + FILE *fd; + fflush(NULL); + fd = popen(file, "r"); if (fd) { if (fgets(buf, sizeof(buf), fd) == NULL) diff --git a/src/bin/psql/psqlscanslash.l b/src/bin/psql/psqlscanslash.l index 878da9f48e9..a467b721449 100644 --- a/src/bin/psql/psqlscanslash.l +++ b/src/bin/psql/psqlscanslash.l @@ -777,6 +777,7 @@ evaluate_backtick(PsqlScanState state) initPQExpBuffer(&cmd_output); + fflush(NULL); fd = popen(cmd, "r"); if (!fd) { diff --git a/src/common/exec.c b/src/common/exec.c index fdcad0ee8cf..22f04aafbec 100644 --- a/src/common/exec.c +++ b/src/common/exec.c @@ -388,9 +388,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/fe_utils/archive.c b/src/fe_utils/archive.c index 53d42c2be41..fda93036615 100644 --- a/src/fe_utils/archive.c +++ b/src/fe_utils/archive.c @@ -55,6 +55,7 @@ RestoreArchivedFile(const char *path, const char *xlogfname, * Execute restore_command, which should copy the missing file from * archival storage. */ + fflush(NULL); rc = system(xlogRestoreCmd); pfree(xlogRestoreCmd); diff --git a/src/fe_utils/print.c b/src/fe_utils/print.c index a48c9196978..55288f8876c 100644 --- a/src/fe_utils/print.c +++ b/src/fe_utils/print.c @@ -3118,6 +3118,7 @@ PageOutput(int lines, const printTableOpt *topt) if (strspn(pagerprog, " \t\r\n") == strlen(pagerprog)) return stdout; } + fflush(NULL); disable_sigpipe_trap(); pagerpipe = popen(pagerprog, "w"); if (pagerpipe) diff --git a/src/interfaces/libpq/fe-print.c b/src/interfaces/libpq/fe-print.c index 783cd9b756f..21d346a08b4 100644 --- a/src/interfaces/libpq/fe-print.c +++ b/src/interfaces/libpq/fe-print.c @@ -180,6 +180,7 @@ PQprint(FILE *fout, const PGresult *res, const PQprintOpt *po) - (po->header != 0) * 2 /* row count and newline */ ))) { + fflush(NULL); fout = popen(pagerenv, "w"); if (fout) { diff --git a/src/test/regress/pg_regress.c b/src/test/regress/pg_regress.c index a803355f8ee..7290948eeee 100644 --- a/src/test/regress/pg_regress.c +++ b/src/test/regress/pg_regress.c @@ -263,15 +263,12 @@ stop_postmaster(void) char buf[MAXPGPATH * 2]; int r; - /* On Windows, system() seems not to force fflush, so... */ - fflush(stdout); - fflush(stderr); - snprintf(buf, sizeof(buf), "\"%s%spg_ctl\" stop -D \"%s/data\" -s", bindir ? bindir : "", bindir ? "/" : "", temp_instance); + fflush(NULL); r = system(buf); if (r != 0) { @@ -1029,6 +1026,7 @@ psql_end_command(StringInfo buf, const char *database) database); /* And now we can execute the shell command */ + fflush(NULL); if (system(buf->data) != 0) { /* psql probably already reported the error */ @@ -1063,13 +1061,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(); @@ -1247,6 +1241,7 @@ run_diff(const char *cmd, const char *filename) { int r; + fflush(NULL); r = system(cmd); if (!WIFEXITED(r) || WEXITSTATUS(r) > 1) { @@ -2264,6 +2259,7 @@ regression_main(int argc, char *argv[], debug ? " --debug" : "", nolocale ? " --no-locale" : "", outputdir); + fflush(NULL); if (system(buf)) { fprintf(stderr, _("\n%s: initdb failed\nExamine %s/log/initdb.log for the reason.\nCommand was: %s\n"), progname, outputdir, buf); @@ -2335,6 +2331,7 @@ regression_main(int argc, char *argv[], for (i = 0; i < 16; i++) { + fflush(NULL); if (system(buf2) == 0) { char s[16]; @@ -2398,6 +2395,7 @@ regression_main(int argc, char *argv[], for (i = 0; i < wait_seconds; i++) { /* Done if psql succeeds */ + fflush(NULL); if (system(buf2) == 0) break;