Re: corrupt pages detected by enabling checksums

On 2013-04-05 16:29:47 -0700, Jeff Davis wrote:
> On Fri, 2013-04-05 at 15:09 +0200, Andres Freund wrote:
> > How does the attached version look? I verified that it survives
> > recovery, but not more.
>
> Comments:
>
> * Regarding full page writes, we can:
> - always write full pages (as in your current patch), regardless of
> the current settings
> - take WALInsertLock briefly to get the current settings from XLogCtl
> - always calculate the full page record, but in XLogInsert, if it
> happens to be a HINT record, and full page writes are not necessary,
> then discard it (right now I somewhat favor this option but I haven't
> looked at the details)

I feel pretty strongly that we shouldn't add any such complications to
XLogInsert() itself, its complicated enough already and it should be
made simpler, not more complicated.

I think we can just make up the rule that changing full page writes also
requires SpinLockAcquire(&xlogctl->info_lck);. Then its easy enough. And
it can hardly be a performance bottleneck given how infrequently its
modified.

In retrospect I think making up the rule that full_page_writes changes
imply a checkpoint would have made things easier performance and
codewise.

> * typo in "Backup blocks are not used in **xlog xlog** records"

Thats just me being "funny" after some long days ;). See its an 'xlog'
'xlog record'.

> * To get the appropriate setting for buffer_std, we should pass it down
> through MarkBufferDirty as an extra flag, I think.

Or just declare as part of the api that only std buffers are allowed to
be passed down. After a quick look it looks like all callers use enough
of the standard page layout to make that viable. But that really was
just a quick look.

> * I'm a bit worried that we'll need a cleanup lock when restoring an FSM
> page. What do you think?

I don't yet see why, while recovery is ongoing there shouldn't be anyone
doing anything concurrently to it but startup itself?

> * In xlog_redo, it seemed slightly awkward to call XLogRecGetData twice.
> Merely a matter of preference but I thought I would mention it.

Youre absolutely right, memcpy should have gotten passed 'data', not
XLogRecGetData().

> > Jeff, any chance you can run this for a round with your suite?
>
> Yes. I don't have a rigorous test suite, but I'll do some manual tests
> and walk through it with gdb.

Heh, in this and only this I was talking to Jeff Janes. Strangely I
never had noticed that you share the same name ;)

Thanks!

Andres Freund

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