Tarantool development patches archive
 help / color / mirror / Atom feed
From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
To: Aleksandr Lyapunov <alyapunov@tarantool.org>,
	tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH v3 13/13] tmx: use new tx manager in memtx
Date: Fri, 17 Jul 2020 00:26:00 +0200	[thread overview]
Message-ID: <dafae4b9-da65-00d4-b322-bdc4fa8c9635@tarantool.org> (raw)
In-Reply-To: <1594821336-14468-14-git-send-email-alyapunov@tarantool.org>

Thanks for the patch!

See 5 comments below.

On 15.07.2020 15:55, Aleksandr Lyapunov wrote:
> Use mvcc transaction engine in memtx if the engine is enabled.
> 
> Closes #4897
> ---
>  src/box/memtx_engine.c | 40 ++++++++++++++++++++++++++++++++++++----
>  src/box/memtx_space.c  | 28 ++++++++++++++++++++++++----
>  src/box/txn.c          |  3 +++
>  src/box/vinyl.c        | 14 ++++++++++----
>  4 files changed, 73 insertions(+), 12 deletions(-)
> 
> diff --git a/src/box/memtx_engine.c b/src/box/memtx_engine.c
> index dfd6fce..1402dd7 100644
> --- a/src/box/memtx_engine.c
> +++ b/src/box/memtx_engine.c
> @@ -335,10 +335,39 @@ static int
>  memtx_engine_begin(struct engine *engine, struct txn *txn)
>  {
>  	(void)engine;
> -	txn_can_yield(txn, false);
> +	if (!tx_manager_use_mvcc_engine)
> +		txn_can_yield(txn, false);

1. You can pass tx_manager_use_mvcc_engine as a second
argument to txn_can_yield() instead of adding one another
'if'.

>  	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)
> +			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) {
> +			ssize_t bsize = txm_history_commit_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,16 @@ 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;
>  	uint32_t index_count;
>  
>  	/* Only roll back the changes if they were made. */
>  	if (stmt->engine_savepoint == NULL)
>  		return;
>  
> +	if (stmt->add_story != NULL || stmt->del_story != NULL)
> +		return txm_history_rollback_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)
> @@ -914,8 +946,8 @@ static const struct engine_vtab memtx_engine_vtab = {
>  	/* .complete_join = */ memtx_engine_complete_join,
>  	/* .begin = */ memtx_engine_begin,
>  	/* .begin_statement = */ generic_engine_begin_statement,
> -	/* .prepare = */ generic_engine_prepare,
> -	/* .commit = */ generic_engine_commit,
> +	/* .prepare = */ memtx_engine_prepare,
> +	/* .commit = */ memtx_engine_commit,
>  	/* .rollback_statement = */ memtx_engine_rollback_statement,
>  	/* .rollback = */ generic_engine_rollback,
>  	/* .switch_to_ro = */ generic_engine_switch_to_ro,
> diff --git a/src/box/memtx_space.c b/src/box/memtx_space.c
> index e48ed3a..66f28c9 100644
> --- a/src/box/memtx_space.c
> +++ b/src/box/memtx_space.c
> @@ -260,6 +260,20 @@ memtx_space_replace_all_keys(struct space *space, struct tuple *old_tuple,
>  	if (pk == NULL)
>  		return -1;
>  	assert(pk->def->opts.is_unique);
> +
> +	if (tx_manager_use_mvcc_engine) {
> +		struct txn *txn = in_txn();
> +		struct txn_stmt *stmt =
> +			txn == NULL ? NULL : txn_current_stmt(txn);
> +		if (stmt != NULL) {
> +			return txm_history_add_stmt(stmt, old_tuple, new_tuple,
> +						    mode, result);
> +		} else {
> +			/** Ephemeral space */
> +			assert(space->def->id == 0);
> +		}
> +	}
> +
>  	/*
>  	 * If old_tuple is not NULL, the index has to
>  	 * find and delete it, or return an error.
> @@ -896,7 +910,9 @@ memtx_space_check_format(struct space *space, struct tuple_format *format)
>  	if (txn_check_singlestatement(txn, "space format check") != 0)
>  		return -1;
>  
> -	txn_can_yield(txn, true);
> +	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.

>  
>  	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.

>  	return rc;
>  }
>  
> @@ -1054,7 +1071,9 @@ memtx_space_build_index(struct space *src_space, struct index *new_index,
>  	if (txn_check_singlestatement(txn, "index build") != 0)
>  		return -1;
>  
> -	txn_can_yield(txn, true);
> +	bool could_yield = txn_has_flag(txn, TXN_CAN_YIELD);
> +	if (!could_yield)
> +		txn_can_yield(txn, true);
>  
>  	struct memtx_engine *memtx = (struct memtx_engine *)src_space->engine;
>  	struct memtx_ddl_state state;
> @@ -1132,7 +1151,8 @@ memtx_space_build_index(struct space *src_space, struct index *new_index,
>  	iterator_delete(it);
>  	diag_destroy(&state.diag);
>  	trigger_clear(&on_replace);
> -	txn_can_yield(txn, false);
> +	if (!could_yield)
> +		txn_can_yield(txn, false);
>  	return rc;
>  }
>  
> diff --git a/src/box/txn.c b/src/box/txn.c
> index b4888b3..b9a190f 100644
> --- a/src/box/txn.c
> +++ b/src/box/txn.c
> @@ -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?

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

  reply	other threads:[~2020-07-16 22:26 UTC|newest]

Thread overview: 41+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-15 13:55 [Tarantool-patches] [PATCH v3 00/13] Transaction engine for memtx engine Aleksandr Lyapunov
2020-07-15 13:55 ` [Tarantool-patches] [PATCH v3 01/13] Update license file (2020) Aleksandr Lyapunov
2020-07-15 13:55 ` [Tarantool-patches] [PATCH v3 02/13] Check data_offset overflow in struct tuple Aleksandr Lyapunov
2020-07-16 14:27   ` Nikita Pettik
2020-07-15 13:55 ` [Tarantool-patches] [PATCH v3 03/13] vinyl: rename tx_manager -> vy_tx_manager Aleksandr Lyapunov
2020-07-15 16:04   ` Nikita Pettik
2020-07-16  8:17     ` Aleksandr Lyapunov
2020-07-15 13:55 ` [Tarantool-patches] [PATCH v3 04/13] txm: introduce dirty tuples Aleksandr Lyapunov
2020-07-15 16:22   ` Nikita Pettik
2020-07-16  0:05   ` Vladislav Shpilevoy
2020-07-15 13:55 ` [Tarantool-patches] [PATCH v3 05/13] txm: save txn in txn_stmt Aleksandr Lyapunov
2020-07-15 16:23   ` Nikita Pettik
2020-07-15 13:55 ` [Tarantool-patches] [PATCH v3 06/13] txm: add TX status Aleksandr Lyapunov
2020-07-15 16:42   ` Nikita Pettik
2020-07-16  0:08   ` Vladislav Shpilevoy
2020-07-15 13:55 ` [Tarantool-patches] [PATCH v3 07/13] txm: save does_require_old_tuple flag in txn_stmt Aleksandr Lyapunov
2020-07-15 16:49   ` Nikita Pettik
2020-07-16  0:09   ` Vladislav Shpilevoy
2020-07-15 13:55 ` [Tarantool-patches] [PATCH v3 08/13] txm: introduce tx manager Aleksandr Lyapunov
2020-07-15 16:51   ` Nikita Pettik
2020-07-15 22:01   ` Vladislav Shpilevoy
2020-07-16  0:10   ` Vladislav Shpilevoy
2020-07-15 13:55 ` [Tarantool-patches] [PATCH v3 09/13] tmx: introduce prepare sequence number Aleksandr Lyapunov
2020-07-15 17:13   ` Nikita Pettik
2020-07-16  0:11   ` Vladislav Shpilevoy
2020-07-15 13:55 ` [Tarantool-patches] [PATCH v3 10/13] tmx: introduce conflict tracker Aleksandr Lyapunov
2020-07-16  0:16   ` Vladislav Shpilevoy
2020-07-15 13:55 ` [Tarantool-patches] [PATCH v3 11/13] txm: introduce txm_story Aleksandr Lyapunov
2020-07-16  0:20   ` Vladislav Shpilevoy
2020-07-17  6:16     ` Aleksandr Lyapunov
2020-07-16 22:25   ` Vladislav Shpilevoy
2020-07-15 13:55 ` [Tarantool-patches] [PATCH v3 12/13] txm: clarify all fetched tuples Aleksandr Lyapunov
2020-07-15 13:55 ` [Tarantool-patches] [PATCH v3 13/13] tmx: use new tx manager in memtx Aleksandr Lyapunov
2020-07-16 22:26   ` Vladislav Shpilevoy [this message]
2020-07-17  5:08     ` Aleksandr Lyapunov
2020-07-23 20:58       ` Vladislav Shpilevoy
2020-07-15 15:47 ` [Tarantool-patches] [PATCH v3 00/13] Transaction engine for memtx engine Aleksandr Lyapunov
2020-07-15 16:38 ` Aleksandr Lyapunov
2020-07-15 16:39 ` Aleksandr Lyapunov
2020-07-15 16:40 ` Aleksandr Lyapunov
2020-07-16  0:05 ` Vladislav Shpilevoy

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=dafae4b9-da65-00d4-b322-bdc4fa8c9635@tarantool.org \
    --to=v.shpilevoy@tarantool.org \
    --cc=alyapunov@tarantool.org \
    --cc=tarantool-patches@dev.tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH v3 13/13] tmx: use new tx manager in memtx' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox