Re: Hash Indexes

On Wed, Dec 14, 2016 at 4:27 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>> It's not required for enabling WAL. You don't *have* to release and
>> reacquire the buffer lock; in fact, that just adds overhead.
>
> If we don't release the lock, then it will break the general coding
> pattern of writing WAL (Acquire pin and an exclusive lock,
> Markbufferdirty, Write WAL, Release Lock). Basically, I think it is
> to ensure that we don't hold it for multiple atomic operations or in
> this case avoid calling MarkBufferDirty multiple times.

I think you're being too pedantic. Yes, the general rule is to
release the lock after each WAL record, but no harm comes if we take
the lock, emit TWO WAL records, and then release it.

> It is possible that we can MarkBufferDirty multiple times (twice in
> hashbucketcleanup and then in _hash_squeezebucket) while holding the
> lock. I don't think that is a big problem as of now but wanted to
> avoid it as I thought we need it for WAL patch.

I see no harm in calling MarkBufferDirty multiple times, either now or
after the WAL patch. Of course we don't want to end up with tons of
extra calls - it's not totally free - but it's pretty cheap.

>> Aside from hopefully fixing the bug we're talking about
>> here, this makes the logic in several places noticeably less
>> contorted.
>
> Yeah, it will fix the problem in hashbucketcleanup, but there are two
> other problems that need to be fixed for which I can send a separate
> patch. By the way, as mentioned to you earlier that WAL patch already
> takes care of removing _hash_wrtbuf and simplified the logic wherever
> possible without introducing the logic of MarkBufferDirty multiple
> times under one lock. However, if you want to proceed with the
> current patch, then I can send you separate patches for the remaining
> problems as addressed in bug fix patch.

That sounds good.

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