[Tarantool-patches] [PATCH 16/16] tx: use new tx manager in memtx
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Wed Jul 15 02:45:02 MSK 2020
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);
> + }
More information about the Tarantool-patches
mailing list