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.kapil...@gmail.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
cache_hash_index_meta_page_09.patch
Description: Binary data
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
