Re: 16-bit page checksums for 9.2

On Tue, Feb 21, 2012 at 10:07 AM, Noah Misch <noah(at)leadboat(dot)com> wrote:

> Consequently, I find the idea of requiring
> a spinlock acquisition to read or write pd_lsn/pd_tli under BUFFER_LOCK_SHARE
> to be a sensible one.

OK

> Within that umbrella, some details need attention:
>
> - Not all BUFFER_LOCK_SHARE callers of PageGetLSN() live in bufmgr.c.  I note
>  gistScanPage() and XLogCheckBuffer()[1].  (Perhaps we'll only require the
>  spinlock for heap buffers, letting gistScanPage() off the hook.)  We need a
>  public API, perhaps LockBufferForLSN().

Yep, I checked all the call points previously. gistScanPage() and also
gistbulkdelete() would be need changing, but since GIST doesn't use
hints, there is no need for locking. I'll document that better.

> - The use of some spinlock need not imply using the buffer header spinlock.
>  We could add a dedicated pd_lsn_tli_lock to BufferDesc.  That has the usual
>  trade-off of splitting a lock: less contention at the cost of more
>  acquisitions.  I have no intuition on which approach would perform better.

Will think about that and try a few ideas.

> I agree that src/backend/storage/buffer/README is the place to document the
> new locking rules.

OK, I'll move the comments.

> I do share your general unease about adding new twists to the buffer access
> rules.  Some of our hairiest code is also the code manipulating buffer locks
> most extensively, and I would not wish to see that code get even more
> difficult to understand.  However, I'm not seeing a credible alternative that
> retains the same high-level behaviors.

Yes, those changes not made lightly and with much checking.

> A possible compromise is to leave the page clean after setting a hint bit,
> much like the patch already has us do under hot standby.  Then there's no new
> WAL and no new rules around pd_lsn.  Wasn't that one of the things Merlin
> benchmarked when he was looking at hint bits?  Does anyone recall the result?

I was thinking exactly that myself. I'll add a GUC to test.

> [1] Patch v10 adds a comment to XLogCheckBuffer() saying otherwise.  The
> comment is true today, but the same patch makes it false by having
> XLogSaveBufferForHint() call XLogInsert() under a share lock.

OK, thats an issue, well spotted. Will fix.

Thanks for thorough review.

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