Re: Page Scan Mode in Hash Index

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