From: | Mithun Cy <mithun(dot)cy(at)enterprisedb(dot)com> |
---|---|
To: | Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> |
Cc: | Robert Haas <robertmhaas(at)gmail(dot)com>, David Steele <david(at)pgmasters(dot)net>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: [POC] A better way to expand hash indexes. |
Date: | 2017-03-29 12:03:21 |
Message-ID: | CAD__OuimuLaHYiRPQeHzSjHPE+vFybB+SkMaRZ5MgRen0HW5DA@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Thanks, Amit for a detailed review.
On Wed, Mar 29, 2017 at 4:09 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)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
Attachment | Content-Type | Size |
---|---|---|
yet_another_expand_hashbucket_efficiently_11.patch | application/octet-stream | 20.2 KB |
sortbuild_hash_B_2.patch | application/octet-stream | 4.3 KB |
sortbuild_hash_A.patch | application/octet-stream | 5.6 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Andreas Karlsson | 2017-03-29 12:04:38 | Re: Refactor handling of database attributes between pg_dump and pg_dumpall |
Previous Message | Mithun Cy | 2017-03-29 11:55:05 | Re: [POC] A better way to expand hash indexes. |