Value returned from Accumulator#update() is non-transactional
Affects | Status | Importance | Assigned to | Milestone | |
---|---|---|---|---|---|
Akiban Persistit |
Fix Released
|
High
|
Peter Beaman |
Bug Description
Persistit r428
Note that the return value is not documented.
Test case below fails with:
java.lang.
@Test
public void updateRetVal() throws PersistitException {
Transaction txn = _persistit.
Exchange ex = _persistit.
Accumulator accum = ex.getTree(
txn.begin();
assertEqual
assertEqual
txn.commit();
txn.end();
txn.begin();
long newValA = accum.update(1, txn);
assertEqual
assertEqual
txn.rollback();
txn.end();
txn.begin();
assertEqual
long newValB = accum.update(1, txn);
assertEqual
assertEqual
txn.commit();
txn.end();
txn.begin();
assertEqual
txn.commit();
txn.end();
}
Looking at the code, it appears it is actually the contents of the live value. If that is "as intended", we at least need to document it. I would vote for remove the behavior, though, so that any live value get is explicitly an opt-in.
Related branches
- Nathan Williams: Needs Fixing
-
Diff: 1193 lines (+380/-182)14 files modifiedpom.xml (+1/-1)
src/main/java/com/persistit/Accumulator.java (+158/-68)
src/main/java/com/persistit/SessionId.java (+13/-2)
src/main/java/com/persistit/SharedResource.java (+5/-1)
src/main/java/com/persistit/Tree.java (+94/-2)
src/test/java/com/persistit/AccumulatorRecoveryTest.java (+11/-18)
src/test/java/com/persistit/AccumulatorTest.java (+44/-38)
src/test/java/com/persistit/Bug1064565Test.java (+9/-8)
src/test/java/com/persistit/Bug911849Test.java (+3/-7)
src/test/java/com/persistit/Bug920754Test.java (+5/-3)
src/test/java/com/persistit/Bug974589Test.java (+5/-4)
src/test/java/com/persistit/JournalManagerTest.java (+6/-7)
src/test/java/com/persistit/stress/unit/AccumulatorRestart.java (+15/-12)
src/test/java/com/persistit/stress/unit/Stress8txn.java (+11/-11)
- Mike McMahon: Needs Information
- Akiban Build User: Needs Fixing
- Thomas Jones-Low: Approve
-
Diff: 1154 lines (+472/-163)28 files modifiedpom.xml (+1/-1)
src/main/java/com/akiban/ais/model/AISMerge.java (+12/-8)
src/main/java/com/akiban/ais/model/Column.java (+12/-7)
src/main/java/com/akiban/ais/model/Index.java (+3/-4)
src/main/java/com/akiban/ais/model/Sequence.java (+45/-39)
src/main/java/com/akiban/ais/model/aisb2/AISBBasedBuilder.java (+1/-1)
src/main/java/com/akiban/ais/util/TableChangeValidator.java (+1/-0)
src/main/java/com/akiban/qp/persistitadapter/OperatorStoreGIHandler.java (+2/-2)
src/main/java/com/akiban/qp/persistitadapter/PersistitAdapter.java (+2/-2)
src/main/java/com/akiban/qp/persistitadapter/indexrow/PersistitIndexRowBuffer.java (+1/-1)
src/main/java/com/akiban/server/AccumulatorAdapter.java (+33/-30)
src/main/java/com/akiban/server/MemoryOnlyTableStatusCache.java (+0/-7)
src/main/java/com/akiban/server/PersistitAccumulatorTableStatusCache.java (+7/-22)
src/main/java/com/akiban/server/TableStatus.java (+0/-2)
src/main/java/com/akiban/server/service/dxl/BasicDDLFunctions.java (+1/-2)
src/main/java/com/akiban/server/store/PersistitStore.java (+8/-2)
src/main/java/com/akiban/server/store/PersistitStoreSchemaManager.java (+27/-22)
src/main/java/com/akiban/server/store/SequenceFixUpRoutines.java (+139/-0)
src/main/java/com/akiban/server/store/statistics/IndexStatisticsServiceImpl.java (+2/-2)
src/main/java/com/akiban/sql/aisddl/TableDDL.java (+3/-4)
src/test/java/com/akiban/server/rowdata/SchemaFactory.java (+2/-1)
src/test/java/com/akiban/server/test/it/dxl/AlterTableBasicIT.java (+19/-0)
src/test/java/com/akiban/server/test/it/keyupdate/FixCountStarIT.java (+1/-1)
src/test/java/com/akiban/sql/aisddl/SequenceDDLIT.java (+53/-1)
src/test/java/com/akiban/sql/aisddl/TableDDLIT.java (+1/-1)
src/test/java/com/akiban/sql/pg/PostgresServerMiscYamlIT.java (+8/-1)
src/test/resources/com/akiban/sql/pg/yaml/functional/test-seq-fixup-routines.yaml (+40/-0)
src/test/resources/com/akiban/sql/pg/yaml/functional/test-sequence.yaml (+48/-0)
information type: | Proprietary → Public |
Changed in akiban-persistit: | |
status: | New → Fix Committed |
importance: | Undecided → High |
assignee: | nobody → Peter Beaman (pbeaman) |
milestone: | none → 3.2.8 |
Changed in akiban-server: | |
status: | New → Fix Committed |
importance: | Undecided → High |
assignee: | nobody → Nathan Williams (nwilliams) |
milestone: | none → 1.6.1 |
Changed in akiban-server: | |
status: | Fix Committed → Fix Released |
Yes, it's interesting that the Javadoc in Accumulator is actually malformed. I don't know why the javadoc task doesn't kick out a warning.
So everything Nathan wrote is correct. This is an attempt to add to it.
The fundamental issue is that every generated auto-increment value needs to be unique for all time (subject to the maxValue wrapping behavior of sequences).
There are two ways to do this:
(a) Serialize all update and insert transactions that increment an auto-increment value. That way, any transaction that needs a new value is able to see all prior committed updates, and the newly incremented value it commits will be unique because no other transaction is running concurrently.
(b) Allocate a provisional ID value (for example, by incrementing an AtomicLong) and then record the transaction in such a way that this value is permanently retired and will never be generated again. As a corollary to this behavior, a transaction that allocates and ID and then rolls back will leave a "hole" in the sequence numbers. We believe this is a reasonable outcome preferable to running all auto-incrementing transactions serially.
Currently the Sequence code uses a Persistit SUM accumulator to maintain the count. The semantics of SUM are to provide a transactionally accurate count. So if three transactions concurrently increment a SUM accumulator, and then two commit and one aborts, the result will be 2.
What confuses the issue is that Accumulators offer two values: the "snapshot" value, which is consistent with the state of the database at the start of the transaction, and the "live" value, which is modified as updates are executed. The live value is not transactionally correct; it is an "estimate" of the value accurate only if ever transaction has committed. (I note that there's no attempt to adjust the live value even when an aborted transaction is pruned. We might want to change that.) The live value of a SUM accumulator is not maintained transactionally - in fact it explicitly diverges from any consistent snapshot view.
What further confuses the issue, as Nathan notes, is that the Accumulator#update operation always returns the live value, but is not documented. What the code today is actually doing is using the live value of a SUM accumulator, which appears to work just fine for awhile. To see a failure there must be one or more update transactions that increment the accumulator and then abort, leaving the live value larger than the snapshot value by N where N is the number of transactions that aborted. Everything continues to work correctly until the server is restarted; when that happens the live value is reset to the snapshot value that existed at the time of the shutdown, and therefore after restarting, the next auto-increment value obtained is smaller than one produced prior to shutdown - hence the duplicate key exceptions.
The only way to use a SUM accumulator to generate unique ID values is to use its snapshot value. But unfortunately, this will work only if all update transactions that perform auto-increment are constrained to execute serially. That is, to take out a big lock (or force a w-w-conflict) to prevent concurrent...