Thanks, Amit for the review. On Sat, Mar 25, 2017 at 7:03 PM, Amit Kapila <amit.kapil...@gmail.com> wrote: > > I think one-dimensional patch has fewer places to touch, so that looks > better to me. However, I think there is still hard coding and > assumptions in code which we should try to improve.
Great!, I will continue with spares 1-dimensional improvement. > > 1. > + /* > + * The first 4 bucket belongs to first splitpoint group 0. And since group > + * 0 have 4 = 2^2 buckets, we double them in group 1. So total buckets > + * after group 1 is 8 = 2^3. Then again at group 2 we add another 2^3 > + * buckets to double the total number of buckets, which will become 2^4. I > + * think by this time we can see a pattern which say if num_bucket > 4 > + * splitpoint group = log2(num_bucket) - 2 > + */ > > + if (num_bucket <= 4) > + splitpoint_group = 0; /* converted to base 0. */ > + else > + splitpoint_group = _hash_log2(num_bucket) - 2; > > This patch defines split point group zero has four buckets and based > on that above calculation is done. I feel you can define it like > #define Buckets_First_Split_Group 4 and then use it in above code. > Also, in else part number 2 looks awkward, can we define it as > log2_buckets_first_group = _hash_log2(Buckets_First_Split_Group) or > some thing like that. I think that way code will look neat. I don't > like the way above comment is worded even though it is helpful to > understand the calculation. If you want, then you can add such a > comment in file header, here it looks out of place. I have removed the comments. And, defined a new macro which maps bucket to SPLIT GROUP #define BUCKET_TO_SPLITPOINT_GRP(num_bucket) \ ((num_bucket <= Buckets_First_Split_Group) ? 0 : \ (_hash_log2(num_bucket) - 2)) I could not find a way to explain why minus 2? better than " The splitpoint group of a given bucket can be taken as (_hash_log2(bucket) - 2). Subtracted by 2 because each group have 2 ^ (x + 2) buckets.". Now I have added those with existing comments I think that should make it little better. Adding comments about spares array in hashutils.c's file header did not appear right to me. I think README has some details about same. > 2. > +power-of-2 groups, called "split points" in the code. That means on every > new > +split points we double the existing number of buckets. > > split points/split point Fixed. > 3. > + * which phase of allocation the bucket_num belogs to with in the group. > > /belogs/belongs Fixed -- Thanks and Regards Mithun C Y EnterpriseDB: http://www.enterprisedb.com
Description: Binary data
-- Sent via pgsql-hackers mailing list (email@example.com) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers