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
--
Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-patches
