Message-ID: <4907187B.7060103@enterprisedb.com>
Date: Tue, 28 Oct 2008 15:49:47 +0200
Organization: EnterpriseDB
User-Agent: Mozilla-Thunderbird 2.0.0.16 (X11/20080724)
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>
	<26727.1225150278@sss.pgh.pa.us>
In-Reply-To: <26727.1225150278@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:
> Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:
>> To modify a page:
>> If PD_ALL_VISIBLE flag is set, the bit in the visibility map is cleared 
>> first. The heap page is kept pinned, but not locked, while the 
>> visibility map is updated. We want to avoid holding a lock across I/O, 
>> even though the visibility map is likely to stay in cache. After the 
>> visibility map has been updated, the page is exclusively locked and 
>> modified as usual, and PD_ALL_VISIBLE flag is cleared before releasing 
>> the lock.
> 
> So after having determined that you will modify a page, you release the
> ex lock on the buffer and then try to regain it later?  Seems like a
> really bad idea from here.  What if it's no longer possible to do the
> modification you intended?

In case of insert/update, you have to find a new target page. I put the 
logic in RelationGetBufferForTuple(). In case of delete and update (old 
page), the flag is checked and bit cleared just after pinning the 
buffer, before doing anything else. (I note that that's not actually 
what the patch is doing for heap_update, will fix..)

If we give up on the strict requirement that the bit in the visibility 
map has to be cleared if the PD_ALL_VISIBLE flag on the page is not set, 
then we could just update the visibility map after releasing the locks 
on the heap pages. I think I'll do that for now, for simplicity.

>> To set the PD_ALL_VISIBLE flag, you must hold an exclusive lock on the 
>> page, while you observe that all tuples on the page are visible to everyone.
> 
> That doesn't sound too good from a concurrency standpoint...

Well, no, but it's only done in VACUUM. And pruning. I implemented it as 
a new loop that call HeapTupleSatisfiesVacuum on each tuple, and 
checking that xmin is old enough for live tuples, but come to think of 
it, we're already calling HeapTupleSatisfiesVacuum for every tuple on 
the page during VACUUM, so it should be possible to piggyback on that by 
restructuring the code.

>> That's how the patch works right now. However, there's a small 
>> performance problem with the current approach: setting the 
>> PD_ALL_VISIBLE flag must be WAL-logged. Otherwise, this could happen:
> 
> I'm more concerned about *clearing* the bit being WAL-logged.  That's
> necessary for correctness.

Yes, clearing the PD_ALL_VISIBLE flag always needs to be WAL-logged. 
There's a new boolean field in xl_heap_insert/update/delete records 
indicating if the operation cleared the flag. On replay, if the flag was 
cleared, the bit in the visibility map is also cleared.


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