Re: PITR potentially broken in 9.2

On 2012-12-05 11:11:23 -0500, Tom Lane wrote:
> Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> > On 2012-12-05 13:34:05 +0000, Simon Riggs wrote:
> >> @@ -5883,6 +5889,17 @@ StartupXLOG(void)
> >> } while (record != NULL && recoveryContinue);
> >>
> >> /*
> >> + * We've reached stop point, but not yet applied last
> >> + * record. Pause AFTER final apply, if requested, but
> >> + * only if users can connect to send a resume message
> >> + */
> >> + if (reachedStopPoint && recoveryPauseAtTarget && recoveryApply)
> >> + {
> >> + SetRecoveryPause(true);
> >> + recoveryPausesHere();
> >> + }
> >> +
> >> + /*
>
> > I find the above comment a bit misleading because by now we have in fact
> > applied the last record...
>
> I'd go further than that: a pause after we've exited the loop is
> completely stupid. The only reason for pausing is to let the user
> choose whether to continue applying WAL or not. If you've already
> made that choice, you might as well let the system come up fully.

Yes, that puzzles me as well.

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. I guess thats
what Simon tried to address with the patch. But the new behaviour seems
to be strane as well a

> But I can't make any sense of the rest of this patch, because it seems
> to be randomly rearranging a whole lot of stuff that's unrelated to
> pausing. If you think all these changes are in fact necessary, could
> you break it down a little more?

I think part !recoveryApply changes makes sense, but it needs more...

Greetings,

Andres Freund

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