On Tue, Feb 21, 2012 at 10:07 AM, Noah Misch <noah@leadboat.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