Re: [COMMITTERS] pgsql: Upgrade to Autoconf 2.69

On Tue, Jan  7, 2014 at 08:19:49AM -0500, Peter Eisentraut wrote:
> That was probably me. I'll look into it.
>
>
>
> > On Jan 6, 2014, at 11:40 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >
> > Bruce Momjian <bruce@momjian.us> writes:
> >>> On Sun, Dec 29, 2013 at 02:48:21AM -0500, Tom Lane wrote:
> >>> 3. pg_upgrade ignores the fact that pg_resetxlog failed, and keeps going.
> >
> >> Does pg_resetxlog return a non-zero exit status?  If so, pg_upgrade
> >> should have caught that and exited.
> >
> > It certainly does:
> >
> >    if (errno)
> >    {
> >        fprintf(stderr, _("%s: could not read from directory \"%s\": %s\n"),
> >                progname, XLOGDIR, strerror(errno));
> >        exit(1);
> >    }
> >
> > The bug is that pg_upgrade appears to assume (in many places not just this
> > one) that exec_prog() will abort if the called program fails, but *it
> > doesn't*, contrary to the claim in its own header comment.  This is
> > because pg_log(FATAL, ...) doesn't call exit().  pg_fatal() does, but
> > that's not what's being called in the throw_error case.
> >
> > I imagine that this used to work correctly and got broken by some
> > ill-advised refactoring, but whatever the origin, it's 100% broken today.

I know Peter is looking at this, but I looked at and I can't see the
problem.  Every call of exec_prog() that uses pg_resetxlog has
throw_error = true, and the test there is:

    result = system(cmd);

    if (result != 0)
    ...
    pg_log(FATAL, ...)

and in pg_log_v() I see:

        switch (type)
        ...
        case PG_FATAL:
            printf("\n%s", _(message));
            printf("Failure, exiting\n");
-->         exit(1);
            break;

so I am not clear how you are seeing the return status of pg_resetxlog
ignored.  I tried the attached patch which causes pg_resetxlog -f to
return -1, and got the proper error from pg_upgrade in git head:

    Performing Upgrade
    ------------------
    Analyzing all rows in the new cluster                       ok
    Freezing all rows on the new cluster                        ok
    Deleting files from new pg_clog                             ok
    Copying old pg_clog to new server                           ok
    Setting next transaction ID for new cluster
    *failure*

    Consult the last few lines of "pg_upgrade_utility.log" for
    the probable cause of the failure.
    Failure, exiting

and the last line in "pg_upgrade_utility.log" is:

    command: "/u/pgsql/bin/pg_resetxlog" -f -x 683 "/u/pgsql/data" >>
    "pg_upgrade_utility.log" 2>&1

--
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

  + Everyone has their own god. +