Re: [Tarantool-patches] [PATCH 16/16] tx: use new tx manager in memtx

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Thanks for the patch!

See 6 comments below.

On 08.07.2020 17:14, Aleksandr Lyapunov wrote:
> ---
>  src/box/memtx_engine.c | 49 ++++++++++++++++++++++++++++++++++++++++++-------
>  src/box/memtx_space.c  | 12 ++++++++++++
>  src/box/txn.c          |  3 +++
>  3 files changed, 57 insertions(+), 7 deletions(-)
> 
> diff --git a/src/box/memtx_engine.c b/src/box/memtx_engine.c
> index dfd6fce..b5c5f03 100644
> --- a/src/box/memtx_engine.c
> +++ b/src/box/memtx_engine.c
> @@ -339,6 +339,35 @@ memtx_engine_begin(struct engine *engine, struct txn *txn)
>  	return 0;
>  }
>  
> +static int
> +memtx_engine_prepare(struct engine *engine, struct txn *txn)
> +{
> +	(void)engine;
> +	struct txn_stmt *stmt;
> +	stailq_foreach_entry(stmt, &txn->stmts, next) {
> +		if (stmt->add_story != NULL || stmt->del_story != NULL)

1. Seems like it is worth moving this check into a static inline
function in the txn.h header. You use it a lot.

> +			txm_history_prepare_stmt(stmt);
> +	}
> +	return 0;
> +}
> +
> +static void
> +memtx_engine_commit(struct engine *engine, struct txn *txn)
> +{
> +	(void)engine;
> +	struct txn_stmt *stmt;
> +	stailq_foreach_entry(stmt, &txn->stmts, next) {
> +		if (stmt->add_story != NULL || stmt->del_story != NULL)
> +		{

2. Opening brace should be on the same line as 'if'. The same please
find and fix in all the other places.

> +			ssize_t bsize = txm_history_release_stmt(stmt);
> +			assert(stmt->space->engine == engine);
> +			struct memtx_space *mspace =
> +				(struct memtx_space *)stmt->space;
> +			mspace->bsize += bsize;
> +		}
> +	}
> +}
> +
>  static void
>  memtx_engine_rollback_statement(struct engine *engine, struct txn *txn,
>  				struct txn_stmt *stmt)
> @@ -348,13 +377,17 @@ memtx_engine_rollback_statement(struct engine *engine, struct txn *txn,
>  	if (stmt->old_tuple == NULL && stmt->new_tuple == NULL)
>  		return;
>  	struct space *space = stmt->space;
> -	struct memtx_space *memtx_space = (struct memtx_space *)space;
> +	struct memtx_space* memtx_space = (struct memtx_space*) space;

3. Unnecessary diff.

>  	uint32_t index_count;
>  
>  	/* Only roll back the changes if they were made. */
>  	if (stmt->engine_savepoint == NULL)
>  		return;
>  
> +	if (memtx_space->replace == memtx_space_replace_all_keys &&

4. Why is it done only in case of memtx_space_replace_all_keys?

> +	    (stmt->add_story != NULL || stmt->del_story != NULL))
> +		return txm_history_unlink_stmt(stmt);
> +
>  	if (memtx_space->replace == memtx_space_replace_all_keys)
>  		index_count = space->index_count;
>  	else if (memtx_space->replace == memtx_space_replace_primary_key)
> @@ -362,12 +395,14 @@ memtx_engine_rollback_statement(struct engine *engine, struct txn *txn,
>  	else
>  		panic("transaction rolled back during snapshot recovery");
>  
> -	for (uint32_t i = 0; i < index_count; i++) {
> -		struct tuple *unused;
> -		struct index *index = space->index[i];
> +	for (uint32_t i = 0; i < index_count; i++)
> +	{
> +		struct tuple* unused;
> +		struct index* index = space->index[i];

5. Unnecessary changes. Also not correct. We use 'type *' instead of 'type*'.
We put { on the same line as cycle, not on a separate line. The same below
and in all the other places.

>  		/* Rollback must not fail. */
>  		if (index_replace(index, stmt->new_tuple, stmt->old_tuple,
> -				  DUP_INSERT, &unused) != 0) {
> +		                  DUP_INSERT, &unused) != 0)
> +		{
>  			diag_log();
>  			unreachable();
>  			panic("failed to rollback change");
> diff --git a/src/box/memtx_space.c b/src/box/memtx_space.c
> index 5820c40..28552a9 100644
> --- a/src/box/memtx_space.c
> +++ b/src/box/memtx_space.c
> @@ -260,6 +260,18 @@ memtx_space_replace_all_keys(struct space *space, struct tuple *old_tuple,
>  	if (pk == NULL)
>  		return -1;
>  	assert(pk->def->opts.is_unique);
> +
> +	struct txn *txn = in_txn();
> +	struct txn_stmt *stmt = txn == NULL ? NULL : txn_current_stmt(txn);
> +	if (stmt != NULL) {
> +		return txm_history_link_stmt(txn_current_stmt(txn),

6. Why do you get txn_current_stmt(stmt) again? You already have it
in stmt variable.

> +		                             old_tuple, new_tuple, mode,
> +		                             result);
> +	} else {
> +		/** Ephemeral space */
> +		assert(space->def->id == 0);
> +	}