On Wed, Mar 29, 2017 at 8:03 AM, Mithun Cy <mithun.cy@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
