Re: [Tarantool-patches] [PATCH v4 12/12] txm: add a test

[Nikita Pettik <korablev@tarantool.org>]

On 22 Sep 20:58, Aleksandr Lyapunov wrote:
> Hi, thanks for the review.
> I added the tests (triggers and recovery I'll do a bit later).
> Found some bugs, here the fixes I made to make the tests pass.

A few non-functional fixes:

diff --git a/src/box/memtx_tx.c b/src/box/memtx_tx.c
index 55748ad64..b10440ae3 100644
--- a/src/box/memtx_tx.c
+++ b/src/box/memtx_tx.c
@@ -418,8 +418,8 @@ memtx_tx_story_gc_step()
                if (link->newer_story == NULL) {
                        /*
                         * We are at the top of the chain. That means
-                        * that story->tuple is in index. If the story is
-                        * actually delete the tuple, it must be deleted from
+                        * that story->tuple is in index. If the story
+                        * actually deletes the tuple, it must be deleted from
                         * index.
                         */
                        if (story->del_psn > 0 && story->space != NULL) {
@@ -946,7 +946,7 @@ memtx_tx_history_prepare_stmt(struct txn_stmt *stmt)
 
        if (stmt->del_story != NULL) {
                stmt->del_story->del_psn = stmt->txn->psn;
-               // Let's conflict all other deleting stories.
+               /* Let's conflict all other deleting stories. */
                struct txn_stmt *dels = stmt->del_story->del_stmt;
                while (dels != NULL) {
                        struct txn_stmt *next = dels->next_in_del_list;
@@ -956,7 +956,7 @@ memtx_tx_history_prepare_stmt(struct txn_stmt *stmt)
                        }
                        dels = next;
                }
-               // Set the only deleting statement for that story.
+               /* Set the only deleting statement for that story. */
                stmt->del_story->del_stmt = stmt;
                stmt->next_in_del_list = NULL;
        }
@@ -1008,8 +1008,8 @@ memtx_tx_tuple_clarify_slow(struct txn *txn, struct space *space,
        if (!own_change)
                memtx_tx_track_read(txn, space, tuple);
        if (mk_index != 0) {
-               assert(false); /* TODO: multiindex */
-               panic("multikey indexes are not supported int TX manager");
+               unreachable(); /* TODO: multiindex */
+               panic("multikey indexes are not supported in TX manager");
        }
        return result;
 }
diff --git a/src/box/memtx_tx.h b/src/box/memtx_tx.h
index 25a203880..6aadad1f0 100644
--- a/src/box/memtx_tx.h
+++ b/src/box/memtx_tx.h
@@ -311,7 +311,7 @@ memtx_tx_tuple_clarify(struct txn *txn, struct space *space,
 }
 
 /**
- * Notify manager the a space is deleted.
+ * Notify manager that the space is deleted.
  * It's necessary because there is a chance that garbage collector hasn't
  * deleted all stories of that space and in that case some actions of
  * story's destructor are not applicable.

Othewise LGTM
 
> diff --git a/src/box/memtx_tx.c b/src/box/memtx_tx.c
> index d72fa92..5aacd22 100644
> --- a/src/box/memtx_tx.c
> +++ b/src/box/memtx_tx.c
> @@ -422,7 +422,7 @@ memtx_tx_story_gc_step()
>               * actually delete the tuple, it must be deleted from
>               * index.
>               */
> -            if (story->del_psn > 0) {
> +            if (story->del_psn > 0 && story->space != NULL) {
>                  struct index *index = story->space->index[i];
>                  struct tuple *unused;
>                  if (index_replace(index, story->tuple, NULL,
> @@ -801,7 +801,6 @@ memtx_tx_history_rollback_stmt(struct txn_stmt *stmt)
>              memtx_tx_story_unlink(story, i);
>          }
>          stmt->add_story->add_stmt = NULL;
> -        memtx_tx_story_delete(stmt->add_story);
>          stmt->add_story = NULL;
>          tuple_unref(stmt->new_tuple);
>      }
> @@ -945,8 +944,22 @@ memtx_tx_history_prepare_stmt(struct txn_stmt *stmt)
>      if (stmt->add_story != NULL)
>          stmt->add_story->add_psn = stmt->txn->psn;
> 
> -    if (stmt->del_story != NULL)
> +    if (stmt->del_story != NULL) {
>          stmt->del_story->del_psn = stmt->txn->psn;
> +        // Let's conflict all other deleting stories.
> +        struct txn_stmt *dels = stmt->del_story->del_stmt;
> +        while (dels != NULL) {
> +            struct txn_stmt *next = dels->next_in_del_list;
> +            if (dels != stmt) {
> +                dels->del_story = NULL;
> +                dels->next_in_del_list = NULL;
> +            }
> +            dels = next;
> +        }
> +        // Set the only deleting statement for that story.
> +        stmt->del_story->del_stmt = stmt;
> +        stmt->next_in_del_list = NULL;
> +    }
>  }