[Tarantool-patches] [PATCH v3 13/13] tmx: use new tx manager in memtx
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Fri Jul 17 01:26:00 MSK 2020
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;
> }
>
>
More information about the Tarantool-patches
mailing list