Message-ID: <492A6032.6080000@enterprisedb.com>
Date: Mon, 24 Nov 2008 10:05:06 +0200
Organization: EnterpriseDB
User-Agent: Mozilla-Thunderbird 2.0.0.17 (X11/20081018)
MIME-Version: 1.0
To: Tom Lane <tgl@sss.pgh.pa.us>
CC: PostgreSQL-development <pgsql-hackers@postgresql.org>
Subject: Re: Visibility map, partial vacuums
References: <4905AE17.7090305@enterprisedb.com>
	<491D376B.9000608@enterprisedb.com>
	<491D7F52.6070908@enterprisedb.com>
	<4925664C.3090605@enterprisedb.com>
	<26361.1227467112@sss.pgh.pa.us>
In-Reply-To: <26361.1227467112@sss.pgh.pa.us>
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit
From: Heikki Linnakangas <heikki.linnakangas@enterprisedb.com>

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