Received: from localhost (unknown [200.46.204.183])
	by postgresql.org (Postfix) with ESMTP id 526A164FCD8
	for <pgsql-patches-postgresql.org@postgresql.org>;
	Thu,  4 Sep 2008 09:54:10 -0300 (ADT)
Received: from postgresql.org ([200.46.204.86])
	by localhost (mx1.hub.org [200.46.204.183]) (amavisd-maia, port 10024)
	with ESMTP id 54617-02 for
	<pgsql-patches-postgresql.org@postgresql.org>;
	Thu,  4 Sep 2008 09:54:07 -0300 (ADT)
X-Greylist: from auto-whitelisted by SQLgrey-1.7.6
Received: from gmp-eb-inf-2.sun.com (gmp-eb-inf-2.sun.com [192.18.6.24])
	by postgresql.org (Postfix) with ESMTP id 3F20A64FCEB
	for <pgsql-patches@postgresql.org>;
	Thu,  4 Sep 2008 09:54:06 -0300 (ADT)
Received: from fe-emea-09.sun.com (gmp-eb-lb-2-fe3.eu.sun.com [192.18.6.12])
	by gmp-eb-inf-2.sun.com (8.13.7+Sun/8.12.9) with ESMTP id
	m84Crs2O022488
	for <pgsql-patches@postgresql.org>; Thu, 4 Sep 2008 12:54:04 GMT
Received: from conversion-daemon.fe-emea-09.sun.com by fe-emea-09.sun.com
	(Sun Java System Messaging Server 6.2-8.04 (built Feb 28 2007))
	id <0K6O005018148C00@fe-emea-09.sun.com>
	(original mail from Zdenek.Kotala@Sun.COM) for
	pgsql-patches@postgresql.org; Thu, 04 Sep 2008 13:53:54 +0100 (BST)
Received: from [129.157.18.76] by fe-emea-09.sun.com
	(Sun Java System Messaging Server 6.2-8.04 (built Feb 28 2007))
	with ESMTPSA id <0K6O009T795QS5F0@fe-emea-09.sun.com>; Thu,
	04 Sep 2008 13:53:50 +0100 (BST)
Date: Thu, 04 Sep 2008 14:54:37 +0200
From: Zdenek Kotala <Zdenek.Kotala@Sun.COM>
Subject: Re: hash index improving v3
In-reply-to: <ded849dd0808171846g2c6c65adub942bd2510a6c94f@mail.gmail.com>
To: Xiao Meng <mx.cogito@gmail.com>
Cc: pgsql-patches@postgresql.org, "Jonah H. Harris" <jonah.harris@gmail.com>
Message-id: <48BFDA8D.3080506@sun.com>
MIME-version: 1.0
Content-type: text/plain; format=flowed; charset=ISO-8859-2
Content-transfer-encoding: 7BIT
References: <ded849dd0808171846g2c6c65adub942bd2510a6c94f@mail.gmail.com>
User-Agent: Thunderbird 2.0.0.14 (X11/20080630)
X-Virus-Scanned: Maia Mailguard 1.0.1
X-Archive-Number: 200809/38
X-Sequence-Number: 7053

I performed code review and see my comments.


pgsql/src/backend/access/hash/hashpage.c
<http://reviewdemo.postgresql.org/r/26/#comment31>

     use sizeof() or something better the 4.



pgsql/src/backend/access/hash/hashpage.c
<http://reviewdemo.postgresql.org/r/26/#comment32>

     New empty line.



pgsql/src/backend/access/hash/hashutil.c
<http://reviewdemo.postgresql.org/r/26/#comment27>

     It would be better remove #define from hash.h and setup it there 
directly.



pgsql/src/backend/access/hash/hashutil.c
<http://reviewdemo.postgresql.org/r/26/#comment28>

     Why not return directly uint32?



pgsql/src/backend/access/hash/hashutil.c
<http://reviewdemo.postgresql.org/r/26/#comment22>

     Retype to correct return type.



pgsql/src/backend/access/hash/hashutil.c
<http://reviewdemo.postgresql.org/r/26/#comment29>

     Whats about null values?



pgsql/src/backend/access/hash/hashutil.c
<http://reviewdemo.postgresql.org/r/26/#comment30>

     I'm not sure if values modification is safe. Please, recheck.



pgsql/src/backend/access/hash/hashutil.c
<http://reviewdemo.postgresql.org/r/26/#comment23>

     Return value is not much clear. I prefer to return InvalidOffset 
when no record is found. However it seems that you use result also for 
PageAddItem to put item on correct ordered position. I think better 
explanation should help to understand how it works.



pgsql/src/backend/access/hash/hashutil.c
<http://reviewdemo.postgresql.org/r/26/#comment26>

     It could return FirstOffset number in case when nothing interesting 
is on the page.



pgsql/src/include/access/hash.h
<http://reviewdemo.postgresql.org/r/26/#comment34>

     Why not define new datatype for example HashKey instead of uint32?



pgsql/src/include/access/hash.h
<http://reviewdemo.postgresql.org/r/26/#comment33>

     It is not good place. See my other comment.


--------------
You also forgot to bump hash index version in meta page.

		Zdenek