On Tue, May 10, 2011 at 7:48 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> I thought I'd explained it fairly thoroughly in the comments, but
> evidently not. Suggestions for improvement are welcome.
ok. that clears it up nicely.
> Here goes in more detail: Every time we insert, update, or delete a
> tuple in a particular heap page, we must check whether the page is
> marked all-visible. If it is, then we need to clear the page-level
> bit marking it as all-visible, and also the corresponding page in the
> visibility map. On the other hand, if the page isn't marked
> all-visible, then we needn't touch the visibility map at all. So,
> there are either one or two buffers involved: the buffer containing
> the heap page ("buffer") and possibly also a buffer containing the
> visibility map page in which the bit for the heap page is to be found
> ("vmbuffer"). Before taking an exclusive content-lock on the heap
> buffer, we check whether the page appears to be all-visible. If it
> does, then we pin the visibility map page and then lock the buffer.
> If not, we just lock the buffer.
I see: here's a comment that was throwing me off:
+ /*
+ * If we didn't get the lock and it turns out we need it, we'll have to
+ * unlock and re-lock, to avoid holding the buffer lock across an I/O.
+ * That's a bit unfortunate, but hopefully shouldn't happen often.
+ */
I think that might be phrased as "didn't get the pin and it turns out
we need it because the bit can change after inspection". The visible
bit isn't 'wrong' as suggested in the comments, it just can change so
that it becomes wrong. Maybe a note of why it could change would be
helpful.
Other than that, it looks pretty good...ISTM an awfully small amount
of code to provide what it's doing (that's a good thing!).
merlin
