Re: Page Scan Mode in Hash Index

On Wed, Sep 20, 2017 at 8:05 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Wed, Sep 20, 2017 at 5:37 AM, Ashutosh Sharma <ashu.coek88@gmail.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