Re: Reviewing freeze map code

On Sun, Jul 17, 2016 at 10:48 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> Took till today. Attached is a rather heavily revised version of
> Sawada-san's patch. Most notably the recovery routines take care to
> reset the vm in all cases, we don't perform visibilitymap_get_status
> from inside a critical section anymore, and
> heap_lock_updated_tuple_rec() also resets the vm (although I'm not
> entirely sure that can practically be hit).
>
> I'm doing some more testing, and Robert said he could take a quick look
> at the patch. If somebody else... Will push sometime after dinner.

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.

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".

+ * 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".

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company