Re: Obsolete coding in fork_process.c

On Thu, May 1, 2014 at 5:59 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 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

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