Re: [Tarantool-patches] [PATCH v3 13/13] tmx: use new tx manager in memtx

[Aleksandr Lyapunov <alyapunov@tarantool.org>]

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;
>>   }
>>   
>>