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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers