Re: crash-safe visibility map, take five

On Sat, Jun 18, 2011 at 10:04:52PM -0400, Robert Haas wrote:
> On Sat, Jun 18, 2011 at 9:37 AM, Noah Misch <noah(at)leadboat(dot)com> wrote:
> > Indeed. ?If we conflicted on the xmin of the most-recent all-visible tuple
> > when setting the bit, all-visible on the standby would become a superset of
> > all-visible on the master, and we could cease to ignore the bits. ?This is an
> > excellent trade-off for masters that do UPDATE and DELETE, because they're
> > already conflicting (or using avoidance measures) on similar xids at VACUUM
> > time. ?(Some INSERT-only masters will disfavor the trade-off, but we could
> > placate them with a GUC. ?On the other hand, hot_standby_feedback works and
> > costs an INSERT-only master so little.)
>
> I hadn't really considered the possibility that making more things
> conflict on the standby server could work out to a win. I think that
> would need some careful testing, and I don't want to get tied up in
> that for purposes of this patch. There is no law against a WAL format
> change, so we can always do it later if someone wants to do the
> legwork.

Fair enough.

> > I ran one repetition of my benchmark from last report, and the amount of WAL
> > has not changed. ?Given that, I think the previous runs remain valid.
>
> As far as I can see, the upshot of those benchmarks was that more WAL
> was generated, but the time to execute vacuum didn't really change. I
> think that's going to be pretty typical. In your test, the additional
> WAL amounted to about 0.6% of the amount of data VACUUM is dirtying,
> which I think is pretty much in the noise, assuming wal_buffers and
> checkpoint_segments are tuned to suitable values.

I think that's about right. The timings were too unstable to conclude much,
so I've focused on the WAL volume, which isn't worrisome.

> The other thing to worry about from a performance view is whether the
> push-ups required to relocate the clear-the-vm-bits logic to within
> the critical sections is going to have a significant impact on
> ordinary insert/update/delete operations. I was a bit unsure at first
> about Heikki's contention that it wouldn't matter, but after going
> through the exercise of writing the code I think I see now why he
> wasn't concerned.

I agree; that part of the cost looks unmeasurable.

> The only possibly-noticeable overhead comes from
> the possibility that we might decide not to pin the visibility map
> buffer before locking the page(s) we're operating on, and need to
> unlock-pin-and-relock. But for that to happen, VACUUM's got to buzz
> through and mark the page all-visible just around the time that we
> examine bit. And I have a tough time imagining that happening very
> often. You have to have a page that has been modified since the last
> VACUUM, but not too recently (otherwise it's not actually all-visible)
> and then VACUUM has to get there and process it at almost the same
> moment that someone decides to make another modification. It's hard
> even to think about an artificial test case that would hit that
> situation with any regularity.

I can't see it happening very often, either.

This version of the patch has some bogus hunks in int.c, int8.c, and heap.c
reverting a few of your other recent commits. Looks good otherwise. I've
marked the patch "Ready for Committer".

Thanks,
nm