make iterator usage safer after intermediate commits #14346
Conversation
…e-iterators-after-intermediate-commits
…e-iterators-after-intermediate-commits
…e-iterators-after-intermediate-commits
…er-intermediate-commits
…rs-after-intermediate-commits
…s see when reading from the storage engine.
| _readDocumentContext._outputRow = &output; | ||
| written = collection->getPhysical()->read( | ||
| &_trx, LocalDocumentId(input.getValue(docRegId).slice().getUInt()), callback).ok(); | ||
| &_trx, LocalDocumentId(input.getValue(docRegId).slice().getUInt()), callback, ReadOwnWrites::no).ok(); |
There was a problem hiding this comment.
Why is this hardcoded to ReadOwnWrites::no. At the very least an explanation for this is needed.
| } | ||
| return true; | ||
| }).ok(); | ||
| }, ReadOwnWrites::no).ok(); |
There was a problem hiding this comment.
I am not sure why this is hard-wired to no here. Please add a comment.
| } else { | ||
| TRI_ASSERT(_ownsReadWriteBatch == false); | ||
| TRI_ASSERT(_hasActiveModificationQuery.load() == false); | ||
| if (_hasActiveModificationQuery.load(std::memory_order_relaxed)) { |
There was a problem hiding this comment.
Why this code if the assertion is correct? Is it only by coincidence that we do not have a test which tries this? In this case we should add such a test.
There was a problem hiding this comment.
I would guess that release builds should return an error instead?
There was a problem hiding this comment.
We currently have no tests that perform concurrent operations on the same transaction. I agree that we need such a test, but I would prefer to add it in a separate PR.
There was a problem hiding this comment.
LGTM. I think in a few places it might be sensible to add a few more comments, if only so that we remember ourselves in half a year, why a certain ReadOwnWrites::no or yes is there. I think the method works and improves our situation. Tests are sufficient.
One area where it would be worthwhile to add a few tests is some AQL queries which modify the collection they run over, to make sure that the new behaviour of not seeing the own writes works. An example would be:
FOR doc IN coll
INSERT {value : doc.value + 1} INTO coll
and then with some index scans, or a graph traversal or the like. But we can also add this later, if time is scarce now (as it always is).
In any case: Well done! This was good and tedious cleanup work and you have done a good job of explaining it to us.
| s = mthds->GetForUpdate(_cf, key.string(), &existing); | ||
| } else { | ||
| s = mthds->Get(_cf, key.string(), &existing); | ||
| s = mthds->Get(_cf, key.string(), &existing, ReadOwnWrites::yes); |
There was a problem hiding this comment.
Why yes here? Same for the two other places in this file. Maybe add a comment.
| _readDocumentContext._outputRow = &output; | ||
| written = collection->getPhysical()->read( | ||
| &_trx, LocalDocumentId(input.getValue(docRegId).slice().getUInt()), callback).ok(); | ||
| &_trx, LocalDocumentId(input.getValue(docRegId).slice().getUInt()), callback, ReadOwnWrites::no).ok(); |
| OperationOptions options = | ||
| ModificationExecutorHelpers::convertOptions(_options, _outVariableNew, _outVariableOld); | ||
| // We must not disable indexing for UPSERTs because the subquery might rely on a non-unique secondary index | ||
| options.canDisableIndexing = false; |
There was a problem hiding this comment.
perhaps this could be encapsulated in a setter, or even be initialized in the constructor?
| } | ||
| return true; | ||
| }).ok(); | ||
| }, ReadOwnWrites::no).ok(); |
| // Even though the assertion is only evaluated in maintainer mode, it must at | ||
| // least compile. But since checkIntermediateCommits is only defined in maintainer | ||
| // mode, we have to wrap this assert in another ifdef. | ||
| TRI_ASSERT(!opts.checkIntermediateCommits || !hasIntermediateCommitsEnabled()); |
There was a problem hiding this comment.
No, we either want intermediate commits to be disabled, or we want the check to be disabled.
…rs-after-intermediate-commits
…rs-after-intermediate-commits
|
@maierlars @neunhoef I have added several comments, perhaps you could take another look if anything still needs clarification. |
Scope & Purpose
When a transaction triggers an intermediate commit, this may affect rocksdb::Iterator objects used by the transaction. These may become silently invalidated if they point to nodes in the committed transaction's
WriteBatchWithIndex's index (i.e. the SkipList).The PR also removes superfluous out-of-bounds checks for read-only transactions, which could even see a small speedup due to the changes.
We should add a lot more tests for these changes and the behavior. In our normal tests, intermediate commits are not happening frequently, and often they have no observable effect on the iterators.
Backports:
Testing & Verification