neo4j/neo4j — pull request #1881 by chrisvest (2014-01-29 10:41:20+00:00)
Title: Also fix false index collisions for online indexes

Note: The point of offering the changes in this state, is to see what our robustness tests thinks of them.

The story so far:

* Indexes store all numbers as doubles, and that means that large long values
can coerce to the same double values and cause collisions in the index.
We fixed this for indexes that are populating, by making them read colliding
values out from the property store to double check if the collision is real
or not.
* We also added a similar "exact match" filtering to the index lookup
operations on StateHandlingStatementOperations, to fix index lookups.
* What we didn't fix in the first round, was adding new stuff to indexes that
are online. This is the bug that Mattias found.
* Specifically, the UniquePropertyIndexUpdater verifies that the constraint is
still holding in its close method.
* This is super bad, because the close method is called in commit - not in
prepare, not during the transaction - in commit. Huge no-no.
* Initially you'd think that this puts recovery at risk, but that turns out not
to be the case, because during recovery, we use a normal index updater
instead of the unique one.
* So this means that the following scenario was possible: Add two nodes with
similar but distinct long values to the index. The
ConstraintEnforcingEntityOperations is fine with this because the values are
really different. However, the UniquePropertyIndexUpdater trashes the
transaction in commit. Now suppose the entire database server crashes for
whatever reason, and we must do recovery. We recover the index using the
normal updater that just adds the values without checking any constraints.
And this is actually totally fine, because we filter for exact matches in the
StateHandlingStatementOperations. And so only then does the index work as
advertised. We had to crash the database in order to add a valid value to the
index. Except, there is a window between the crash of the transaction, and
the crash of the database, where the transaction could have been retried with
the same exact value and encounter the same exact crash. Now we recover both
transactions, and boom: we got a duplicate in the unique index!
* The solution I'm going for currently is to just not verify the constraint in
the UniquePropertyIndexUpdater.close method... just accept the value as
given, and trust that the ConstraintEnforcingEntityOperations has done it's
job.
* Implementing this solution is made a bit harder by a number of tests
operating directly with the IndexUpdaters, instead of checking the publicly
observable behaviour.
* I intend to rephrase these tests in terms of higher level APIs, but in the
mean time (because there are a lot of cases to cover) we will have to ignore
all the now failing tests, and see what the robustness suite says.