Re: Page Scan Mode in Hash Index

On Thu, Mar 23, 2017 at 2:35 PM, Ashutosh Sharma <ashu(dot)coek88(at)gmail(dot)com> 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 itemIndex == 0, you
can call it again. Notice that the code in the "else" branch of the
if (itemIndex == 0) stuff could actually be removed from the else
block without changing anything, because the "if" block always either
returns or does a goto. That's worthy of a little more work to try to
make things simple and clear.

+ * We find the first item(or, if backward scan, the last item) in

Missing space.

In _hash_dropscanbuf(), the handling of hashso_bucket_buf is now
inconsistent with the handling of hashso_split_bucket_buf, which seems
suspicious. Suppose we enter this function with so->hashso_bucket_buf
and so->currPos.buf both being valid buffers, but not the same one.
It looks to me as if we'll release the first pin but not the second
one. 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 == so->currPos.buf on the last
_hash_dropbuf() should be removed. If that's not right, then I think
I need somebody to explain why not.

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.

This is not a full review, but I'm out of time for the moment.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company