pg.ddx.io  pgsql-patches@postgresql.org mailing list archive  

help / color / mirror / Atom feed
From: Zdenek Kotala <Zdenek.Kotala@Sun.COM>
To: Xiao Meng <mx.cogito@gmail.com>
Cc: pgsql-patches@postgresql.org, "Jonah H. Harris" <jonah.harris@gmail.com>
Subject: Re: hash index improving v3
Date: Thu, 04 Sep 2008 14:54:37 +0200
Message-ID: <48BFDA8D.3080506@sun.com> (raw)
In-Reply-To: <ded849dd0808171846g2c6c65adub942bd2510a6c94f@mail.gmail.com>
References: <ded849dd0808171846g2c6c65adub942bd2510a6c94f@mail.gmail.com>

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






view thread (63+ messages)  latest in thread

Message-ID: <48BFDA8D.3080506@sun.com>
Permalink:  ../48BFDA8D.3080506@sun.com/
Also on:    postgresql.org/message-id/48BFDA8D.3080506@sun.com

 · 

reply
Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: pgsql-patches@postgresql.org
  Cc: Zdenek.Kotala@Sun.COM, mx.cogito@gmail.com, jonah.harris@gmail.com
  Subject: Re: hash index improving v3
  In-Reply-To: <48BFDA8D.3080506@sun.com>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox