Re: PITR potentially broken in 9.2

On 2012-12-05 11:40:16 -0500, Tom Lane wrote:
> Andres Freund <andres(at)2ndquadrant(dot)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