Message-ID: <4E42DFCE.3050904@enterprisedb.com>
Date: Wed, 10 Aug 2011 22:45:18 +0300
From: Heikki Linnakangas <heikki.linnakangas@enterprisedb.com>
User-Agent: Mozilla/5.0 (X11; U; Linux x86_64; fi-FI;
	rv:1.9.2.18) Gecko/20110626 Icedove/3.1.11
MIME-Version: 1.0
To: Alexander Korotkov <aekorotkov@gmail.com>
CC: pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: WIP: Fast GiST index build
References: <BANLkTikgOQ1+r=A9b08szNTeEWsk-cNMMg@mail.gmail.com>
	<CAPpHfdtVJJyinb1TuFF3gw6-YLre2O9svUZgZGAWzogXswP6Bg@mail.gmail.com>
	<CAPpHfdvoOh78ycX3eRePTS29635pHCSTfLdFDzHZhxTKsggCuQ@mail.gmail.com>
	<4E2DBB60.80604@enterprisedb.com>
	<4E2EF83A.8000304@enterprisedb.com>
	<CAPpHfdso-oOo4+3r9yBn=PATz3BC4z4cxK9Ew8tFRT1MkVVoiA@mail.gmail.com>
	<4E31D03D.70206@enterprisedb.com>
	<CAPpHfdtmgwOjNr7J15eCe6sXrtmq_c_52oNh-=D934SKekoGxg@mail.gmail.com>
	<CAPpHfdv=HQDmUGTARKjH+AiHMDpXa+8+WwuqjUSfZnVdC-LJbQ@mail.gmail.com>
	<CAPpHfduAhSNo8LHMPj_pqQ_2W6ZDPzdQBPzHbnpqaKz6kf4nrw@mail.gmail.com>
	<4E3977D8.1020506@enterprisedb.com>
	<CAPpHfducs8Ussw+t8iP3WS3ZTuX2J-77FOY=jAZUg-K6AuOC0g@mail.gmail.com>
	<CAPpHfdusd_b_dtvNY2uuJict6=VEUTGAS8ELXGck=o=kOdMnOw@mail.gmail.com>
	<4E3FAB1D.1060105@enterprisedb.com>
	<CAPpHfdsovwbgs30TojJc_zDsc+0ghapm+-AALfP58UaHiYjr2A@mail.gmail.com>
	<CAPpHfdvi5WZKdRD+s60R+pdm8dLczRQ5_RYt4L8OA_bhorPjpg@mail.gmail.com>
In-Reply-To: 
 <CAPpHfdvi5WZKdRD+s60R+pdm8dLczRQ5_RYt4L8OA_bhorPjpg@mail.gmail.com>
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit

On 10.08.2011 13:19, Alexander Korotkov wrote:
> Hi!
>
> Here is last verion of the patch.
> List of changes:
> 1) Neighbor relocation and prefetch were removed. They will be supplied as
> separate patches.

unloadNodeBuffers() is now dead code.

> 2) Final emptying now using standart lists of all buffers by levels.
> 3) Automatic switching again use simple comparison of index size and
> effective_cache_size.

LEAF_PAGES_STATS_* are unused now. Should avoid calling smgrnblocks() on 
every tuple, the overhead of that could add up.

> 4) Some renames. In particular GISTLoadedPartItem
> to GISTBufferingInsertStack.
> 5) Some comments were corrected and some were added.
> 6) pgindent
> 7) rebased with head
>
> Readme update and user documentation coming soon.

I wonder, how hard would it be to merge gistBufferingBuildPlaceToPage() 
with the gistplacetopage() function used in the main codepath? There's 
very little difference between them, and it would be nice to maintain 
just one function. At the very least I think there should be a comment 
in both along the lines of "NOTE: if you change this function, make sure 
you update XXXX (the other function) as well!"

In gistbuild(), in the final emptying stage, there's this special-case 
handling for the root block before looping through the buffers in the 
buffersOnLevels lists:

> 		for (;;)
> 		{
> 			nodeBuffer = getNodeBuffer(gfbb, &buildstate.giststate, GIST_ROOT_BLKNO,
> 									   InvalidOffsetNumber, NULL, false);
> 			if (!nodeBuffer || nodeBuffer->blocksCount <= 0)
> 				break;
> 			MemoryContextSwitchTo(gfbb->context);
> 			gfbb->bufferEmptyingStack = lcons(nodeBuffer, gfbb->bufferEmptyingStack);
> 			MemoryContextSwitchTo(buildstate.tmpCtx);
> 			processEmptyingStack(&buildstate.giststate, &insertstate);
> 		}

What's the purpose of that? Wouldn't the loop through buffersOnLevels 
lists take care of the root node too?

The calculations in initBuffering() desperately need comments. As does 
the rest of the code too, but the heuristics in that function are 
particularly hard to understand without some explanation.

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