-
Notifications
You must be signed in to change notification settings - Fork 377
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
vinyl: fix index build crash on invalid UPSERT #10027
vinyl: fix index build crash on invalid UPSERT #10027
Conversation
There is no regression test in the patch. I cannot guarantee that fuzzing test will cover patches for all found bugs. So there is a non-zero possibility that bug #10026 will happen again in the future. |
Like UPDATE, UPSERT must not modify primary key parts. Unlike UPDATE, such an invalid UPSERT statement doesn't fail (raise an error) - we just log the error and ignore the statement. The problem is, we don't clear txn_stmt. As a result, if we're currently building a new index, the on_replace trigger installed by the build procedure will try to process this statement, triggering the assertion in the transaction manager that doesn't expect any statements in a secondary index without the corresponding statement in the primary index: ./src/box/vy_tx.c:728: vy_tx_prepare: Assertion `lsm->space_id == current_space_id' failed. Let's fix this by clearing the txn_stmt corresponding to a skipped UPSERT. Note, this also means that on_replace triggers installed by the user won't run on invalid UPSERT (hence test/vinyl/on_replace.result update), but this is consistent with the memtx engine, which doesn't run them in this case, either. Closes tarantool#10026 NO_DOC=bug fix
87b72b6
to
e582882
Compare
Added a regression test. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
The |
Cherry-picked to 2.11 and 3.1. |
Like
UPDATE
,UPSERT
must not modify primary key parts. UnlikeUPDATE
, such an invalidUPSERT
statement doesn't fail (raise an error) - we just log the error and ignore the statement. The problem is, we don't cleartxn_stmt
. As a result, if we're currently building a new index, theon_replace
trigger installed by the build procedure will try to process this statement, triggering the assertion in the transaction manager that doesn't expect any statements in a secondary index without the corresponding statement in the primary index:Let's fix this by clearing the
txn_stmt
corresponding to a skippedUPSERT
.Note, this also means that
on_replace
triggers installed by the user won't run on invalidUPSERT
(hencetest/vinyl/on_replace.result
update), but this is consistent with the memtx engine, which doesn't run them in this case, either.Closes #10026