DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=beta;
	h=received:message-id:date:from:user-agent:mime-version:to:cc:subject:references:in-reply-to:content-type:content-transfer-encoding:sender;
	b=MCcNXXRkeKpMqBCYn8c/emoARygzeaPUDP26qwTD7hpYYLkoatDQ4/2urgx0suJA/WT/Uh+uTe4pTv9yzmgm+K6mVFxnVHO01TCndjVhQrmjv/7FDlNYzUVIBArUJ4kXPhb7W8zXNpwXVHHBCIERxIv0gOGizbuBWKKinsQ+E2A=
Message-ID: <46761DB7.7070505@enterprisedb.com>
Date: Mon, 18 Jun 2007 06:52:55 +0100
From: Heikki Linnakangas <heikki@enterprisedb.com>
User-Agent: Icedove 1.5.0.10 (X11/20070329)
MIME-Version: 1.0
To: Tom Lane <tgl@sss.pgh.pa.us>
CC: Jaime Casanova <systemguards@gmail.com>,
	PostgreSQL-patches <pgsql-patches@postgresql.org>
Subject: Re: Maintaining cluster order on insert
References: <200705152223.l4FMNJa17969@momjian.us>
	<464A33AB.1050401@enterprisedb.com>
	<20070516003810.GI20707@nasby.net>
	<464B15EF.4000808@enterprisedb.com>
	<c2d9e70e0705172153g3207adb6kd4abdfdf3a9215ff@mail.gmail.com>
	<464D96D6.1030608@enterprisedb.com>
	<c2d9e70e0705182205o44a4bbbft18a73b9de91de726@mail.gmail.com>
	<464F410C.8090900@enterprisedb.com>
	<10328.1182113758@sss.pgh.pa.us>
In-Reply-To: <10328.1182113758@sss.pgh.pa.us>
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit

Tom Lane wrote:
> Heikki Linnakangas <heikki@enterprisedb.com> writes:
>> The implementation has changed a bit since August. I thought I had
>> submitted an updated version in the winter but couldn't find it. Anyway,
>> I updated and dusted off the source tree, tidied up the comments a
>> little bit, and fixed some inconsistencies in pg_proc entries that made
>> opr_sanity to fail.
> 
> I started looking at this patch.  My first reaction is that I liked last
> August's API (an independent "suggestblock" call) a lot better.  I think
> trying to hold an index page lock across the heap insert is an extremely
> bad idea; it will hurt concurrency and possibly cause deadlocks
> (especially for unique indexes).

The index page is not kept locked across the calls. Just pinned.

The reason for switching to the new API instead of the amsuggestblock 
API is CPU overhead. It avoids constructing the IndexTuple twice and 
descending the tree twice.

Clustering is mainly useful when the table doesn't fit in cache, so one 
could argue that if you care about clustering you're most likely I/O 
bound and don't care about the CPU overhead that much. Nevertheless, 
avoiding it seems like a good idea to me.

The amsuggestblock API is simpler, though. We might choose it on those 
grounds alone.

> The other question is why is execMain involved in this?  That makes the
> design nonfunctional for tuples inserted in any other way than through
> the main executor --- COPY for instance.  Also, if this is successful,
> I could see using it on system catalogs eventually.  I'm inclined to
> think that the right design is for heap_insert to call amsuggestblock
> for itself, or maybe even push that down to RelationGetBufferForTuple.
> (Note: having heap_insert contain logic that duplicates
> RelationGetBufferForTuple's is another bit of bad design here, but
> that's at least correctable locally.)  Now the difficulty in that is
> that the heapam.c routines don't have hold of any data structure
> containing index knowledge ... but they do have hold of the Relation
> structure for the heap.  I suggest making RelationGetIndexList() cache
> the OID of the clustered index (if any) in relcache entries, and add
> RelationGetClusterIndex much like RelationGetOidIndex, and then
> heap_insert can use that.

Hmm. My first reaction on that is that having heap_insert reach into an 
index is a modularity violation. It wouldn't be too hard to make similar 
changes to COPY that I did to the executor.

I doubt it's very useful for system catalogs; they should never grow 
very large, and should stay mostly cached anyway.

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