On Wed, Mar 29, 2017 at 8:03 AM, Mithun Cy <mithun...@enterprisedb.com> wrote: > Thanks, Amit for a detailed review.
I think that the macros in hash.h need some more work: - Pretty much any time you use the argument of a macro, you need to parenthesize it in the macro definition to avoid surprises if the macros is called using an expression. That isn't done consistently here. - The macros make extensive use of magic numbers like 1, 2, and 3. I suggest something like: #define SPLITPOINT_PHASE_BITS 2 #define SPLITPOINT_PHASES_PER_GROUP (1 << SPLITPOINT_PHASE_BITS) And then use SPLITPOINT_PHASE_BITS any place where you're currently saying 2. The reference to 3 is really SPLITPOINT_PHASE_BITS + 1. - Many of these macros are only used in one place. Maybe just move the computation to that place and get rid of the macro. For example, _hash_spareindex() could be written like this: if (splitpoint_group < SPLITPOINT_GROUPS_WITH_ONLY_ONE_PHASE) return splitpoint_group; /* account for single-phase groups */ splitpoint = SPLITPOINT_GROUPS_WITH_ONLY_ONE_PHASE; /* account for completed groups */ splitpoint += (splitpoint_group - SPLITPOINT_GROUPS_WITH_ONLY_ONE_PHASE) << SPLITPOINT_PHASE_BITS; /* account for phases within current group */ splitpoint += (bucket_num >> (SPLITPOINT_PHASE_BITS + 1)) & SPLITPOINT_PHASE_MASK; return splitpoint; That eliminates the only use of two complicated macros and is in my opinion more clear than what you've currently got. - Some of these macros lack clear comments explaining their purpose. - Some of them don't include HASH anywhere in the name, which is essential for a header that may easily be included by non-hash index code. - The names don't all follow a consistent format. Maybe that's too much to hope for at some level, but I think they could be more consistent than they are. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (email@example.com) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers