Re: pgsql 10: hash indexes testing

On Sat, Aug 5, 2017 at 7:50 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Fri, Aug 4, 2017 at 2:45 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>> I have not done anything for this comment as it doesn't sound wrong to
>> me. I think it is not making much sense in the current code and we
>> can remove it or change it as part of the separate patch if you or
>> others think so.
>
> I don't get it. The comment claims we have to _hash_getnewbuf before
> releasing the metapage write lock. But the function neither calls
> _hash_getnewbuf nor releases the metapage write lock.

Both the actions _hash_getnewbuf and release of the metapage lock are
done in the caller (_hash_expandtable).

> It then goes on
> to say that for this reason we pass the new buffer rather than just
> the block number. However, even if it were true that we have to call
> _hash_getnewbuf before releasing the metapage write lock, what does
> that have to do with the choice of passing a buffer vs. a block
> number?
>

For this, you have to look at PG9.6 code. In PG9.6, we were passing
old bucket's block number and new bucket's buffer and the reason is
that in the caller (_hash_expandtable) we only have access to a buffer
of new bucket block.

> Explain to me what I'm missing, please, because I'm completely befuddled!
>

I think you need to compare the code of PG10 with PG9.6 for functions
_hash_splitbucket and _hash_expandtable. I don't find this comment
much useful starting PG10. Patch attached to remove it.

> (On another note, I committed these patches.)
>

Thanks.

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