Re: [HACKERS] GSoC 2017: weekly progress reports (week 4) and patch for hash index

On Fri, Mar 2, 2018 at 9:27 AM, Thomas Munro
<thomas(dot)munro(at)enterprisedb(dot)com> wrote:
> On Fri, Mar 2, 2018 at 3:57 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
>> Based on this sub-thread this patch's status of 'needs review' doesn't
>> quite seem accurate and 'waiting on author' and then 'returned with
>> feedback' would be more fitting?
>
> I personally think this patch is really close to RFC. Shubham has
> fulfilled the project requirement, it's a tidy and short patch, it has
> tests. I think we really just need to verify that the split case
> works correctly.
>
> Hmm. I notice that this calls PredicateLockPageSplit() after both
> calls to _hash_splitbucket() (the one in _hash_finish_split() and the
> one in _hash_expandtable()) instead of doing it inside that function,
> and that _hash_splitbucket() unlocks bucket_nbuf before returning.
> What if someone else accesses bucket_nbuf between
> LockBuffer(bucket_nbuf, BUFFER_LOCK_UNLOCK) and
> PredicateLockPageSplit()? Doesn't that mean that another session can
> read a newly created page and miss a predicate lock that is about to
> be transferred to it?
>

Yes. I think you are primarily worried about if there is an insert on
new bucket from another session as scans will anyway take the
predicate lock, right?

> If that is indeed a race, could it be fixed by
> calling PredicateLockPageSplit() at the start of _hash_splitbucket()
> instead?
>

Yes, but I think it would be better if we call this once we are sure
that at least one tuple from the old bucket has been transferred
(consider if all tuples in the old bucket are dead). Apart from this,
I think this patch has missed handling the cases where we scan the
buckets when the split is in progress. In such cases, we scan both
old and new bucket, so I think we need to ensure that we take
PredicateLock on both the buckets during such scans.

> Could we get a few days to mull over this and Shubham's other patches?
>

I would also like to see this patch going in v11. So, I can try to
finish the remaining review comments, if Shubham is not able to spare
time and you can help with the review. I am also okay to review if
anyone else other than me can handle the remaining points.

> It'd be really great to get some of these into 11.
>

+1.

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com