Re: hash index improving v3 - Mailing list pgsql-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