On Thu, Jan 5, 2017 at 11:38 AM, Mithun Cy <mithun.cy@enterprisedb.com> wrote:
> On Wed, Jan 4, 2017 at 5:21 PM, Mithun Cy <mithun.cy@enterprisedb.com> wrote:
> I have re-based the patch to fix one compilation warning
> @_hash_doinsert where variable bucket was only used for Asserting but
> was not declared about its purpose.
>
Few more comments:
1.}
+
+
+/*
+ * _hash_getbucketbuf_from_hashkey() -- Get the bucket's buffer for the given
no need to two extra lines, one is sufficient and matches the nearby
coding pattern.
2.
+ * old bucket in case of bucket split, see @_hash_doinsert().
Do you see anywhere else in the code the pattern of using @ symbol in
comments before function name? In general, there is no harm in using
it, but maybe it is better to be consistent with usage at other
places.
3.
+ /*
+ * @_hash_getbucketbuf_from_hashkey we have verified the hasho_bucket.
+ * Should be safe to use further.
+ */
+ bucket = pageopaque->hasho_bucket;
/* * If this bucket is in the process of being split, try to finish the
@@ -152,7 +107,9 @@ restart_insert: LockBuffer(buf, BUFFER_LOCK_UNLOCK);
_hash_finish_split(rel, metabuf, buf, pageopaque->hasho_bucket,
- maxbucket, highmask, lowmask);
+ usedmetap->hashm_maxbucket,
after this change, i think you can directly use bucket in
_hash_finish_split instead of pageopaque->hasho_bucket
4.
@@ -154,8 +154,11 @@ _hash_readprev(IndexScanDesc scan, *bufp = InvalidBuffer; /* check for interrupts while we're not
holdingany buffer lock */ CHECK_FOR_INTERRUPTS();
- if (BlockNumberIsValid(blkno))
+
+ /* If it is a bucket page there will not be a prevblkno. */
+ if (!((*opaquep)->hasho_flag & LH_BUCKET_PAGE))
Won't the check similar to the existing check (if (*bufp ==
so->hashso_bucket_buf || *bufp == so->hashso_split_bucket_buf)) just
above this code will suffice the need? If so, then you can check it
once and use it in both places.
5. The reader and insertion algorithm needs to be updated in README.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com