Re: Cache Hash Index meta page.

Thanks Amit for detailed review, and pointing out various issues in
the patch. I have tried to fix all of your comments as below.

On Mon, Jan 2, 2017 at 11:29 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:

> 1.
> usage "into to .." in above comment seems to be wrong.usage "into to .." in above comment seems to be wrong.usage "into to .." in above comment seems to be wrong.usage "into to .." in above comment seems to be wrong.

-- Fixed.

> 2.
> In the above comment, a reference to HashMetaCache is confusing, are
> your referring to any structure here? If you change this, consider toyour referring to any structure here? If you change this, consider toyour referring to any structure here? If you change this, consider toyour referring to any structure here? If you change this, consider to
> change the similar usage at other places in the patch as well.change the similar usage at other places in the patch as well.change the similar usage at other places in the patch as well.change the similar usage at other places in the patch as well.

-- Fixed. Removed HashMetaCache everywhere in the code. Where ever
needed added HashMetaPageData.

> Also, it is not clear to what do you mean by ".. to indicate bucketto indicate bucket
> has been initialized .."? assigning maxbucket as a special value tohas been initialized .."? assigning maxbucket as a special value to
> prevblkno is not related to initializing a bucket page.prevblkno is not related to initializing a bucket page.

-- To be consistent with our definition of prevblkno, I am setting
prevblkno with current hashm_maxbucket when we initialize the bucket
page. I have tried to correct the comments accordingly.

> 3.
> In above comment, where you are saying ".. caching the some of the
> meta page data .." is slightly confusing, because it appears to me
> that you are caching whole of the metapage not some part of it.

-- Fixed. Changed to caching the HashMetaPageData.

> 4.
> +_hash_getbucketbuf_from_hashkey(uint32 hashkey, Relation rel,
>
> Generally, for _hash_* API's, we use rel as the first parameter, so I
> think it is better to maintain the same with this API as well.

-- Fixed.

> 5.
> _hash_finish_split(rel, metabuf, buf, pageopaque->hasho_bucket,
> - maxbucket, highmask, lowmask);
> + usedmetap->hashm_maxbucket,
> + usedmetap->hashm_highmask,
> + usedmetap->hashm_lowmask);

> I think we should add an Assert for the validity of usedmetap before using it.

-- Fixed. Added Assert(usedmetap != NULL);

> 6. Getting few compilation errors in assert-enabled build.
>

-- Fixed. Sorry, I missed handling bucket number which is needed at
below codes. I have fixed same now.

> 7.
> I can understand what you want to say in above comment, but I think
> you can write it in somewhat shorter form. There is no need to
> explain the exact check.

-- Fixed. I have tried to compress it into a few lines.

> 8.
> @@ -152,6 +152,11 @@ _hash_readprev(IndexScanDesc scan,
> _hash_relbuf(rel, *bufp);
>
> *bufp = InvalidBuffer;
> +
> + /* If it is a bucket page there will not be a prevblkno. */
> + if ((*opaquep)->hasho_flag & LH_BUCKET_PAGE)
> + return;
> +
>
> I don't think above check is right. Even if it is a bucket page, we
> might need to scan bucket being populated, refer check else if
> (so->hashso_buc_populated && so->hashso_buc_split). Apart from that,
> you can't access bucket page after releasing the lock on it. Why have
> you added such a check?
>

-- Fixed. That was a mistake, now I have fixed it. Actually, if bucket
page is passed to _hash_readprev then there will not be a prevblkno.
But from this patch onwards prevblkno of bucket page will store
hashm_maxbucket. So a check BlockNumberIsValid (blkno) will not be
valid anymore. I have fixed by adding as below.
+ /* If it is a bucket page there will not be a prevblkno. */
+ if (!((*opaquep)->hasho_flag & LH_BUCKET_PAGE))
{
+ Assert(BlockNumberIsValid(blkno));

There are 2 other places which does same @_hash_freeovflpage,
@_hash_squeezebucket.
But that will only be called for overflow pages. So I did not make
changes. But I think we should also change there to make it
consistent.

--
Thanks and Regards
Mithun C Y
EnterpriseDB: http://www.enterprisedb.com