Thread

1. pg_upgrade and system() return value

   Bruce Momjian <bruce@momjian.us> — 2013-01-20T02:46:32Z

   Can someone comment on the attached patch?  pg_upgrade was testing if
   system() returned a non-zero value, while I am thinking I should be
   adjusting system()'s return value with WEXITSTATUS().  
   
   Is there any possible bug in back branches just compariing system()'s
   turn value to non-zero without calling WEXITSTATUS()?  I never saw a bug
   related to this.  I am thinking of applying this just to git head.
   
   -- 
     Bruce Momjian  <bruce@momjian.us>        http://momjian.us
     EnterpriseDB                             http://enterprisedb.com
   
     + It's impossible for everything to be true. +
   

2. Re: pg_upgrade and system() return value

   Tom Lane <tgl@sss.pgh.pa.us> — 2013-01-20T16:14:47Z

   Bruce Momjian <bruce@momjian.us> writes:
   > Can someone comment on the attached patch?  pg_upgrade was testing if
   > system() returned a non-zero value, while I am thinking I should be
   > adjusting system()'s return value with WEXITSTATUS().  
   
   AFAIK it's not very good style to test the result as an integer, and
   testing for -1 is not an improvement on that.  Actually it's a
   disimprovement, because the only case where the standard guarantees
   anything about the integer representation is that zero corresponds
   to "exited with status 0".  See the Single Unix Spec, wherein system's
   result code is defined in terms of wait's, and the wait man page saith
   
   	If and only if the status returned is from a terminated child process
   	that returned 0 from main() or passed 0 as the status argument to
   	_exit() or exit(), the value stored at the location pointed to by
   	stat_loc will be 0. Regardless of its value, this information may be
   	interpreted using the following macros ...
   
   If you want to do something different, then you could test for
   WIFEXITED && WEXITSTATUS == 0.  (Testing the latter alone is flat
   wrong.)  But I'm not particularly convinced that that's an improvement
   on what's there now.  I note that your proposed patch would prevent
   any possibility of printing debug information about failure cases,
   since it loses the original result value.
   
   In short: it's not broken now, but this patch would break it.
   
   			regards, tom lane
   
   
   

3. Re: pg_upgrade and system() return value

   Bruce Momjian <bruce@momjian.us> — 2013-01-21T14:19:30Z

   On Sun, Jan 20, 2013 at 11:14:47AM -0500, Tom Lane wrote:
   > Bruce Momjian <bruce@momjian.us> writes:
   > > Can someone comment on the attached patch?  pg_upgrade was testing if
   > > system() returned a non-zero value, while I am thinking I should be
   > > adjusting system()'s return value with WEXITSTATUS().  
   > 
   > AFAIK it's not very good style to test the result as an integer, and
   > testing for -1 is not an improvement on that.  Actually it's a
   > disimprovement, because the only case where the standard guarantees
   > anything about the integer representation is that zero corresponds
   > to "exited with status 0".  See the Single Unix Spec, wherein system's
   > result code is defined in terms of wait's, and the wait man page saith
   > 
   > 	If and only if the status returned is from a terminated child process
   > 	that returned 0 from main() or passed 0 as the status argument to
   > 	_exit() or exit(), the value stored at the location pointed to by
   > 	stat_loc will be 0. Regardless of its value, this information may be
   > 	interpreted using the following macros ...
   > 
   > If you want to do something different, then you could test for
   > WIFEXITED && WEXITSTATUS == 0.  (Testing the latter alone is flat
   > wrong.)  But I'm not particularly convinced that that's an improvement
   > on what's there now.  I note that your proposed patch would prevent
   > any possibility of printing debug information about failure cases,
   > since it loses the original result value.
   > 
   > In short: it's not broken now, but this patch would break it.
   
   I thought checking for non-zero was sufficient too, but my Debian
   Squeeze system() manual page says:
   
   	The value returned is -1 on error (e.g. fork(2) failed), and the
   	return status of the command otherwise.  
   
   I am good with the above sentence, but the next sentences have me
   confused:
   
   	This latter return status is in the format specified in wait(2).
   	Thus, the exit code of the command will be WEXITSTATUS(status).
   	In case /bin/sh could not be executed, the exit status will be
   	that of a command that does exit(127).
   
   I assume my pg_upgrade waitpid() code is OK:
   
       ret = waitpid(-1, &work_status, wait_for_child ? 0 : WNOHANG);
   
       /* no children or, for WNOHANG, no dead children */
       if (ret <= 0 || !WIFEXITED(work_status))
           return false;
   
       if (WEXITSTATUS(work_status) != 0)
           pg_log(PG_FATAL, "child worker exited abnormally: %s\n", strerror(errno));
   
   Can that be simplified too?
   
   -- 
     Bruce Momjian  <bruce@momjian.us>        http://momjian.us
     EnterpriseDB                             http://enterprisedb.com
   
     + It's impossible for everything to be true. +