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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
