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 items.). That part of the
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
>> hashbucketcleanup(). Please find the attached 0003*.pa
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
>> hashbucketcleanup(). Please find the attached 0003*.pa
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 committed 0001 and 0002
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
> significantly more work. In several places, they fail
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
> significantly more work. In several places, they fail
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 differences, and the header com
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 (!BlockNumberIsValid(opaque->hasho_nextblkno))
> +{
> +if (so->
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 ||
+so->c
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 heaptid
>> can't be reused via Page-at-a-time 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
> entry as dead.
It can mar
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.
>>
>> Page-at-a-time index vacuum also allows TIDs to
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 only for already marked dead
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 temporary
>> tables as we are doing without this patch.
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
> won't be able to remove the TI
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 patch to "Ready for
>> Committer".
>
> Reviewing 0001:
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 gets the page LSN to see if we can apply LP_DEAD hin
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 temporary, the LSN will never
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)
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 the changes in vacuum patch
>>
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, currently in _hash_kill_items(),
>> if an overflow page is pinned we do not
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
don'
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 modified since
> the last read or
> not. Hence, if the
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 Sharma
>>> wrote:
>
> 7.
> _hash_kill_items(IndexScanDesc scan)
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 scan)
{
..
+ /*
+ * If page LSN differs it means that
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 modified since the
>>> + * last read. killedItems coul
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-
>> + * ing is not safe.
>> +
>
> 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 hav
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
> + * last read. killedItems could be not valid so LP_D
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, Ashutosh Sharma
>>> wrote:
While doing the code coverage testing of v
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 testing of v7 patch shared with - [1], I
>>> found that there are few lines o
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 doesn't:
We often include these
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 _hash_next() that are
>> redundant and needs to be re
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
> testing the scenario wh
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 remov
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 = BufferGetPage(so->hashso_curbuf);
> + blkno = so->currPos.cu
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 where it
>>> was before, just after the first if stat
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
>> condition so->hashso_bucket_buf
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 == so->currPos.buf)
+ {
+ buf = so->
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
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 similar check for hash_kill_
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] -
>>
>> https://w
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] -
https://www.postgresql.org/message-id
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
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
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 c
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 it return itemIndex. If
> 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 < Ma
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 < MaxIndexTuplesPerPage)
instead.
Done. P
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
patch rewrites the hash in
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
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 th
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 c
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 depe
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
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) 0001-Rewrite-hash-index-scans-to-work-a-page-at
56 matches
Mail list logo