Re: Hash Indexes

On Tue, Sep 27, 2016 at 3:06 PM, Jesper Pedersen
<jesper.pedersen@redhat.com> wrote:
> I have been running various tests, and applications with this patch together
> with the WAL v5 patch [1].
>
> As I havn't seen any failures and doesn't currently have additional feedback
> I'm moving this patch to "Ready for Committer" for their feedback.

Cool!  Thanks for reviewing.

Amit, can you please split the buffer manager changes in this patch
into a separate patch?  I think those changes can be committed first
and then we can try to deal with the rest of it.  Instead of adding
ConditionalLockBufferShared, I think we should add an "int mode"
argument to the existing ConditionalLockBuffer() function.  That way
is more consistent with LockBuffer().  It means an API break for any
third-party code that's calling this function, but that doesn't seem
like a big problem.  There are only 10 callers of
ConditionalLockBuffer() in our source tree and only one of those is in
contrib, so probably there isn't much third-party code that will be
affected by this, and I think it's worth it for the long-term
cleanliness.

As for CheckBufferForCleanup, I think that looks OK, but: (1) please
add an Assert() that we hold an exclusive lock on the buffer, using
LWLockHeldByMeInMode; and (2) I think we should rename it to something
like IsBufferCleanupOK.  Then, when it's used, it reads like English:
if (IsBufferCleanupOK(buf)) { /* clean up the buffer */ }.

I'll write another email with my thoughts about the rest of the patch.
For the record, Amit and I have had extensive discussions about this
effort off-list, and as Amit noted in his original post, the design is
based on suggestions which I previously posted to the list suggesting
how the issues with hash indexes might get fixed.  Therefore, I don't
expect to have too many basic disagreements regarding the design of
the patch; if anyone else does, please speak up.  Andres already
stated that he things working on btree-over-hash would be more
beneficial than fixing hash, but at this point it seems like he's the
only one who takes that position.  Even if we accept that working on
the hash AM is a reasonable thing to do, it doesn't follow that the
design Amit has adopted here is ideal.  I think it's reasonably good,
but that's only to be expected considering that I drafted the original
version of it and have been involved in subsequent discussions;
someone else might dislike something that I thought was OK, and any
such opinions certainly deserve a fair hearing.  To be clear, It's
been a long time since I've looked at any of the actual code in this
patch and I have at no point studied it deeply, so I expect that I may
find a fair number of things that I'm not happy with in detail, and
I'll write those up along with any design-level concerns that I do
have.  This should in no way forestall review from anyone else who
wants to get involved.

Thanks,

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