[Tarantool-patches] [PATCH v3 13/13] tmx: use new tx manager in memtx
Aleksandr Lyapunov
alyapunov at tarantool.org
Fri Jul 17 08:08:44 MSK 2020
Hi, thanks for review!
On 7/17/20 1:26 AM, Vladislav Shpilevoy wrote:
> Th
> 1. You can pass tx_manager_use_mvcc_engine as a second
> argument to txn_can_yield() instead of adding one another
> 'if'.
Actually it's not so simple. It would fall in assert.
>
>>
>> + bool could_yield = txn_has_flag(txn, TXN_CAN_YIELD);
>> + if (!could_yield)
>> + txn_can_yield(txn, true);
> 2. This change looks unnecessary. You just added 'if'.
> And still the result is that the txn can yield.
See above. It's just a common restore-to-the-original state semantics.
>
>>
>> struct memtx_engine *memtx = (struct memtx_engine *)space->engine;
>> struct memtx_ddl_state state;
>> @@ -940,7 +956,8 @@ memtx_space_check_format(struct space *space, struct tuple_format *format)
>> iterator_delete(it);
>> diag_destroy(&state.diag);
>> trigger_clear(&on_replace);
>> - txn_can_yield(txn, false);
>> + if (!could_yield)
>> + txn_can_yield(txn, false);
> 3. The same. And the same in the 2 hunks below.
The same!
> @@ -203,6 +203,9 @@ txn_stmt_new(struct region *region)
> static inline void
> txn_stmt_destroy(struct txn_stmt *stmt)
> {
> + if (stmt->add_story != NULL || stmt->del_story != NULL)
> + txm_history_rollback_stmt(stmt);
> 4. Doing rollback from a destructor is totally cursed. Why are you
> doing it from there?
Good, replaced with assert.
>
>> +
>> if (stmt->old_tuple != NULL)
>> tuple_unref(stmt->old_tuple);
>> if (stmt->new_tuple != NULL)
>> diff --git a/src/box/vinyl.c b/src/box/vinyl.c
>> index f9252f1..f69d3d9 100644
>> --- a/src/box/vinyl.c
>> +++ b/src/box/vinyl.c
>> @@ -1084,7 +1084,9 @@ vinyl_space_check_format(struct space *space, struct tuple_format *format)
>> return -1;
>>
>> /* See the comment in vinyl_space_build_index(). */
>> - txn_can_yield(txn, true);
>> + bool could_yield = txn_has_flag(txn, TXN_CAN_YIELD);
>> + if (!could_yield)
>> + txn_can_yield(txn, true);
> 5. Unnecessary change. The same for all the other similar places.
>
>>
>> struct trigger on_replace;
>> struct vy_check_format_ctx ctx;
>> @@ -1136,7 +1138,8 @@ vinyl_space_check_format(struct space *space, struct tuple_format *format)
>> out:
>> diag_destroy(&ctx.diag);
>> trigger_clear(&on_replace);
>> - txn_can_yield(txn, false);
>> + if (!could_yield)
>> + txn_can_yield(txn, false);
>> return rc;
>> }
>>
>> @@ -4183,7 +4186,9 @@ vinyl_space_build_index(struct space *src_space, struct index *new_index,
>> * change the data dictionary, so there is no dirty state
>> * that can be observed.
>> */
>> - txn_can_yield(txn, true);
>> + bool could_yield = txn_has_flag(txn, TXN_CAN_YIELD);
>> + if (!could_yield)
>> + txn_can_yield(txn, true);
>>
>> /*
>> * Iterate over all tuples stored in the space and insert
>> @@ -4284,7 +4289,8 @@ vinyl_space_build_index(struct space *src_space, struct index *new_index,
>> out:
>> diag_destroy(&ctx.diag);
>> trigger_clear(&on_replace);
>> - txn_can_yield(txn, false);
>> + if (!could_yield)
>> + txn_can_yield(txn, false);
>> return rc;
>> }
>>
>>
More information about the Tarantool-patches
mailing list