On Mon, Feb 20, 2012 at 08:57:08AM -0500, Robert Haas wrote: > On Mon, Feb 20, 2012 at 4:18 AM, Simon Riggs <si...@2ndquadrant.com> wrote: > > What straightforward implementation is that?? This *is* the straightforward > > one. > > > > God knows what else we'd break if we drop the lock, reacquire as an > > exclusive, then drop it again and reacquire in shared mode. Code tends > > to assume that when you take a lock you hold it until you release; > > doing otherwise would allow all manner of race conditions to emerge. > > > > And do you really think that would be faster? > > I don't know, but neither do you, because you apparently haven't tried > it. Games where we drop the shared lock and get an exclusive lock > are used in numerous places in the source code; see, for example, > heap_update(), so I don't believe that there is any reason to reject > that a priori. Indeed, I can think of at least one pretty good reason > to do it exactly that way: it's the way we've handled all previous > problems of this type, and in general it's better to make new code > look like existing code than to invent something new, especially when > you haven't made any effort to quantify the problem or the extent to > which the proposed solution mitigates it.
We do, in numerous places, drop a shared buffer content lock and reacquire it in exclusive mode. However, the existing users of that pattern tend to trade the lock, complete subsequent work, and unlock the buffer all within the same function. So they must, because several of them recheck facts that can change during the period of holding no lock. SetBufferCommitInfoNeedsSave() callers do not adhere to that pattern; they can be quite distant from the original lock acquisition and eventual lock release. Take the prototypical case of SetHintBits(). Our shared lock originated in a place like heapgettup(), which expects to hold it continually until finished. We would need to somehow push up through HeapTupleSatisfiesVisibility() the fact that we traded the buffer lock, then have heapgettup() reorient itself as needed. Every caller of code that can reach SetBufferCommitInfoNeedsSave() would need to do the same. Perhaps there's a different way to apply the lock-trade technique that avoids this mess, but I'm not seeing it. 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. 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(). - 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. I agree that src/backend/storage/buffer/README is the place to document the new locking rules. 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. 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? Thanks, nm [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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
