Skip to content

memtx: finalize prepare of all transactions in memtx MVCC#11007

Merged
locker merged 1 commit intotarantool:masterfrom
drewdzzz:memtx_mvcc_sysview
Feb 4, 2025
Merged

memtx: finalize prepare of all transactions in memtx MVCC#11007
locker merged 1 commit intotarantool:masterfrom
drewdzzz:memtx_mvcc_sysview

Conversation

@drewdzzz
Copy link
Contributor

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).

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

@coveralls
Copy link

coveralls commented Dec 27, 2024

Coverage Status

coverage: 87.345% (-0.01%) from 87.357%
when pulling b6a7ae7 on drewdzzz:memtx_mvcc_sysview
into 43aa0bf
on tarantool:master
.

@locker locker assigned drewdzzz and unassigned locker Jan 13, 2025
@CuriousGeorgiy CuriousGeorgiy removed their assignment Jan 13, 2025
@drewdzzz drewdzzz assigned CuriousGeorgiy and unassigned drewdzzz Jan 28, 2025
@drewdzzz drewdzzz requested a review from locker January 29, 2025 09:46
@drewdzzz drewdzzz assigned locker and unassigned drewdzzz Jan 29, 2025
@locker locker assigned drewdzzz and unassigned locker Jan 29, 2025
@drewdzzz drewdzzz added full-ci Enables all tests for a pull request backport/2.11 Automatically create a 2.11 backport PR backport/3.2 Automatically create a 3.2 backport PR backport/3.3 Automatically create a 3.3 backport PR labels 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
@drewdzzz drewdzzz removed backport/2.11 Automatically create a 2.11 backport PR backport/3.2 Automatically create a 3.2 backport PR backport/3.3 Automatically create a 3.3 backport PR labels Feb 3, 2025
@drewdzzz drewdzzz assigned locker and unassigned drewdzzz Feb 4, 2025
@locker locker merged commit a217b0f into tarantool:master Feb 4, 2025
61 checks passed
@locker
Copy link
Member

locker commented Feb 4, 2025

Cherry-picked to 2.11, 3.2, 3.3.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

full-ci Enables all tests for a pull request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Assertion `victim->psn == 0' failed in memtx_tx_handle_conflict()

5 participants