Skip site navigation (1) Skip section navigation (2)

Re: hash index improving v3

From: Zdenek Kotala <Zdenek(dot)Kotala(at)Sun(dot)COM>
To: Xiao Meng <mx(dot)cogito(at)gmail(dot)com>
Cc: pgsql-patches(at)postgresql(dot)org, "Jonah H(dot) Harris" <jonah(dot)harris(at)gmail(dot)com>
Subject: Re: hash index improving v3
Date: 2008-09-04 12:54:37
Message-ID: 48BFDA8D.3080506@sun.com (view raw or flat)
Thread:
Lists: pgsql-hackerspgsql-patches
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




In response to

Responses

pgsql-hackers by date

Next:From: Xiao MengDate: 2008-09-04 13:03:45
Subject: Re: [PATCHES] hash index improving v3
Previous:From: Tobias AnstettDate: 2008-09-04 12:48:06
Subject: xml2 vs XMLFunctions

pgsql-patches by date

Next:From: Xiao MengDate: 2008-09-04 13:03:45
Subject: Re: [PATCHES] hash index improving v3
Previous:From: Tom LaneDate: 2008-09-04 05:35:16
Subject: Re: hash index improving v3

Privacy Policy | About PostgreSQL
Copyright © 1996-2014 The PostgreSQL Global Development Group