On Tue, Sep 20, 2016 at 8:55 AM, Heikki Linnakangas <hlinn...@iki.fi> wrote: > Thanks for working on this, and sorry for disappearing and dropping this on > the floor earlier. This doesn't apply anymore, thanks to 9c810a2e. Shouldn't > be hard to fix.
Thanks for looking at it again. > I was in support of this earlier in general, even though I had some > questions on the details. But now that I look at the patch again, I'm not so > sure anymore. Honestly, I almost withdrew this patch myself, simply because it has dragged on so long. It has become ridiculous. > I don't think this actually makes things clearer. It adds new > cases that the code needs to deal with. Index AM writers now have to care > about the difference between a UNIQUE_CHECK_PARTIAL and > UNIQUE_CHECK_SPECULATIVE. You can have the exact same implementation for > both, but at the very least the index AM author needs to read the paragraph > you added to the docs to understand the difference. That adds some cognitive > load. I think it gives us the opportunity to discuss the differences, and in particular to explain why this "speculative insertion" thing exists at all. Besides, we can imagine an amcanunique implementation in which the difference might matter. Honestly, since it is highly unlikely that there ever will be another amcanunique access method, the cognitive burden to implementers of new amcanunique AMs is not a concern to me. Rather, the concern with that part of the patch is educating people about how the whole speculative insertion thing works in general, and drawing specific attention to it in a few key places in the code. Speculative insertion is subtle and complicated enough that I feel that that should be well described, as I've said many times. Remember how hard it was for us to come to agreement on the basic requirements in the first place! > Likewise, in ExecInsertIndexTuples(), this makes the deferred-index > case work slightly differently from speculative insertion. It's not a big > difference, but it again forces you to keep one more scenario in mind, when > reading the code This actually does have useful practical consequences, although I didn't point that out earlier (though I should have). To see what I mean, consider the complaint in this recent thread, which is based on an actual user application problem: https://www.postgresql.org/message-id/flat/1472134358.649524...@f146.i.mail.ru#1472134358.649524...@f146.i.mail.ru I go on to explain how this patch represents a partial solution to that . That's what I mean by "useful practical consequences". As I say in , I think we could even get a full solution, if we applied this patch and *also* made the ordering in which the relcache returns a list of index OIDs more useful (it should still be based on something stable, to avoid deadlocks, but more than just OID order. Instead, relcache.c can sort indexes such that we insert into primary keys first, then unique indexes, then all other indexes. This also avoids bloat if there is a unique violation, by getting unique indexes out of the way first during ExecInsert()). > So overall, I think we should just drop this. Maybe a comment somewhere > would be in order, to point out that ExecInsertIndexTuples() and > index_insert might perform some unnecessary work, by inserting index tuples > for a doomed heap tuple, if a speculative insertion fails. But that's all. Perhaps. I'm curious about what your thoughts are on what I've said about "useful practical consequences" of finishing insertion earlier for speculative inserters. If you're still not convinced about this patch, having considered that as well, then I will drop the patch (or maybe we just add some comments, as you suggest).  https://www.postgresql.org/message-id/CAM3SWZTFTbK_Y%3D7uWJaXYLHnYQ99pV4KFpmjTKbNJR5_%2BQThzA%40mail.gmail.com -- Peter Geoghegan -- Sent via pgsql-hackers mailing list (email@example.com) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers