Re: PITR potentially broken in 9.2

Andres Freund <andres@2ndquadrant.com> writes:
> Basically the whole logical arround recoveryApply seems to be broken
> currently. Because if recoveryApply=false we currently don't pause at
> all because we jump out of the apply loop with the break.

Huh?  That break is after the pause:

                /*
                 * Have we reached our recovery target?
                 */
                if (recoveryStopsHere(record, &recoveryApply))
                {
                    /*
                     * Pause only if users can connect to send a resume
                     * message
                     */
                    if (recoveryPauseAtTarget && standbyState == STANDBY_SNAPSHOT_READY)
                    {
                        SetRecoveryPause(true);
                        recoveryPausesHere();
                    }
                    reachedStopPoint = true;    /* see below */
                    recoveryContinue = false;
                    if (!recoveryApply)
                        break;
                }

The point of recoveryApply is that the stop can be defined as occurring
either before or after the current WAL record.  However, I do see your
point, which is that if the stop is defined to be after the current WAL
record then we probably should apply same before pausing.  Otherwise
the pause is a bit useless since the user can't see the state he's being
asked to approve.

The real question here probably needs to be "what is the point of
recoveryPauseAtTarget in the first place?".  I find it hard to envision
what's the point of pausing unless the user has an opportunity to
make a decision about whether to continue applying WAL.  As Simon
mentioned, we seem to be lacking some infrastructure that would let
the user adjust the recovery_target parameters before resuming WAL
processing.  But, assuming for the moment that our workaround for
that is "shutdown the server, adjust recovery.conf, and restart",
is the pause placed in a useful spot for that?

BTW, could we make this more convenient by letting recoveryPausesHere()
reread recovery.conf?  Also, shouldn't the code re-evaluate
recoveryStopsHere() after that?

            regards, tom lane