Re: 16-bit page checksums for 9.2

On Wed, Feb 8, 2012 at 3:24 AM, Noah Misch <noah(at)leadboat(dot)com> wrote:
> On Tue, Feb 07, 2012 at 08:58:59PM +0000, Simon Riggs wrote:
>> On Thu, Jan 26, 2012 at 8:20 PM, Noah Misch <noah(at)leadboat(dot)com> wrote:
>> > On Wed, Jan 11, 2012 at 10:12:31PM +0000, Simon Riggs wrote:
>
>> > This patch uses FPIs to guard against torn hint writes, even when the
>> > checksums are disabled. ?One could not simply condition them on the
>> > page_checksums setting, though. ?Suppose page_checksums=off and we're hinting
>> > a page previously written with page_checksums=on. ?If that write tears,
>> > leaving the checksum intact, that block will now fail verification. ?A couple
>> > of ideas come to mind. ?(a) Read the on-disk page and emit an FPI only if the
>> > old page had a checksum. ?(b) Add a checksumEnableLSN field to pg_control.
>> > Whenever the cluster starts with checksums disabled, set the field to
>> > InvalidXLogRecPtr. ?Whenever the cluster starts with checksums enabled and the
>> > field is InvalidXLogRecPtr, set the field to the next LSN. ?When a checksum
>> > failure occurs in a page with LSN older than the stored one, emit either a
>> > softer warning or no message at all.
>>
>> We can only change page_checksums at restart (now) so the above seems moot.
>>
>> If we crash with FPWs enabled we repair any torn pages.
>
> There's no live bug, but that comes at a high cost: the patch has us emit
> full-page images for hint bit writes regardless of the page_checksums setting.

Sorry, I don't understand what you mean. I don't see any failure cases
that require that.

page_checksums can only change at a shutdown checkpoint,

The statement "If that write tears,
>> > leaving the checksum intact, that block will now fail verification."
cannot happen, ISTM.

If we write out a block we update the checksum if page_checksums is
set, or we clear it. If we experience a torn page at crash, the FPI
corrects that, so the checksum never does fail verification. We only
need to write a FPI when we write with checksums.

If that's wrong, please explain a failure case in detail.

>> > PageSetLSN() is not atomic, so the shared buffer content lock we'll be holding
>> > is insufficient.
>>
>> Am serialising this by only writing PageLSN while holding buf hdr lock.
>
> That means also taking the buffer header spinlock in every PageGetLSN() caller
> holding only a shared buffer content lock.  Do you think that will pay off,
> versus the settled pattern of trading here your shared buffer content lock for
> an exclusive one?

Yes, I think it will pay off. This is the only code location where we
do that, and we are already taking the buffer header lock, so there is
effectively no overhead.

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