Re: Visibility map, partial vacuums

Tom Lane wrote:
> * ISTM that the patch is designed on the plan that the PD_ALL_VISIBLE
> page header flag *must* be correct, but it's really okay if the backing
> map bit *isn't* correct --- in particular we don't trust the map bit
> when performing antiwraparound vacuums. This isn't well documented.

Right. Will add comments.

We can't use the map bit for antiwraparound vacuums, because the bit
doesn't tell you when the tuples have been frozen. And we can't advance
relfrozenxid if we've skipped any pages.

I've been thinking that we could add one frozenxid field to each
visibility map page, for the oldest xid on the heap pages covered by the
visibility map page. That would allow more fine-grained anti-wraparound
vacuums as well.

> * Also, I see that vacuum has a provision for clearing an incorrectly
> set PD_ALL_VISIBLE flag, but shouldn't it fix the map too?

Yes, will fix. Although, as long as we don't trust the visibility map,
no real damage would be done.

> * It would be good if the visibility map fork were never created until
> there is occasion to set a bit in it; this would for instance typically
> mean that temp tables would never have one. I think that
> visibilitymap.c doesn't get this quite right --- in particular
> vm_readbuf seems willing to create/extend the fork whether its extend
> argument is true or not, so it looks like an inquiry operation would
> cause the map fork to be created. It should be possible to act as
> though a nonexistent fork just means "all zeroes".

The visibility map won't be inquired unless you vacuum. This is a bit
tricky. In vacuum, we only know whether we can set a bit or not, after
we've acquired a cleanup lock on the page, and scanned all the tuples.
While we're holding a cleanup lock, we don't want to do I/O, which could
potentially block out other processes for a long time. So it's too late
to extend the visibility map at that point.

I agree that vm_readbuf should not create the fork if 'extend' is false,
that's an oversight, but it won't change the actual behavior because
visibilitymap_test calls it with 'extend' true. Because of the above.

I will add comments about that, though, there's nothing describing that
currently.

> * heap_insert's all_visible_cleared variable doesn't seem to get
> initialized --- didn't your compiler complain?

Hmph, I must've been compiling with -O0.

> * You missed updating SizeOfHeapDelete and SizeOfHeapUpdate

Thanks.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com