Re: Verified fix for Bug 4137

On Tue, 2008-05-06 at 17:52 +0100, Heikki Linnakangas wrote:
> Simon Riggs wrote:
> > 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.
>
> I didn't suggest that alphabetical sorting property is a new assumption;
> it sure isn't. The new assumption is that you never call ReadRecord()
> for record 0002 before you call it for record 0001 (before initializing
> the checkPointCopy field from the checkpoint record, anyway).

I've not done anything with the ordering of calls to ReadRecord(). There
is no such assumption introduced here.

The patch prevents an erroneous call to delete files. The earlier code
just assumed such a call would never occur, which was wrong, hence the
patch. There are no new assumptions introduced into the code by this.

I very much respect your opinion in all things, most especially on
matters of code, more so now you are a committer. I would be willing to
make changes to any patch on your say-so, though I can't see how any of
your comments improve this patch in this specific case only. I don't
doubt that you will find flaws in many of my future patches.

> I can imagine a future patch to do xlog file prefetching, for example,
> that breaks that assumption.

Maybe, but I think its for such a patch to consider in full the
consequences of doing that, not for me to do so in advance. The current
assumption is filename ordered recovery, there is definitely more than
one part of the code that relies significantly on that point.

I think any prefetcher would be advised to stay within one file at a
time. Anything else will effect other behaviours in ways that would need
careful planning to avoid unintended consequences.

> Or falling back to the previous checkpoint
> as discussed. Or maybe you screwed up your recovery.conf, and try to use
> WAL files that belong to a different installation. The database won't
> start up, of course, because the checkpoint record isn't in the right
> place, but the damage has already been done because the recovery command
> deleted some files it shouldn't have.
>
> Granted, none of those are particularly likely, but we should be extra
> careful when deleting files.

With respect, that is exactly what the patch does, though it is
certainly better for being tested by your comments.

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