Re: [HACKERS] Page Scan Mode in Hash Index
От | Amit Kapila |
---|---|
Тема | Re: [HACKERS] Page Scan Mode in Hash Index |
Дата | |
Msg-id | CAA4eK1LnQi51L8+msZasZ926fbiO7BGpjeaY5XcgQn3Xdcvm3w@mail.gmail.com обсуждение исходный текст |
Ответ на | Re: [HACKERS] Page Scan Mode in Hash Index (Ashutosh Sharma <ashu.coek88@gmail.com>) |
Ответы |
Re: [HACKERS] Page Scan Mode in Hash Index
(Robert Haas <robertmhaas@gmail.com>)
Re: [HACKERS] Page Scan Mode in Hash Index (Ashutosh Sharma <ashu.coek88@gmail.com>) |
Список | pgsql-hackers |
On Sun, Jul 30, 2017 at 2:07 PM, Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > Hi, > > On Wed, May 10, 2017 at 2:28 PM, Ashutosh Sharma <ashu.coek88@gmail.com> wrote: >> While doing the code coverage testing of v7 patch shared with - [1], I >> found that there are few lines of code in _hash_next() that are >> redundant and needs to be removed. I basically came to know this while >> testing the scenario where a hash index scan starts when a split is in >> progress. I have removed those lines and attached is the newer version >> of patch. >> > > Please find the new version of patches for page at a time scan in hash > index rebased on top of latest commit in master branch. Also, i have > ran pgindent script with pg_bsd_indent version 2.0 on all the modified > files. Thanks. > Few comments: 1. +_hash_kill_items(IndexScanDesc scan, bool havePin) I think you can do without the second parameter. Can't we detect inside _hash_kill_items whether the page is pinned or not as we do for btree? 2. + /* + * We remember prev and next block number along with current block + * number so that if fetching the tup- les using cursor we know + * the page from where to startwith. This is for the case where we + * have re- ached the end of bucket chain without finding any + * matching tuples. The spelling of tuples and reached contain some unwanted symbol. Have space between "startwith" or just use "begin" 3. + if (!BlockNumberIsValid((opaque)->hasho_nextblkno)) + { + so->currPos.prevPage = (opaque)->hasho_prevblkno; + so->currPos.nextPage = InvalidBlockNumber; + } There is no need to use Parentheses around opaque. I mean there is no problem with that, but it is redundant and makes code less readable. Also, there is similar usage at other places in the code, please change all another place as well. I think you can save the value of prevblkno in a local variable and use it in else part. 4. + if (so->hashso_sk_hash == _hash_get_indextuple_hashkey(itup) && + _hash_checkqual(scan, itup)) + { + /* tuple is qualified, so remember it */ + _hash_saveitem(so, itemIndex, offnum, itup); + itemIndex++; + } + else + + /* + * No more matching tuples exist in this page. so, exit while + * loop. + */ + break; It is better to have braces for the else part. It makes code look better. The type of code exists few line down as well, change that as well. 5. + /* + * We save the LSN of the page as we read it, so that we know whether it + * safe to apply LP_DEAD hints to the page later. + */ "whether it safe"/"whether it is safe" -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
В списке pgsql-hackers по дате отправления:
Предыдущее
От: Masahiko SawadaДата:
Сообщение: Re: [HACKERS] pgbench: Skipping the creating primary keys after initialization