Received: from localhost (maia-5.hub.org [200.46.204.182]) by postgresql.org (Postfix) with ESMTP id 3D2789FA479 for ; Sun, 17 Jun 2007 17:56:09 -0300 (ADT) Received: from postgresql.org ([200.46.204.71]) by localhost (mx1.hub.org [200.46.204.182]) (amavisd-maia, port 10024) with ESMTP id 23084-07 for ; Sun, 17 Jun 2007 17:55:59 -0300 (ADT) X-Greylist: from auto-whitelisted by SQLgrey-1.7.4 Received: from sss.pgh.pa.us (sss.pgh.pa.us [66.207.139.130]) by postgresql.org (Postfix) with ESMTP id F2BF99FB344 for ; Sun, 17 Jun 2007 17:56:01 -0300 (ADT) Received: from sss2.sss.pgh.pa.us (tgl@localhost [127.0.0.1]) by sss.pgh.pa.us (8.14.1/8.14.1) with ESMTP id l5HKtwxS010329; Sun, 17 Jun 2007 16:55:59 -0400 (EDT) To: Heikki Linnakangas cc: Jaime Casanova , PostgreSQL-patches Subject: Re: Maintaining cluster order on insert In-reply-to: <464F410C.8090900@enterprisedb.com> References: <200705152223.l4FMNJa17969@momjian.us> <464A33AB.1050401@enterprisedb.com> <20070516003810.GI20707@nasby.net> <464B15EF.4000808@enterprisedb.com> <464D96D6.1030608@enterprisedb.com> <464F410C.8090900@enterprisedb.com> Comments: In-reply-to Heikki Linnakangas message dated "Sat, 19 May 2007 19:25:16 +0100" Date: Sun, 17 Jun 2007 16:55:58 -0400 Message-ID: <10328.1182113758@sss.pgh.pa.us> From: Tom Lane X-Virus-Scanned: Maia Mailguard 1.0.1 X-Archive-Number: 200706/175 X-Sequence-Number: 2791 Heikki Linnakangas 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 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. regards, tom lane