Re: crash-safe visibility map, take five

On Tue, May 10, 2011 at 7:38 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> On Tue, May 10, 2011 at 9:45 AM, Merlin Moncure <mmoncure(at)gmail(dot)com>
> wrote:
> > 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.
>
> Oh, I see. I did write "lock" when I meant "pin", and your other
> point is well-taken as well. Here's a revised version with some
> additional wordsmithing.
>
>
Some trivial comments.

Why do the set and clear functions need pass-by-reference (Buffer *)
argument ? I don't see them modifying the argument at all. This patch adds
the clear function, but the existing set function also suffers from that.

There are several invocations of pin/clear/release combos. May be you would
want a convenience routine for doing that in a single step or just pass
InvalidBuffer to clear() in which case, it would assume that the vm buffer
is not pinned and do the needful.

The comment at the top of visibilitymap_pin_ok says "On entry, *buf", but
the function really takes just a buf. You can possibly fold
visibilitymap_pin_ok() into a macro (and also name it slightly differently
like visibilitymap_is_pinned ?).

Thanks,
Pavan

--
Pavan Deolasee
EnterpriseDB http://www.enterprisedb.com