On Tue, Sep 27, 2016 at 3:06 PM, Jesper Pedersen <jesper.peder...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers