On Fri, Jun 24, 2016 at 2:38 PM, Mithun Cy <mithun.cy@enterprisedb.com> wrote:
> On Thu, Jun 16, 2016 at 12:58 PM, Amit Kapila <amit.kapila16@gmail.com>
> wrote:
>
> I have a question regarding code changes in _hash_first.
>
> + /*
> + * Conditionally get the lock on primary bucket page for search
> while
> + * holding lock on meta page. If we have to wait, then release the
> meta
> + * page lock and retry it in a hard way.
> + */
> + bucket = _hash_hashkey2bucket(hashkey,
> +
> metap->hashm_maxbucket,
> +
> metap->hashm_highmask,
> +
> metap->hashm_lowmask);
> +
> + blkno = BUCKET_TO_BLKNO(metap, bucket);
> +
> + /* Fetch the primary bucket page for the bucket */
> + buf = ReadBuffer(rel, blkno);
> + if (!ConditionalLockBufferShared(buf))
>
> Here we try to take lock on bucket page but I think if successful we do not
> recheck whether any split happened before taking lock. Is this not necessary
> now?
>
Yes, now that is not needed, because we are doing that by holding the
read lock on metapage. Split happens by having a write lock on
metapage. The basic idea of this optimization is that if we get the
lock immediately, then do so by holding metapage lock, else if we
decide to wait for getting a lock on bucket page then we still
fallback to previous kind of mechanism.
> Also below "if" is always true as we enter here only when outer "if
> (retry)" is true.
> + if (retry)
> + {
> + if (oldblkno == blkno)
> + break;
> + _hash_relbuf(rel, buf);
> + }
>
Good catch, I think we don't need this retry check now. We do need
similar change in _hash_doinsert().
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com