Re: Refactoring speculative insertion with unique indexes a little

On Tue, Sep 20, 2016 at 8:55 AM, Heikki Linnakangas <hlinnaka(at)iki(dot)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(dot)649524557(at)f146(dot)i(dot)mail(dot)ru#1472134358(dot)649524557@f146.i.mail.ru

I go on to explain how this patch represents a partial solution to
that [1]. That's what I mean by "useful practical consequences". As I
say in [1], 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).

[1] https://www.postgresql.org/message-id/CAM3SWZTFTbK_Y%3D7uWJaXYLHnYQ99pV4KFpmjTKbNJR5_%2BQThzA%40mail.gmail.com
--
Peter Geoghegan