Re: corrupt pages detected by enabling checksums

On 1 May 2013 19:16, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Wed, May 1, 2013 at 1:02 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>> I agree, but that was in the original coding wasn't it?
>
> I believe the problem was introduced by this commit:
>
> commit fdf9e21196a6f58c6021c967dc5776a16190f295
> Author: Heikki Linnakangas <heikki(dot)linnakangas(at)iki(dot)fi>
> Date: Wed Feb 13 17:46:23 2013 +0200
>
> Update visibility map in the second phase of vacuum.
>
> There's a high chance that a page becomes all-visible when the second phase
> of vacuum removes all the dead tuples on it, so it makes sense to check for
> that. Otherwise the visibility map won't get updated until the next vacuum.
>
> Pavan Deolasee, reviewed by Jeff Janes.
>
>> Why aren't we writing just one WAL record for this action? We use a
>> single WAL record in other places where we make changes to multiple
>> blocks with multiple full page writes, e.g. index block split. That
>> would make the action atomic and we'd just have this...
>>
>> 1. Perform the cleanup operations on the buffer.
>> 2. Set the visibility map bit.
>> 3. Log the cleanup operations and visibility map change.
>>
>> which can then be replayed with correct sequence, locking etc.
>> and AFAICS would likely be faster also.
>
> I thought about that, too. It certainly seems like more than we want
> to try to do for 9.3 at this point. The other complication is that
> there's a lot of conditional logic here. We're definitely going to
> emit a cleanup record. We're going to emit a record to make the page
> all-visible only sometimes, because it might not be all-visible yet:
> it could have tuples on it that are deleted but not yet dead. And
> then there's additional logic to handle the checksum case. Plus, the
> all-visible marking can happen in other code paths, too, specifically
> in phase 1 of vacuum. So it might be possible to consolidate this,
> but off-hand it looks messy to me out of proportion to the benefits.

Looks easy. There is no additional logic for checksums, so there's no
third complexity.

So we either have
* cleanup info with vismap setting info
* cleanup info only

which is the same number of WAL records as we have now, just that we
never emit 2 records when one will do.

> Now that I'm looking at this, I'm a bit confused by the new logic in
> visibilitymap_set(). When checksums are enabled, we set the page LSN,
> which is described like this: "we need to protect the heap page from
> being torn". But how does setting the page LSN do that?

It doesn't

> Don't we
> need to mark the buffer dirty or something like that?

We do.

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