Thanks, Amit for a detailed review. On Wed, Mar 29, 2017 at 4:09 PM, Amit Kapila <amit.kapil...@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
yet_another_expand_hashbucket_efficiently_11.patch
Description: Binary data
sortbuild_hash_B_2.patch
Description: Binary data
sortbuild_hash_A.patch
Description: Binary data
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers