Thanks, Amit for a detailed review.
On Wed, Mar 29, 2017 at 4:09 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> Okay, your current patch looks good to me apart from minor comments,
> so marked as Read For Committer. Please either merge the
> sort_hash_b_2.patch with main patch or submit it along with next
> revision for easier reference.
I will keep it separated just in case commiter likes
sortbuild_hash_A.patch. We can use either of sortbuild_hash_*.patch on
top of main patch.
>
> Few minor comments:
> 1.
> +splitpoint phase S. The hashm_spares[0] is always 0, so that buckets 0 and 1
> +(which belong to splitpoint group 0's phase 1 and phase 2 respectively) always
> +appear at block numbers 1 and 2, just after the meta page.
>
> This explanation doesn't seem to be right as with current patch we
> start phased allocation only after splitpoint group 9.
Again a mistake, removed the sentence in parentheses.
> 2.
> -#define HASH_MAX_SPLITPOINTS 32
> #define HASH_MAX_BITMAPS 128
>
> +#define SPLITPOINT_PHASES_PER_GRP 4
> +#define SPLITPOINT_PHASE_MASK (SPLITPOINT_PHASES_PER_GRP - 1)
> +#define SPLITPOINT_GROUPS_WITH_ONLY_ONE_PHASE 10
> +#define HASH_MAX_SPLITPOINTS \
> + ((32 - SPLITPOINT_GROUPS_WITH_ONLY_ONE_PHASE) * \
> + SPLITPOINT_PHASES_PER_GRP) + \
> + SPLITPOINT_GROUPS_WITH_ONLY_ONE_PHASE
>
> You have changed the value of HASH_MAX_SPLITPOINTS, but the comments
> explaining that value are still unchanged. Refer below text.
> "The limitation on the size of spares[] comes from the fact that there's
> * no point in having more than 2^32 buckets with only uint32 hashcodes."
The limitation is still indirectly imposed by the fact that we can
have only 2^32 buckets. But I also added a note that
HASH_MAX_SPLITPOINTS also considers that after
SPLITPOINT_GROUPS_WITH_ONLY_ONE_PHASE bucket allocation will be done
in multiple(exactly 4) phases.
--
Thanks and Regards
Mithun C Y
EnterpriseDB: http://www.enterprisedb.com
