From: | Ashutosh Sharma <ashu(dot)coek88(at)gmail(dot)com> |
---|---|
To: | Robert Haas <robertmhaas(at)gmail(dot)com> |
Cc: | Jesper Pedersen <jesper(dot)pedersen(at)redhat(dot)com>, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Page Scan Mode in Hash Index |
Date: | 2017-09-20 15:43:42 |
Message-ID: | CAE9k0PktGvfidvzoTJNRD8WXY0hUAo3wktYWy4tFN0oY6xWM9Q@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Wed, Sep 20, 2017 at 8:05 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Wed, Sep 20, 2017 at 5:37 AM, Ashutosh Sharma <ashu(dot)coek88(at)gmail(dot)com> 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->currPos.buf == so->hashso_split_bucket_buf)
> + prev_blkno = InvalidBlockNumber;
> + else
> + prev_blkno = opaque->hasho_prevblkno;
> + }
>
> 1. Why not remove the outer "if" statement?
>
Yes, the outer if statement is not required. I just missed to remove
that in my earlier patch.
> 2. How about adding a comment, like /* If this is a primary bucket
> page, hasho_prevblkno is not a real block number. */
>
Added.
>> When _hash_readpage() doesn't find any qualifying tuples i.e. when
>> _hash_readnext() returns Invalid buffer, we just update prevPage,
>> nextPage and buf in
>> currPos (not currPage or lsn) as currPage and lsn should point to last
>> page in the hash bucket so that we can mark the killed items as dead
>> at the end of scan (with the help of _hash_kill_items). Hence, we keep
>> the currpage and lsn as it is if no more valid hash pages are found.
>
> How about adding a comment about this, by extending this comment:
>
> + * Remember next and previous block numbers for scrollable
> + * cursors to know the start position and return FALSE
> + * indicating that no more matching tuples were found.
>
> e.g. (Don't reset currPage or lsn, because we expect _hash_kill_items
> to be called for the old page after this function returns.)
>
>
Added.
Attached are the patches with above changes. Thanks.
--
With Regards,
Ashutosh Sharma
EnterpriseDB:http://www.enterprisedb.com
Attachment | Content-Type | Size |
---|---|---|
0001-Rewrite-hash-index-scan-to-work-page-at-a-time_v16.patch | text/x-patch | 32.1 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_v7.patch | text/x-patch | 7.8 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Amit Khandekar | 2017-09-20 15:57:10 | Re: UPDATE of partition key |
Previous Message | Peter Geoghegan | 2017-09-20 15:40:02 | Re: Varying results when using merge joins over postgres_fdw vs hash joins |