Repository navigation
Fix logical error with mutation and transaction - #90469
Conversation
Description: Suppose we have a part all_1_1_0. trx1: begin trx1: update all_1_1_0->all_1_1_1 mutation: update all_1_1_1->all_1_1_2 trx1: rollback all_1_1_1 dropped mutation: continue all_1_1_0->all_1_1_2 But all_1_1_2 already exists, logical error happens. Solution: If a part has been created or removed in an active transaction, it's not visible to a mutation without transaction. The mutation should wait for the active transaction to commit or rollback.
|
Workflow [PR], commit [3a88c56] Summary: ❌
|
|
|
||
| bool IMergeTreeDataPart::isInvolvedInTransaction() const | ||
| { | ||
| assert(!storage.data_parts_loading_finished || !version.creation_tid.isEmpty() || (state == MergeTreeDataPartState::Temporary /* && std::uncaught_exceptions() */)); |
There was a problem hiding this comment.
!storage.data_parts_loading_finished
Does it mean that the function can return incorrect result if data parts loading is not finished?
There was a problem hiding this comment.
No, storage.data_parts_loading_finished should be true for mutations.
I've changed it to assert(storage.data_parts_loading_finished).
| bool IMergeTreeDataPart::isInvolvedInTransaction() const | ||
| { | ||
| assert(!storage.data_parts_loading_finished || !version.creation_tid.isEmpty() || (state == MergeTreeDataPartState::Temporary /* && std::uncaught_exceptions() */)); | ||
| bool created_in_transaction = !version.creation_tid.isPrehistoric() && version.creation_csn == Tx::UnknownCSN; |
There was a problem hiding this comment.
Is it supposed to be true only when it is being created by a running transaction? If the transaction is committed, then creation_csn will not be UnknownCSN
isInvolvedInTransaction and created_in_transaction sound like it should be truу for any transactions, not only running
There was a problem hiding this comment.
"Is it supposed to be true only when it is being created by a running transaction?"
Yes.
"If the transaction is committed, then creation_csn will not be UnknownCSN"
Then created_in_transaction is true, otherwise it is false if the transaction is running.
There was a problem hiding this comment.
Finally use version.isVisible to check(outdated parts, being created or removed parts in an active transaction).
|
Can you add a test to reproduce the problem in the description? |
Already have one in tests/queries/0_stateless/01168_mutations_isolation_2.sh test update rollback$CLICKHOUSE_CLIENT -q "create table mt (n int) engine=MergeTree order by tuple()" tx 4 "begin transaction" |
| "Deadlock detected: mutation {} in transaction {} depends on earlier mutation {} " | ||
| "from the same transaction with intermediate mutations in between. ", | ||
| mutation_entry.file_name, mutation_entry.tid, earlier_mutation.file_name); | ||
| result.latest_fail_error_code_name = ErrorCodes::getName(ErrorCodes::LOGICAL_ERROR); |
There was a problem hiding this comment.
We use LOGICAL_ERROR for runtime assertions, and this error code means that a bug was detected. In release build it's a normal exception, but in debug and sanitizer builds it aborts like assert would do
But looks like this scenario is possible with someuser inputs, so it's not a suitable error code. Btw, we have DEADLOCK_AVOIDED
67063c9
| "Deadlock detected: mutation {} in transaction {} depends on earlier mutation {} " | ||
| "from the same transaction with intermediate mutations in between. ", | ||
| mutation_entry.file_name, mutation_entry.tid, earlier_mutation.file_name); | ||
| result.latest_fail_error_code_name = ErrorCodes::getName(ErrorCodes::LOGICAL_ERROR); |
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):
Fix logical error when mutation without transaction mutates parts in an active transaction, which is rolled back finally.
Documentation entry for user-facing changes
Details
Suppose we have a part all_1_1_0.
But all_1_1_2 already exists, logical error happens.
Solution:
If a part has been created or removed in an active transaction, it's not visible to a mutation without transaction. The mutation should wait for the active transaction to commit or rollback.