vinyl: fix abort of writers on DDL#10916
Merged
locker merged 1 commit intotarantool:masterfrom Dec 12, 2024
Merged
Conversation
When a DDL statement is committed, all transactions writing to the space must be aborted so that they don't access the stale `txn_stmt::space` pointer on commit. In Vinyl we abort all writers from the special `space_invalidate` engine callback, where we iterate over all writing transactions and abort those of them that have a write set entry pointing to the primary key of the invalidated space, see commit d3e1236 ("vinyl: abort affected transactions when space is removed from cache"). We also abort transactions that haven't yet created a write set entry because they need to read the disk first, for example, to perform a tuple update, see commit 4f62ec2 ("vinyl: make tx_manager_abort_writers_for_ddl more thorough"). However, as spotted by @drewdzzz, this code doesn't always work as expected. The problem is, unless dropped, the primary index is moved to the new space container so using the primary index stored in the old space container to abort transactions is wrong: there can't possibly be any transactions associated with it. We don't get a crash because we have an extra check in `vy_lsm_set` that aborts the transaction if the space format was updated (it is updated on every space change), see commit e3c5fde ("vinyl: fix DDL with active transactions"). Another problem with `space_invalidate` is that it doesn't abort transactions that turned out to be no-op, such as an update of a non-existing key because such transactions don't add any entries to the write set. Still, they must be aborted because they have a `txn_stmt` pointing to the old space. Letting them proceed to commit would result in a use-after-free bug, see tarantool#10707. Let's fix `space_invalidate` by making it iterate over all statements of writing transactions instead of checking their Vinyl write sets. To achieve that, we store a pointer to `struct txn` in `struct vy_tx`. This lets us drop `vy_tx::last_stmt_space` and `vy_tx::isolation` because they can be accessed via `txn`. We also replace the format check in `vy_lsm_set` mentioned above with an assertion because such transactions should be aborted by `space_invalidate` now. Closes tarantool#10707 NO_DOC=bug fix
drewdzzz
reviewed
Dec 11, 2024
drewdzzz
approved these changes
Dec 11, 2024
Contributor
drewdzzz
left a comment
There was a problem hiding this comment.
LGTM, it's nice that we don't need to handle writes that haven't written something yet anymore!
xuniq
approved these changes
Dec 12, 2024
Member
Author
|
Cherry-picked to 3.2 and 3.3. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When a DDL statement is committed, all transactions writing to the space must be aborted so that they don't access the stale
txn_stmt::spacepointer on commit. In Vinyl we abort all writers from the specialspace_invalidateengine callback, where we iterate over all writing transactions and abort those of them that have a write set entry pointing to the primary key of the invalidated space, see commit d3e1236 ("vinyl: abort affected transactions when space is removed from cache"). We also abort transactions that haven't yet created a write set entry because they need to read the disk first, for example, to perform a tuple update, see commit 4f62ec2 ("vinyl: make tx_manager_abort_writers_for_ddl more thorough").However, as spotted by @drewdzzz, this code doesn't always work as expected. The problem is, unless dropped, the primary index is moved to the new space container so using the primary index stored in the old space container to abort transactions is wrong: there can't possibly be any transactions associated with it. We don't get a crash because we have an extra check in
vy_lsm_setthat aborts the transaction if the space format was updated (it is updated on every space change), see commit e3c5fde ("vinyl: fix DDL with active transactions").Another problem with
space_invalidateis that it doesn't abort transactions that turned out to be no-op, such as an update of a non-existing key because such transactions don't add any entries to the write set. Still, they must be aborted because they have atxn_stmtpointing to the old space. Letting them proceed to commit would result in a use-after-free bug, see #10707.Let's fix
space_invalidateby making it iterate over all statements of writing transactions instead of checking their Vinyl write sets. To achieve that, we store a pointer tostruct txninstruct vy_tx. This lets us dropvy_tx::last_stmt_spaceandvy_tx::isolationbecause they can be accessed viatxn. We also replace the format check invy_lsm_setmentioned above with an assertion because such transactions should be aborted byspace_invalidatenow.Closes #10707