Re: Page Scan Mode in Hash Index

From: Ashutosh Sharma <ashu(dot)coek88(at)gmail(dot)com>
To: Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
Cc: Robert Haas <robertmhaas(at)gmail(dot)com>, Jesper Pedersen <jesper(dot)pedersen(at)redhat(dot)com>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Page Scan Mode in Hash Index
Date: 2017-08-07 12:20:05
Message-ID: CAE9k0PnZB6cCRSFjVG5SLK180LNHH8gZ-Hsb_Q3KgzOtn_bpFQ@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: pgsql-hackers

On Fri, Aug 4, 2017 at 7:14 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> 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:

Thanks for reviewing the patch.

> 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?

Okay, done that way. Please check attached v10 patch.

>
> 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"

Corrected.

>
> 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.

Removed parenthesis around opaque.

I think you can save the value of
> prevblkno in a local variable and use it in else part.

Okay, done that way.

>
> 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.

Added braces in the else part.

>
> 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"

Corrected.

--
With Regards,
Ashutosh Sharma
EnterpriseDB:http://www.enterprisedb.com

Attachment Content-Type Size
0001-Rewrite-hash-index-scan-to-work-page-at-a-time_v10.patch text/x-patch 32.8 KB
0002-Remove-redundant-hash-function-_hash_step-and-do-som.patch text/x-patch 8.5 KB
0003-Improve-locking-startegy-during-VACUUM-in-Hash-Index.patch text/x-patch 3.9 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Ashutosh Sharma 2017-08-07 12:37:01 free space % calculation in pgstathashindex
Previous Message Alvaro Herrera 2017-08-07 11:51:18 Re: Adding hook in BufferSync for backup purposes