Re: PITR potentially broken in 9.2

On 2012-12-05 11:40:16 -0500, Tom Lane wrote:
> 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;
>                 }

Oh, yea. I mixed what I read in the patch and what exists today in my
mind, sorry for that.

But it still seems rather strange that we break out of the loop
depending on recovery_target_inclusive which is what recoveryApply
basically is set to.

We do:
    ereport(LOG,
            (errmsg("recovery has paused"),
             errhint("Execute pg_xlog_replay_resume() to continue.")));

if we reach the target, so jumping out of the loop seems really counter
intuitive to me.

> 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.

Yes. Seems easy enough if we do the SetRecoveryPause() up there and just
fall through to the pause later in the loop.

> 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.

You can do SELECT pg_xlog_replay_pause|resume(); but that obviously
sucks without further support...

> 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?

I actually think Simon's proposed pause location makes more sense if
thats the only mode we support, but if so it probably should be the one
for inclusive/non-inclusive replay.

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

At least it seems like something we could do without introducing more
functions (i.e. possibly back-branch fit). Given that pauses are
controlled via sql functions setting the target also via an sql
functions seems kinda fitting otherwise.

Greetings,

Andres

--
 Andres Freund                       http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services