On Tue, Jan 17, 2017 at 10:07 AM, Amit Kapila <amit.kapil...@gmail.com> wrote: > 1. > (a) I think you can retain the previous comment or modify it slightly. > Just removing the whole comment and replacing it with a single line > seems like a step backward.
-- Fixed, Just modified to say it > (b) Another somewhat bigger problem is that with this new change it > won't retain the pin on meta page till the end which means we might > need to perform an I/O again during operation to fetch the meta page. > AFAICS, you have just changed it so that you can call new API > _hash_getcachedmetap, if that's true, then I think you have to find > some other way of doing it. BTW, why can't you design your new API > such that it always take pinned metapage? You can always release the > pin in the caller if required. I understand that you don't always > need a metapage in that API, but I think the current usage of that API > is also not that good. -- Yes what you say is right. I wanted to make _hash_getcachedmetap API self-sufficient. But always 2 possible consecutive reads for metapage are connected as we pin the page to buffer to avoid I/O. Now redesigned the API such a way that caller pass pinned meta page if we want to set the metapage cache; So this gives us the flexibility to use the cached meta page data in different places. > 2. > + if (bucket_opaque->hasho_prevblkno != InvalidBlockNumber || > + bucket_opaque->hasho_prevblkno > cachedmetap->hashm_maxbucket) > + cachedmetap = _hash_getcachedmetap(rel, true); > > I don't understand the meaning of above if check. It seems like you > will update the metapage when previous block number is not a valid > block number which will be true at the first split. How will you > ensure that there is a re-split and cached metapage is not relevant. > I think if there is && in the above condition, then we can ensure it. > -- Oops that was a mistake corrected as you stated. > 3. > + Given a hashkey get the target bucket page with read lock, using cached > + metapage. The getbucketbuf_from_hashkey method below explains the same. > + > > All the sentences in algorithm start with small letters, then why do > you need an exception for this sentence. I think you don't need to > add an empty line. Also, I think the usage of > getbucketbuf_from_hashkey seems out of place. How about writing it > as: > > The usage of cached metapage is explained later. > -- Fixed as like you have asked. > > 4. > + If target bucket is split before metapage data was cached then we are > + done. > + Else first release the bucket page and then update the metapage cache > + with latest metapage data. > > I think it is better to retain original text of readme and add about > meta page update. > -- Fixed. Now where ever it is meaning full I have kept original wordings. But, the way we get to right target buffer after the latest split is slightly different than what the original code did. So there is a slight modification to show we use metapage cache. > 5. > + Loop: > .. > .. > + Loop again to reach the new target bucket. > > No need to write "Loop again ..", that is implicit. > -- Fixed as liked you have asked. -- Thanks and Regards Mithun C Y EnterpriseDB: http://www.enterprisedb.com
cache_hash_index_meta_page_12.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
