On Tue, Feb 21, 2012 at 10:07 AM, Noah Misch <n...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
