Re: Verified fix for Bug 4137

On Tue, 2008-05-06 at 15:00 +0100, Heikki Linnakangas wrote:
> Simon Riggs wrote:
> > Falling back to the secondary checkpoint implies we have a corrupted or
> > absent WAL file, so making recovery startup work correctly won't avoid
> > the need to re-run the base backup. We'll end with an unrecoverable
> > error in either case, so it doesn't seem worth attempting to improve
> > this in the way you suggest.
>
> That's true whenever you have to fall back to a secondary checkpoint,
> but we still try to get the database up. One could argue that we
> shouldn't, of course.

I wouldn't. If we're doing a crash recovery we need that.

> Anyway, the point is that the patch relies on a non-obvious assumption.
> Even if the secondary checkpoint issue is a non-issue, it's not obvious
> (to me at least) that there isn't other similar scenarios. And someone
> might inadvertently break the assumption in a future patch, because it's
> not an obvious one; calling ReadRecord looks very innocent. We shouldn't
> introduce an assumption like that when we don't have to.

We were already assuming archive files were "OK to delete, if before".
The whole of recovery already relies heavily on the alphabetic sorting
property of WAL and associated filenames. Those filenames have been
specifically documented as maintaining that sorted order for that
reason. If somebody wanted to recover files in non-sorted order, then
yes I would expect a few things to break - this aspect wouldn't be the
most critical thing though.

The patch adds only what we already knew: you should never expect to
delete a file *after* one that is being requested. So I see no reason to
remove or change anything from the patch as it stands.

I'd be happy to add comments to say

* We use the alphanumeric sorting property of the filenames to decide
* which ones are earlier

as we have done elsewhere in xlog.c

I do want everything to work in a clear way, but I honestly can't see
any reason to add more code, especially in the absence of any problem.

> > I think we should completely prevent access to secondary checkpoints
> > during archive recovery, because if the primary checkpoint isn't present
> > or is corrupt we aren't ever going to get passed it to get to the
> > pg_stop_backup() point. Hence an archive recovery can never be valid in
> > that case. I'll do a separate patch for that because they are unrelated
> > issues.
>
> Well, we already don't use the secondary checkpoint if a backup label
> file is present.

Yep, looking at the wrong section of code. I wondered why I hadn't
plugged that gap previously.

--
Simon Riggs
2ndQuadrant http://www.2ndQuadrant.com