Hi, >> I've assigned to review this patch. >> At first, I'd like to notice that I like idea and general design. >> Secondly, patch set don't apply cleanly to master. Please, rebase it. > > > Thanks for showing your interest towards this patch. I would like to > inform that this patch has got dependency on patch for 'Write Ahead > Logging in hash index - [1]' and 'Microvacuum support in hash index > [1]'. Hence, until above two patches becomes stable I may have to keep > on rebasing this patch. However, I will try to share you the updated > patch asap. > >> >> >> On Tue, Feb 14, 2017 at 8:27 AM, Ashutosh Sharma <ashu.coe...@gmail.com> >> wrote: >>> >>> 1) 0001-Rewrite-hash-index-scans-to-work-a-page-at-a-time.patch: this >>> patch rewrites the hash index scan module to work in page-at-a-time >>> mode. It basically introduces two new functions-- _hash_readpage() and >>> _hash_saveitem(). The former is used to load all the qualifying tuples >>> from a target bucket or overflow page into an items array. The latter >>> one is used by _hash_readpage to save all the qualifying tuples found >>> in a page into an items array. Apart from that, this patch bascially >>> cleans _hash_first(), _hash_next and hashgettuple(). >> >> >> I see that forward and backward scan cases of function _hash_readpage >> contain a lot of code duplication >> Could you please refactor this function to have less code duplication? > > Sure, I will try to avoid the code duplication as much as possible.
I had close look into hash_readpage() function and could see that there are only few if-else conditions which are similar for both forward and backward scan cases and can't be optimised further. However, If you have a cursory look into this function definition it looks like the code for forward and backward scan are exactly the same but that's not the case. Attached is the diff report (hash_readpage.html) for forward and backward scan code used in hash_readpage(). This shows what all lines in the hash_readpage() are same or different. Please note that before applying the patches for page scan mode in hash index you may need to first apply the following patches on HEAD, 1) v10 patch for WAL in hash index - [1] 2) v1 patch for wal consistency check for hash index - [2] 3) v6 patch for microvacuum support in hash index - [3] [1] - https://www.postgresql.org/message-id/CAA4eK1%2Bk5wR4-kAjPqLoKemuHayQd6RkQQT9gheTfpn%2B72o1UA%40mail.gmail.com [2] - https://www.postgresql.org/message-id/CAGz5QCKPU2qX75B1bB_LuEC88xWZa5L55J0TLvYMVD8noSH3pA%40mail.gmail.com [3] - https://www.postgresql.org/message-id/CAE9k0PkYpAPDJBfgia08o7XhO8nypH9WoO9M8%3DdqLrwwObXKcw%40mail.gmail.com -- With Regards, Ashutosh Sharma EnterpriseDB:http://www.enterprisedb.comTitle: Text Compare Text Compare
Produced: 3/14/2017 4:32:14 PM
Mode: All
Left file: C:\Users\ashu\Downloads\fwd_scan.txt Right file: C:\Users\ashu\Downloads\bwd_scan.txt
| +loop_top_fwd: | <> | +loop_top_bwd: |
| + /* load items[] in ascending order */ | + /* load items[] in descending order */ | |
| + itemIndex = 0; | + itemIndex = MaxIndexTuplesPerPage; | |
| + | = | + |
| + /* new page, locate starting position by binary search */ | + /* new page, locate starting position by binary search */ | |
| + offnum = _hash_binsearch(page, so->hashso_sk_hash); | <> | + offnum = _hash_binsearch_last(page, so->hashso_sk_hash); |
| + | = | + |
| + while (offnum <= maxoff) | <> | + while (offnum >= FirstOffsetNumber) |
| + { | = | + { |
| + Assert(offnum >= FirstOffsetNumber); | <> | + Assert(offnum <= maxoff); |
| + itup = (IndexTuple) PageGetItem(page, PageGetItemId(page, offnum)); | = | + itup = (IndexTuple) PageGetItem(page, PageGetItemId(page, offnum)); |
| + | + | |
| + /* | + /* | |
| + * skip the tuples that are moved by split operation | + * skip the tuples that are moved by split operation | |
| + * for the scan that has started when split was in | + * for the scan that has started when split was in | |
| + * progress. Also, skip the tuples that are marked | + * progress. Also, skip the tuples that are marked | |
| + * as dead. | + * as dead. | |
| + */ | + */ | |
| + if ((so->hashso_buc_populated && !so->hashso_buc_split && | + if ((so->hashso_buc_populated && !so->hashso_buc_split && | |
| + (itup->t_info & INDEX_MOVED_BY_SPLIT_MASK)) || | + (itup->t_info & INDEX_MOVED_BY_SPLIT_MASK)) || | |
| + (scan->ignore_killed_tuples && | + (scan->ignore_killed_tuples && | |
| + (ItemIdIsDead(PageGetItemId(page, offnum))))) | + (ItemIdIsDead(PageGetItemId(page, offnum))))) | |
| + { | + { | |
| + offnum = OffsetNumberNext(offnum); /* move forward */ | <> | + offnum = OffsetNumberPrev(offnum); /* move back */ |
| + continue; | = | + continue; |
| + } | + } | |
| + | + | |
| + if (so->hashso_sk_hash == _hash_get_indextuple_hashkey(itup) && | + if (so->hashso_sk_hash == _hash_get_indextuple_hashkey(itup) && | |
| + _hash_checkqual(scan, itup)) | + _hash_checkqual(scan, itup)) | |
| + { | + { | |
| -+ | + itemIndex--; | |
| + /* tuple is qualified, so remember it */ | = | + /* tuple is qualified, so remember it */ |
| + _hash_saveitem(so, itemIndex, offnum, itup); | + _hash_saveitem(so, itemIndex, offnum, itup); | |
| + itemIndex++; | +- | |
| + } | = | + } |
| + | + | |
| + offnum = OffsetNumberNext(offnum); | <> | + offnum = OffsetNumberPrev(offnum); |
| + } | = | + } |
| + | + | |
| + Assert(itemIndex <= MaxIndexTuplesPerPage); | <> | + Assert(itemIndex >= 0); |
| + | = | + |
| + if (itemIndex == 0) | <> | + if (itemIndex == MaxIndexTuplesPerPage) |
| + { | = | + { |
| + /* | + /* | |
| + * Could not find any matching tuples in the current page, move | + * Could not find any matching tuples in the current page, move | |
| + * to the next page. Before leaving the current page, also deal | <> | + * to the prev page. Before leaving the current page, also deal |
| + * with any killed items. | = | + * with any killed items. |
| + */ | + */ | |
| + if (so->numKilled > 0) | + if (so->numKilled > 0) | |
| + _hash_kill_items(scan); | + _hash_kill_items(scan); | |
| + | + | |
| + _hash_readnext(scan, &buf, &page, &opaque); | <> | + _hash_readprev(scan, &buf, &page, &opaque); |
| + if (BufferIsValid(buf)) | = | + if (BufferIsValid(buf)) |
| + { | + { | |
| + so->currPos.buf = buf; | + so->currPos.buf = buf; | |
| + so->currPos.currPage = BufferGetBlockNumber(buf); | + so->currPos.currPage = BufferGetBlockNumber(buf); | |
| + maxoff = PageGetMaxOffsetNumber(page); | + maxoff = PageGetMaxOffsetNumber(page); | |
| + offnum = _hash_binsearch(page, so->hashso_sk_hash); | <> | + offnum = _hash_binsearch_last(page, so->hashso_sk_hash); |
| + goto loop_top_fwd; | + goto loop_top_bwd; | |
| + } | = | + } |
| + else | + else | |
| + return false; | + return false; | |
| + } | + } | |
| + else | + else | |
| + { | + { | |
| + if (so->currPos.buf == so->hashso_bucket_buf || | + if (so->currPos.buf == so->hashso_bucket_buf || | |
| + so->currPos.buf == so->hashso_split_bucket_buf) | + so->currPos.buf == so->hashso_split_bucket_buf) | |
| + LockBuffer(so->currPos.buf, BUFFER_LOCK_UNLOCK); | + LockBuffer(so->currPos.buf, BUFFER_LOCK_UNLOCK); | |
| + else | + else | |
| + _hash_relbuf(rel, so->currPos.buf); | + _hash_relbuf(rel, so->currPos.buf); | |
| + | <> | |
| + so->currPos.nextPage = (opaque)->hasho_nextblkno; | + so->currPos.prevPage = (opaque)->hasho_prevblkno; | |
| + } | = | + } |
| + | + | |
| + so->currPos.firstItem = 0; | <> | + so->currPos.firstItem = itemIndex; |
| + so->currPos.lastItem = itemIndex - 1; | + so->currPos.lastItem = MaxIndexTuplesPerPage - 1; | |
| + so->currPos.itemIndex = 0; | + so->currPos.itemIndex = MaxIndexTuplesPerPage - 1; | |
| + } | = | + } |
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
