Re: Refactoring speculative insertion with unique indexes a little

On 03/17/2016 01:43 AM, Peter Geoghegan wrote:
> I attach a revision, that makes all the changes that Heikki suggested,
> except one. As already noted several times, following this suggestion
> would have added a bug. Alvaro preferred my original approach here in
> any case. I refer to my original approach of making the new
> UNIQUE_CHECK_SPECULATIVE case minimally different from the existing
> UNIQUE_CHECK_PARTIAL case currently used for deferred unique
> constraints and speculative insertion, as opposed to making the new
> UNIQUE_CHECK_SPECULATIVE "like CHECK_UNIQUE_YES, but return FALSE
> instead of throwing an error on conflict". That was broken because
> CHECK_UNIQUE_YES waits for the outcome of an xact, which
> UNIQUE_CHECK_PARTIAL never does, and so UNIQUE_CHECK_SPECULATIVE must
> never do.
>
> Any and all waits happen in the first phase of speculative insertion,
> and never the seconds. I could give a complicated explanation for why,
> involving a deadlock scenario, but a simple explanation will do: it
> has always worked that way, and was tested to work that way.
>
> Feedback from Heikki led to these changes for this revision:
>
> * The use of arguments within ExecInsert() was simplified.
>
> * More concise AM documentation.
>
> The docs essentially describe two new concepts:
>
> - What unique index insertion needs to know about speculative
> insertion in general. This doesn't just apply to speculative inserters
> themselves, of course.
>
> - What speculative insertion is. Why it exists (why we don't just wait
> on xact). In other words, what "unprincipled deadlocks" are, and how
> they are avoided (from a relatively high level).
>
> I feel like I have a responsibility to make sure that this mechanism
> is well documented, especially given that not that many people were
> involved in its design. It's possible that no more than the 3 original
> authors of UPSERT fully understand speculative insertion -- it's easy
> to miss some of the subtleties.

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.

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. 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. 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.

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.

- Heikki