On 2016-07-17 23:34:01 -0400, Robert Haas wrote:
> Thanks very much for working on this. Random suggestions after a quick look:
>
> + * Before locking the buffer, pin the visibility map page if it may be
> + * necessary.
>
> s/necessary/needed/
>
> More substantively, what happens if the situation changes before we
> obtain the buffer lock? I think you need to release the page lock,
> pin the page after all, and then relock the page.
It shouldn't be able to. Cleanup locks, which are required for
vacuumlazy to do anything relevant, aren't possible with the buffer
pinned. This pattern is used in heap_delete/heap_update, so I think
we're on a reasonably well trodden path.
> There seem to be several ways to escape from this function without
> releasing the pin on vmbuffer. From the visibilitymap_pin call here,
> search downward for "return".
Hm, that's cleary not good.
The best thing to address that seems to be to create a
separate jump label, which check vmbuffer and releases the page
lock. Unless you have a better idea.
> + * visibilitymap_clear - clear bit(s) for one page in visibility map
>
> I don't really like the parenthesized-s convention as a shorthand for
> "one or more". It tends to confuse non-native English speakers.
>
> + * any I/O. Returns whether any bits have been cleared.
>
> I suggest "Returns true if any bits have been cleared and false otherwise".
Will change.
- Andres