Re: [HACKERS] Obsolete coding in fork_process.c
On Thu, May 01, 2014 at 11:07:51PM -0400, Tom Lane wrote: Noah Misch n...@leadboat.com writes: On Thu, May 01, 2014 at 08:44:46PM -0400, Tom Lane wrote: You're only considering one aspect of the problem. Yeah, you might not get duplicated output unless system() prints something before exec(), but we would also like to have causality: that is, whatever we sent to stdout before calling system() should appear there before anything the child process sends to stdout. Good point. I suppose a couple of fflush() calls have negligible cost next to a system() or popen(). Introduce pg_popen()/pg_system(), and adopt a rule that they are [almost] our only callers of raw popen()/system()? Meh. I'm not usually in favor of adopting nonstandard notation, and this doesn't seem like a place to start. In particular, if you don't want to use fflush(NULL) in these proposed wrappers, then call sites are still going to have an issue with needing to do manual fflushes; pg_regress.c's spawn_process is an example: /* * 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) fflush(logfile); pid = fork(); I think that removing the need for fflush(stdout) and fflush(stderr) in this context would mostly result in people forgetting to fflush other output files. I'd rather have the two lines of boilerplate (and a comment about why we're refusing to depend on fflush(NULL)) than take that risk. Works for me. -- Noah Misch EnterpriseDB http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Obsolete coding in fork_process.c
fork_process.c quoth: /* * 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); Is there any reason not to change this to just fflush(NULL)? We dropped support for SunOS 4.1 quite some time ago ... While it's still true that the postmaster proper doesn't need to fflush anything but stdout and stderr, this coding seems a bit less than safe when you consider the possibility of third-party libraries loaded into the postmaster process. 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-hackers
Re: [HACKERS] Obsolete coding in fork_process.c
On Thu, May 01, 2014 at 12:13:28PM -0400, Tom Lane wrote: fork_process.c quoth: /* * 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); 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 -- Noah Misch EnterpriseDB http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Obsolete coding in fork_process.c
Noah Misch n...@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. 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-hackers
Re: [HACKERS] Obsolete coding in fork_process.c
I wrote:
Noah Misch n...@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
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,
--- 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
Re: [HACKERS] Obsolete coding in fork_process.c
On Thu, May 1, 2014 at 5:59 PM, Tom Lane t...@sss.pgh.pa.us wrote: I wrote: Noah Misch n...@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 I am having a hard time understanding what the real impetus to change this is. IIUC, we have zero reports of the current coding being a problem, so I'm not sure why we want to go make it different. Sure, there are hypothetical problems with the current code, but there are also hypothetical problems with the proposed change, and the current code has survived quite a bit of real-world deployment. I guess it's hard for me to be dead-set against this if we're using that pattern elsewhere, but I won't be very surprised if more weird cases where it doesn't work turn up down the road; and I'm a bit worried that if we let it proliferate we'll find it hard to get rid of if and when those reports turn up. 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? I think that would probably be a good idea. I wouldn't be shocked if there are problems there that we've failed to notice. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Obsolete coding in fork_process.c
On Thu, May 01, 2014 at 05:59:08PM -0400, Tom Lane wrote: I wrote: Noah Misch n...@leadboat.com writes: 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. Do those programs, operating in those modes, read from stdin or some other long-lived, pipe-backed FILE*? 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? You need the fflush() when forking and then using stdio in the child before any exec(). Have you caught wind of any system() or popen() implementation having that property? -- Noah Misch EnterpriseDB http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Obsolete coding in fork_process.c
Noah Misch n...@leadboat.com writes: On Thu, May 01, 2014 at 05:59:08PM -0400, Tom Lane wrote: 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. Do those programs, operating in those modes, read from stdin or some other long-lived, pipe-backed FILE*? Probably not. However, if that's your criterion, I'd say that I'd much rather assume that the postmaster doesn't have a pipe connected to stdin than that it has no stdio outputs besides stdout/stderr. 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? You need the fflush() when forking and then using stdio in the child before any exec(). Have you caught wind of any system() or popen() implementation having that property? You're only considering one aspect of the problem. Yeah, you might not get duplicated output unless system() prints something before exec(), but we would also like to have causality: that is, whatever we sent to stdout before calling system() should appear there before anything the child process sends to stdout. 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-hackers
Re: [HACKERS] Obsolete coding in fork_process.c
On Thu, May 01, 2014 at 08:44:46PM -0400, Tom Lane wrote: Noah Misch n...@leadboat.com writes: On Thu, May 01, 2014 at 05:59:08PM -0400, Tom Lane wrote: 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. Do those programs, operating in those modes, read from stdin or some other long-lived, pipe-backed FILE*? Probably not. Then the use of fflush(NULL) in those programs tells us nothing about the status of the aforementioned HP-UX bug. However, if that's your criterion, I'd say that I'd much rather assume that the postmaster doesn't have a pipe connected to stdin than that it has no stdio outputs besides stdout/stderr. Either way, the beneficiary is theoretical; who can tell which one deserves priority? Let's do what we usually do in the absence of a clear improvement: keep the longstanding behavior. 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? You need the fflush() when forking and then using stdio in the child before any exec(). Have you caught wind of any system() or popen() implementation having that property? You're only considering one aspect of the problem. Yeah, you might not get duplicated output unless system() prints something before exec(), but we would also like to have causality: that is, whatever we sent to stdout before calling system() should appear there before anything the child process sends to stdout. Good point. I suppose a couple of fflush() calls have negligible cost next to a system() or popen(). Introduce pg_popen()/pg_system(), and adopt a rule that they are [almost] our only callers of raw popen()/system()? -- Noah Misch EnterpriseDB http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Obsolete coding in fork_process.c
Noah Misch n...@leadboat.com writes: On Thu, May 01, 2014 at 08:44:46PM -0400, Tom Lane wrote: You're only considering one aspect of the problem. Yeah, you might not get duplicated output unless system() prints something before exec(), but we would also like to have causality: that is, whatever we sent to stdout before calling system() should appear there before anything the child process sends to stdout. Good point. I suppose a couple of fflush() calls have negligible cost next to a system() or popen(). Introduce pg_popen()/pg_system(), and adopt a rule that they are [almost] our only callers of raw popen()/system()? Meh. I'm not usually in favor of adopting nonstandard notation, and this doesn't seem like a place to start. In particular, if you don't want to use fflush(NULL) in these proposed wrappers, then call sites are still going to have an issue with needing to do manual fflushes; pg_regress.c's spawn_process is an example: /* * 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) fflush(logfile); pid = fork(); I think that removing the need for fflush(stdout) and fflush(stderr) in this context would mostly result in people forgetting to fflush other output files. I'd rather have the two lines of boilerplate (and a comment about why we're refusing to depend on fflush(NULL)) than take that risk. Now, if you were willing to put fflush(NULL) into the wrappers, I could get on board with 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-hackers
