From: | Mithun Cy <mithun(dot)cy(at)enterprisedb(dot)com> |
---|---|
To: | Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> |
Cc: | 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-28 09:00:55 |
Message-ID: | CAD__OuiD00hOygJBcx9GTYxv5m5Ux0FpE9Am=dX4DYoMn_aeAQ@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Tue, Mar 28, 2017 at 12:21 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> I think both way it can work. I feel there is no hard pressed need
> that we should make the computation in sorting same as what you do
> _hash_doinsert. In patch_B, I don't think new naming for variables is
> good.
>
> Assert(!a->isnull1);
> - hash1 = DatumGetUInt32(a->datum1) & state->hash_mask;
> + bucket1 = DatumGetUInt32(a->datum1) % state->num_buckets;
>
> Can we use hash_mod instead of num_buckets and retain hash1 in above
> code and similar other places?
Yes done renamed it to hash_mod.
>
> Few comments:
> 1.
> +#define BUCKETS_WITHIN_SP_GRP(sp_g, nphase) \
> + ((sp_g < SPLITPOINT_GROUPS_WITH_ONLY_ONE_PHASE) ? \
> + (1 << (sp_g - 1)) : \
> + ((nphase) * ((1 << (sp_g - 1)) >> 2)))
>
> This will go wrong for split point group zero. In general, I feel if
> you handle computation for split groups lesser than
> SPLITPOINT_GROUPS_WITH_ONLY_ONE_PHASE in the caller, then all your
> macros will become much simpler and less error prone.
Fixed, apart from SPLITPOINT_PHASE_TO_SPLITPOINT_GRP rest all macros
only handle multi phase group. The SPLITPOINT_PHASE_TO_SPLITPOINT_GRP
is used in one more place at expand index so thought kepeping it as it
is is better.
.
> 2.
> +#define BUCKET_TO_SPLITPOINT_GRP(num_bucket) (_hash_log2(num_bucket))
>
> What is the use of such a define, can't we directly use _hash_log2 in
> the caller?
No real reason, just that NAMED macro appeared more readable than just
_hash_log2. Now I have removed same.
> --
> With Regards,
> Amit Kapila.
> EnterpriseDB: http://www.enterprisedb.com
--
Thanks and Regards
Mithun C Y
EnterpriseDB: http://www.enterprisedb.com
Attachment | Content-Type | Size |
---|---|---|
yet_another_expand_hashbucket_efficiently_09.patch | application/octet-stream | 19.9 KB |
sortbuild_hash_B_2.patch | application/octet-stream | 4.3 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Rushabh Lathia | 2017-03-28 09:07:39 | Re: crashes due to setting max_parallel_workers=0 |
Previous Message | Amit Langote | 2017-03-28 08:25:25 | Re: pg_dump emits ALTER TABLE ONLY partitioned_table |