Re: Recovery bug

On 19.10.2010 22:40, Jeff Davis wrote:
> On Tue, 2010-10-19 at 09:51 -0700, Jeff Davis wrote:
>> On Tue, 2010-10-19 at 12:26 +0300, Heikki Linnakangas wrote:
>>> Excluding pg_xlog is just a recommendation at the moment, though, so we
>>> would need a big warning in the docs. And some way to enforce that
>>> just_kidding is not included in the backup would be nice, maybe we could
>>> remove read-permission from it?
>>
>> Hmm, removing the read bit would add some confidence into the process. I
>> like that idea better than just assuming that the user won't copy it.
>>
>> I think I like this direction most, because it doesn't leave us
>> guessing. If the file is there then we assume normal recovery. If we
>> encounter recovery.conf we throw FATAL. If we encounter backup_label we
>> can simply remove it (perhaps with a warning that there was a crash in
>> the middle of a backup).
>
> On second thought, this doesn't sound backpatch-friendly. We should
> probably put a simpler fix in first and back-patch it. Then we can do
> something like your proposal for 9.1. What do you think of my proposed
> patch?

Yes, let's go ahead with your original patch.

It seems we should use ReadRecord instead of the lower-level
XLogPageRead function. One difference is that ReadRecord performs a
bunch of sanity checks on the record, while XLogPageRead just reads the
raw page. Extra sanity checking before removing backup_label seems like
a good idea. Another difference is that in standby-mode, ReadRecord will
retry until it succeeds. A standby server should keep retrying, even the
very first record, until it succeeds, otherwise we have a change in
behavior.

So I'm thinking of the attached patch. I'll give this some testing on
older branches and commit.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com