memtx: finalize prepare of all transactions in memtx MVCC#11007
Merged
locker merged 1 commit intotarantool:masterfrom Feb 4, 2025
drewdzzz:memtx_mvcc_sysview
Merged
memtx: finalize prepare of all transactions in memtx MVCC#11007locker merged 1 commit intotarantool:masterfrom drewdzzz:memtx_mvcc_sysview
locker merged 1 commit intotarantool:masterfrom
drewdzzz:memtx_mvcc_sysview
Conversation
CuriousGeorgiy
requested changes
Dec 30, 2024
changelogs/unreleased/gh-10715-memtx-mvcc-conflict-with-prepared-sysview-txn.md
Show resolved
Hide resolved
changelogs/unreleased/gh-10715-memtx-mvcc-conflict-with-prepared-sysview-txn.md
Show resolved
Hide resolved
test/box-luatest/gh_10715_memtx_mvcc_conflict_with_prepared_sysview_txn_test.lua
Show resolved
Hide resolved
test/box-luatest/gh_10715_memtx_mvcc_conflict_with_prepared_sysview_txn_test.lua
Show resolved
Hide resolved
test/box-luatest/gh_10715_memtx_mvcc_conflict_with_prepared_sysview_txn_test.lua
Outdated
Show resolved
Hide resolved
locker
reviewed
Jan 13, 2025
lenkis
approved these changes
Jan 13, 2025
changelogs/unreleased/gh-10715-memtx-mvcc-conflict-with-prepared-sysview-txn.md
Outdated
Show resolved
Hide resolved
CuriousGeorgiy
approved these changes
Jan 28, 2025
locker
approved these changes
Jan 29, 2025
We have a special `sysview` engine that bypasses transaction management and reads right from memtx indexes. The problem is memtx indexes use memtx MVCC by themselves, so despite `sysview` bypasses transaction management process, it actually uses memtx MVCC. So the problem here is that transaction can actually read something using `sysview`, a tracker in MVCC will be allocated, but `txn->engine[memtx->id]` still will be `NULL`, so the preparation of the transaction won't be finalized by memtx_tx and the tracker won't be deleted when transaction is prepared. On conflict, MVCC will try to abort that already prepared transaction and it will lead to a crash. Actually, crashing scenarios are rather synthetic and break only our engine fuzzer. Here is an example of such scenario without manual use of system spaces: 1. Open a transaction and do an operation in a vinyl space. 2. Try to alter the space in the same transaction - it will fail with cross-engine transaction error. 3. Commit the transaction, but do not wait for WAL write (internally, such state is called `prepared`). 4. Alter the space in another transaction and commit it before the previous transaction was written to WAL. Here MVCC will try to conflict that already prepared transaction and it will break Tarantool. To solve the problem, we could try to write quite a large patch to handle such transactions differently in memtx MVCC. But instead, let's simply finalize prepare of all transaction in MVCC. The current design already lacks encapsulation - all transactions (even non-memtx) are registered there, so such solution not only simple but also consistent with other MVCC parts. Note that memtx MVCC still can abort such non-memtx transactions when they are in-progress, but it seems that such behavior doesn't break other yielding engines (`vinyl` is the only one) - the commit covers it with a simple test along the way. In order not to do unnecessary function call when MVCC is disabled, function `memtx_tx_prepare_finalize` is `static inline` now and uses a `*_slow` helper when MVCC is actually enabled - such approach is common in this subsystem. Closes #10715 NO_DOC=bugfix
Member
|
Cherry-picked to 2.11, 3.2, 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.
We have a special
sysviewengine that bypasses transaction management and reads right from memtx indexes. The problem is memtx indexes use memtx MVCC by themselves, so despitesysviewbypasses transaction management process, it actually uses memtx MVCC. So the problem here is that transaction can actually read something usingsysview, a tracker in MVCC will be allocated, buttxn->engine[memtx->id]still will beNULL, so the preparation of the transaction won't be finalized by memtx_tx and the tracker won't be deleted when transaction is prepared. On conflict, MVCC will try to abort that already prepared transaction and it will lead to a crash.Actually, crashing scenarios are rather synthetic and break only our engine fuzzer. Here is an example of such scenario without manual use of system spaces:
prepared).To solve the problem, we could try to write quite a large patch to handle such transactions differently in memtx MVCC. But instead, let's simply finalize prepare of all transaction in MVCC. The current design already lacks encapsulation - all transactions (even non-memtx) are registered there, so such solution not only simple but also consistent with other MVCC parts. Note that memtx MVCC still can abort such non-memtx transactions when they are in-progress, but it seems that such behavior doesn't break other yielding engines (
vinylis the only one).In order not to do unnecessary function call when MVCC is disabled, function
memtx_tx_prepare_finalizeisstatic inlinenow and uses a*_slowhelper when MVCC is actually enabled - such approach is common in this subsystem.Closes #10715