Re: [HACKERS] Page Scan Mode in Hash Index

On Mon, Sep 25, 2017 at 12:25 AM, Amit Kapila wrote: > I think your proposal makes sense. Your patch looks good but you > might want to tweak the comments atop _hash_kill_items ("However, > having pin on the overflow page doesn't guarantee that vacuum won't > delete any

Re: [HACKERS] Page Scan Mode in Hash Index

On Fri, Sep 22, 2017 at 11:56 PM, Robert Haas wrote: > On Thu, Sep 21, 2017 at 3:08 AM, Ashutosh Sharma > wrote: >> I have added a note for handling of logged and unlogged tables in >> README file and also corrected the header comment for >>

Re: [HACKERS] Page Scan Mode in Hash Index

On Fri, Sep 22, 2017 at 11:56 PM, Robert Haas wrote: > On Thu, Sep 21, 2017 at 3:08 AM, Ashutosh Sharma > wrote: >> I have added a note for handling of logged and unlogged tables in >> README file and also corrected the header comment for >>

Re: [HACKERS] Page Scan Mode in Hash Index

On Thu, Sep 21, 2017 at 3:08 AM, Ashutosh Sharma wrote: > I have added a note for handling of logged and unlogged tables in > README file and also corrected the header comment for > hashbucketcleanup(). Please find the attached 0003*.patch having these > changes. Thanks. I

Re: [HACKERS] Page Scan Mode in Hash Index

On Thu, Sep 21, 2017 at 9:30 AM, Robert Haas wrote: > On Wed, Sep 20, 2017 at 11:43 AM, Ashutosh Sharma > wrote: >> Attached are the patches with above changes. Thanks. > > Thanks. I think that the comments and README changes in 0003 need >

Re: [HACKERS] Page Scan Mode in Hash Index

On Thu, Sep 21, 2017 at 9:30 AM, Robert Haas wrote: > On Wed, Sep 20, 2017 at 11:43 AM, Ashutosh Sharma > wrote: >> Attached are the patches with above changes. Thanks. > > Thanks. I think that the comments and README changes in 0003 need >

Re: [HACKERS] Page Scan Mode in Hash Index

On Wed, Sep 20, 2017 at 11:43 AM, Ashutosh Sharma wrote: > Attached are the patches with above changes. Thanks. Thanks. I think that the comments and README changes in 0003 need significantly more work. In several places, they fail to note the unlogged vs. logged

Re: [HACKERS] Page Scan Mode in Hash Index

On Wed, Sep 20, 2017 at 8:05 PM, Robert Haas wrote: > On Wed, Sep 20, 2017 at 5:37 AM, Ashutosh Sharma > wrote: >> Thanks for all your review comments. Please find my comments in-line. > > +if

Re: [HACKERS] Page Scan Mode in Hash Index

On Wed, Sep 20, 2017 at 5:37 AM, Ashutosh Sharma wrote: > Thanks for all your review comments. Please find my comments in-line. +if (!BlockNumberIsValid(opaque->hasho_nextblkno)) +{ +if (so->currPos.buf == so->hashso_bucket_buf || +

Re: [HACKERS] Page Scan Mode in Hash Index

On Wed, Sep 20, 2017 at 6:44 PM, Robert Haas wrote: > On Wed, Sep 20, 2017 at 7:45 AM, Amit Kapila wrote: >> Right, I was thinking from the perspective of the index entry. Before >> marking index entry as dead, we do check for heaptid. So, as

Re: [HACKERS] Page Scan Mode in Hash Index

On Wed, Sep 20, 2017 at 7:45 AM, Amit Kapila wrote: > Right, I was thinking from the perspective of the index entry. Before > marking index entry as dead, we do check for heaptid. So, as heaptid > can't be reused via Page-at-a-time index vacuum, scan won't mark index >

Re: [HACKERS] Page Scan Mode in Hash Index

On Wed, Sep 20, 2017 at 4:56 PM, Robert Haas wrote: > On Wed, Sep 20, 2017 at 7:19 AM, Amit Kapila wrote: >>> Page-at-a-time index vacuum as in _hash_vacuum_one_page doesn't matter >>> because such an operation doesn't allow TIDs to be reused. >>

Re: [HACKERS] Page Scan Mode in Hash Index

On Wed, Sep 20, 2017 at 7:19 AM, Amit Kapila wrote: >> Page-at-a-time index vacuum as in _hash_vacuum_one_page doesn't matter >> because such an operation doesn't allow TIDs to be reused. > > Page-at-a-time index vacuum also allows TIDs to be reused but this is > done

Re: [HACKERS] Page Scan Mode in Hash Index

On Wed, Sep 20, 2017 at 4:04 PM, Robert Haas wrote: > On Tue, Sep 19, 2017 at 11:34 PM, Amit Kapila wrote: >> This point has been discussed above [1] and to avoid this problem we >> are keeping the scan always behind vacuum for unlogged and

Re: [HACKERS] Page Scan Mode in Hash Index

On Tue, Sep 19, 2017 at 11:34 PM, Amit Kapila wrote: > This point has been discussed above [1] and to avoid this problem we > are keeping the scan always behind vacuum for unlogged and temporary > tables as we are doing without this patch. That will ensure vacuum >

Re: [HACKERS] Page Scan Mode in Hash Index

Thanks for all your review comments. Please find my comments in-line. On Tue, Sep 19, 2017 at 9:49 PM, Robert Haas wrote: > On Thu, Aug 24, 2017 at 11:26 AM, Jesper Pedersen > wrote: >> Based on the feedback in this thread, I have moved the

Re: [HACKERS] Page Scan Mode in Hash Index

On Tue, Sep 19, 2017 at 9:49 PM, Robert Haas wrote: > On Thu, Aug 24, 2017 at 11:26 AM, Jesper Pedersen > wrote: >> Based on the feedback in this thread, I have moved the patch to "Ready for >> Committer". > > Reviewing 0001: > > _hash_readpage

Re: [HACKERS] Page Scan Mode in Hash Index

On Thu, Aug 24, 2017 at 11:26 AM, Jesper Pedersen wrote: > Based on the feedback in this thread, I have moved the patch to "Ready for > Committer". Reviewing 0001: _hash_readpage gets the page LSN to see if we can apply LP_DEAD hints, but if the table is unlogged or

Re: [HACKERS] Page Scan Mode in Hash Index

On 08/24/2017 01:21 AM, Ashutosh Sharma wrote: Done. Attached are the patches with above changes. Thanks ! Based on the feedback in this thread, I have moved the patch to "Ready for Committer". Best regards, Jesper -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)

Re: [HACKERS] Page Scan Mode in Hash Index

On Wed, Aug 23, 2017 at 7:39 PM, Jesper Pedersen wrote: > On 08/23/2017 07:38 AM, Amit Kapila wrote: >> >> Thanks for the new version. I again looked at the patches and fixed >> quite a few comments in the code and ReadMe. You have forgotten to >> update README for

Re: [HACKERS] Page Scan Mode in Hash Index

Hi Amit, On Wed, Aug 23, 2017 at 5:08 PM, Amit Kapila wrote: > On Tue, Aug 22, 2017 at 7:24 PM, Ashutosh Sharma > wrote: >> On Tue, Aug 22, 2017 at 3:55 PM, Amit Kapila wrote: >> >> Okay, I got your point now. I think,

Re: [HACKERS] Page Scan Mode in Hash Index

On 08/23/2017 07:38 AM, Amit Kapila wrote: Thanks for the new version. I again looked at the patches and fixed quite a few comments in the code and ReadMe. You have forgotten to update README for the changes in vacuum patch (0003-Improve-locking-startegy-during-VACUUM-in-Hash-Index_v7). I

Re: [HACKERS] Page Scan Mode in Hash Index

On Tue, Aug 22, 2017 at 7:24 PM, Ashutosh Sharma wrote: > On Tue, Aug 22, 2017 at 3:55 PM, Amit Kapila wrote: > > Okay, I got your point now. I think, currently in _hash_kill_items(), > if an overflow page is pinned we do not check if it got

Re: [HACKERS] Page Scan Mode in Hash Index

On Tue, Aug 22, 2017 at 3:55 PM, Amit Kapila wrote: > On Tue, Aug 22, 2017 at 2:28 PM, Ashutosh Sharma > wrote: >> On Sat, Aug 19, 2017 at 11:37 AM, Amit Kapila >> wrote: >>> On Fri, Aug 11, 2017 at 6:51 PM, Ashutosh

Re: [HACKERS] Page Scan Mode in Hash Index

On Tue, Aug 22, 2017 at 2:28 PM, Ashutosh Sharma wrote: > On Sat, Aug 19, 2017 at 11:37 AM, Amit Kapila wrote: >> On Fri, Aug 11, 2017 at 6:51 PM, Ashutosh Sharma >> wrote: 7. _hash_kill_items(IndexScanDesc

Re: [HACKERS] Page Scan Mode in Hash Index

On Sat, Aug 19, 2017 at 11:37 AM, Amit Kapila wrote: > On Fri, Aug 11, 2017 at 6:51 PM, Ashutosh Sharma > wrote: >>> >>> 7. >>> _hash_kill_items(IndexScanDesc scan) >>> { >>> .. >>> + /* >>> + * If page LSN differs it means that the page was

Re: [HACKERS] Page Scan Mode in Hash Index

On Fri, Aug 11, 2017 at 6:51 PM, Ashutosh Sharma wrote: >> >> 7. >> _hash_kill_items(IndexScanDesc scan) >> { >> .. >> + /* >> + * If page LSN differs it means that the page was modified since the >> + * last read. killedItems could be not valid so LP_DEAD hints apply- >> +

Re: [HACKERS] Page Scan Mode in Hash Index

> > Comments on the latest patch: Thank you once again for reviewing my patches. > > 1. > /* > + * We remember prev and next block number along with current block > + * number so that if fetching the tuples using cursor we know the > + * page from where to begin. This is for the case where we

Re: [HACKERS] Page Scan Mode in Hash Index

On Wed, Aug 9, 2017 at 2:58 PM, Amit Kapila wrote: > On Mon, Aug 7, 2017 at 5:50 PM, Ashutosh Sharma wrote: > > 7. > _hash_kill_items(IndexScanDesc scan) > { > .. > + /* > + * If page LSN differs it means that the page was modified since the > + *

Re: [HACKERS] Page Scan Mode in Hash Index

On Mon, Aug 7, 2017 at 5:50 PM, Ashutosh Sharma wrote: > On Fri, Aug 4, 2017 at 7:14 PM, Amit Kapila wrote: >> On Sun, Jul 30, 2017 at 2:07 PM, Ashutosh Sharma >> wrote: >>> Hi, >>> >>> On Wed, May 10, 2017 at 2:28 PM,

Re: [HACKERS] Page Scan Mode in Hash Index

On Fri, Aug 4, 2017 at 7:14 PM, Amit Kapila wrote: > On Sun, Jul 30, 2017 at 2:07 PM, Ashutosh Sharma > wrote: >> Hi, >> >> On Wed, May 10, 2017 at 2:28 PM, Ashutosh Sharma >> wrote: >>> While doing the code coverage

Re: [HACKERS] Page Scan Mode in Hash Index

On Fri, Aug 4, 2017 at 9:44 AM, Amit Kapila wrote: > 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. Amit, I'm sure you know this, but just for the benefit of anyone who

Re: [HACKERS] Page Scan Mode in Hash Index

On Sun, Jul 30, 2017 at 2:07 PM, Ashutosh Sharma wrote: > Hi, > > On Wed, May 10, 2017 at 2:28 PM, Ashutosh Sharma > wrote: >> While doing the code coverage testing of v7 patch shared with - [1], I >> found that there are few lines of code in

Re: [HACKERS] Page Scan Mode in Hash Index

Hi, On Wed, May 10, 2017 at 2:28 PM, Ashutosh Sharma 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 >

Re: [HACKERS] Page Scan Mode in Hash Index

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

Re: [HACKERS] Page Scan Mode in Hash Index

Hi, On Tue, Apr 4, 2017 at 3:56 PM, Amit Kapila wrote: > On Sun, Apr 2, 2017 at 4:14 AM, Ashutosh Sharma wrote: >> >> Please note that these patches needs to be applied on top of [1]. >> > > Few more review comments: > > 1. > - page =

Re: [HACKERS] Page Scan Mode in Hash Index

On Tue, Apr 4, 2017 at 6:29 AM, Amit Kapila wrote: > On Mon, Mar 27, 2017 at 7:04 PM, Ashutosh Sharma > wrote: >> My guess (which could be wrong) is that so->hashso_bucket_buf = >>> InvalidBuffer should be moved back up higher in the function

Re: [HACKERS] Page Scan Mode in Hash Index

On Mon, Mar 27, 2017 at 7:04 PM, Ashutosh Sharma wrote: > > My guess (which could be wrong) is that so->hashso_bucket_buf = >> InvalidBuffer should be moved back up higher in the function where it >> was before, just after the first if statement, and that the new >>

Re: [HACKERS] Page Scan Mode in Hash Index

On Sun, Apr 2, 2017 at 4:14 AM, Ashutosh Sharma wrote: > > Please note that these patches needs to be applied on top of [1]. > Few more review comments: 1. - page = BufferGetPage(so->hashso_curbuf); + blkno = so->currPos.currPage; + if (so->hashso_bucket_buf ==

Re: [HACKERS] Page Scan Mode in Hash Index

Hi, Thanks for the review. >>> I am suspicious that _hash_kill_items() is going to have problems if >>> the overflow page is freed before it reacquires the lock. >>> _btkillitems() contains safeguards against similar cases. >> >> I have added similar check for hash_kill_items() as well. >> > > I

Re: [HACKERS] Page Scan Mode in Hash Index

On Mon, Mar 27, 2017 at 7:04 PM, Ashutosh Sharma wrote: >> >> I am suspicious that _hash_kill_items() is going to have problems if >> the overflow page is freed before it reacquires the lock. >> _btkillitems() contains safeguards against similar cases. > > I have added

Re: [HACKERS] Page Scan Mode in Hash Index

Hi, > > On 03/29/2017 09:16 PM, Ashutosh Sharma wrote: >>> >>> This patch needs a rebase. >> >> >> Please try applying these patches on top of [1]. I think you should be >> able >> to apply it cleanly. Sorry, I think I forgot to mention this point in my >> earlier mail. >> >> [1] - >> >>

Re: [HACKERS] Page Scan Mode in Hash Index

Hi Ashutosh, On 03/29/2017 09:16 PM, Ashutosh Sharma wrote: This patch needs a rebase. Please try applying these patches on top of [1]. I think you should be able to apply it cleanly. Sorry, I think I forgot to mention this point in my earlier mail. [1] -

Re: [HACKERS] Page Scan Mode in Hash Index

I think you should consider refactoring this so that it doesn't need >> to use goto. Maybe move the while (offnum <= maxoff) logic into a >> helper function and have it return itemIndex. If itemIndex == 0, you >> can call it again. >> > > okay, Added a helper function for _hash_readpage().

Re: [HACKERS] Page Scan Mode in Hash Index

Hi, On 03/27/2017 09:34 AM, Ashutosh Sharma wrote: Hi, I think you should consider refactoring this so that it doesn't need to use goto. Maybe move the while (offnum <= maxoff) logic into a helper function and have it return itemIndex. If itemIndex == 0, you can call it again. okay, Added

Re: [HACKERS] Page Scan Mode in Hash Index

Hi, > I think you should consider refactoring this so that it doesn't need > to use goto. Maybe move the while (offnum <= maxoff) logic into a > helper function and have it return itemIndex. If itemIndex == 0, you > can call it again. okay, Added a helper function for _hash_readpage(). Please

Re: [HACKERS] Page Scan Mode in Hash Index

On Thu, Mar 23, 2017 at 2:35 PM, Ashutosh Sharma wrote: > I take your suggestion. Please find the attachments. I think you should consider refactoring this so that it doesn't need to use goto. Maybe move the while (offnum <= maxoff) logic into a helper function and have

Re: [HACKERS] Page Scan Mode in Hash Index

> Hi, > > On 03/23/2017 02:11 PM, Ashutosh Sharma wrote: >> >> On Thu, Mar 23, 2017 at 8:29 PM, Jesper Pedersen >> wrote: >>> >>> 0001v2: >>> >>> In hashgettuple() you can remove the 'currItem' and 'offnum' from the >>> 'else' >>> part, and do the assignment inside >>>

Re: [HACKERS] Page Scan Mode in Hash Index

Hi, On 03/23/2017 02:11 PM, Ashutosh Sharma wrote: On Thu, Mar 23, 2017 at 8:29 PM, Jesper Pedersen wrote: 0001v2: In hashgettuple() you can remove the 'currItem' and 'offnum' from the 'else' part, and do the assignment inside if (so->numKilled <

Re: [HACKERS] Page Scan Mode in Hash Index

On Thu, Mar 23, 2017 at 8:29 PM, Jesper Pedersen wrote: > Hi, > > On 03/22/2017 09:32 AM, Ashutosh Sharma wrote: >> >> Done. Please refer to the attached v2 version of patch. >> > > Thanks. > 1) 0001-Rewrite-hash-index-scans-to-work-a-page-at-a-time.patch: this

Re: [HACKERS] Page Scan Mode in Hash Index

Hi, On 03/22/2017 09:32 AM, Ashutosh Sharma wrote: Done. Please refer to the attached v2 version of patch. Thanks. 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

Re: [HACKERS] Page Scan Mode in Hash Index

Hi, >> Attached patch modifies hash index scan code for page-at-a-time mode. >> For better readability, I have splitted it into 3 parts, >> > > Due to the commits on master these patches applies with hunks. > > The README should be updated to mention the use of page scan. Done. Please refer to

Re: [HACKERS] Page Scan Mode in Hash Index

Hi, On 02/14/2017 12:27 AM, Ashutosh Sharma wrote: Currently, Hash Index scan works tuple-at-a-time, i.e. for every qualifying tuple in a page, it acquires and releases the lock which eventually increases the lock/unlock traffic. For example, if an index page contains 100 qualified tuples, the

Re: [HACKERS] Page Scan Mode in Hash Index

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

Re: [HACKERS] Page Scan Mode in Hash Index

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

Re: [HACKERS] Page Scan Mode in Hash Index

Hi, Ashutosh! 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. On Tue, Feb 14, 2017 at 8:27 AM, Ashutosh Sharma wrote: > 1)