Re: Avoiding unnecessary reads in recovery

"Heikki Linnakangas" <heikki(at)enterprisedb(dot)com> writes:

> While working on this, this comment in ReadBuffer caught my eye:
>
>> /*
>> * During WAL recovery, the first access to any data page should
>> * overwrite the whole page from the WAL; so a clobbered page
>> * header is not reason to fail. Hence, when InRecovery we may
>> * always act as though zero_damaged_pages is ON.
>> */
>> if (zero_damaged_pages || InRecovery)
>> {
>
> But that assumption only holds if full_page_writes is enabled, right? I changed
> that in the attached patch as well, but if it isn't accepted that part of it
> should still be applied, I think.

Well it's only true if full_page_writes was on when the WAL was written. Which
isn't necessarily the same as saying it's enabled during recovery...

As long as there's a backup block in the log we can use it to clobber pages in
the heap -- which is what your patch effectively does anyways. If we're
replaying a log entry where there isn't a backup block and we find a damaged
page then we're in trouble. Either the damaged page was in a previous backup
block or it's the recovery itself that's damaging it.

In the latter case it would be pretty useful to abort the recovery so the user
doesn't lose his backup and has a chance to recovery properly (possibly after
reporting and fixing the bug).

So in short I think with your patch this piece of code no longer has a role.
Either your patch kicks in and we never even look at the damaged page at all,
or we should be treating it as corrupt data and just check zero_damaged_pages
alone and not do anything special in recovery.

--
Gregory Stark
EnterpriseDB http://www.enterprisedb.com