Re: crash-safe visibility map, take five

On Wed, May 11, 2011 at 8:46 AM, Pavan Deolasee
<pavan(dot)deolasee(at)gmail(dot)com> wrote:
> 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.

Yep, I just copied the style of the existing function, in the interest
of changing no more than necessary. But maybe it'd be better to
change them both to take just the Buffer, instead of a pointer.
Anyone else want to weigh in?

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

I don't think there are enough copies of this logic to worry about it;
and I think that keeping those routines separate is more clear.

> The comment at the top of visibilitymap_pin_ok  says "On entry, *buf", but
> the function really takes just a buf.

Good catch. Very slightly updated patch attached.

> You can possibly fold
> visibilitymap_pin_ok() into a macro (and also name it slightly differently
> like visibilitymap_is_pinned ?).

Well, that name would be kind of misleading, because it's not so much
checking whether we have a pin, but rather whether any pin we have is
the right one.

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