Re: Page Scan Mode in Hash Index

On Sun, Jul 30, 2017 at 2:07 PM, Ashutosh Sharma <ashu(dot)coek88(at)gmail(dot)com> wrote:
> Hi,
>
> On Wed, May 10, 2017 at 2:28 PM, Ashutosh Sharma <ashu(dot)coek88(at)gmail(dot)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