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
instead of throwing an error on conflict". That was broken because
CHECK_UNIQUE_YES waits for the outcome of an xact, which
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

Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:

Reply via email to