From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp47.i.mail.ru (smtp47.i.mail.ru [94.100.177.107]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 90773469719 for ; Thu, 22 Oct 2020 19:02:46 +0300 (MSK) From: Aleksandr Lyapunov Date: Thu, 22 Oct 2020 19:02:33 +0300 Message-Id: <1603382553-5570-3-git-send-email-alyapunov@tarantool.org> In-Reply-To: <1603382553-5570-1-git-send-email-alyapunov@tarantool.org> References: <1603382553-5570-1-git-send-email-alyapunov@tarantool.org> Subject: [Tarantool-patches] [PATCH 2/2] memtx: fix a bug in TX that caused deletion of a durty tuple List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: tarantool-patches@dev.tarantool.org, Nikita Pettik , "Alexander V . Tikhonov" There was a mess in tuple refernce in TX history. Now it was remade in the following asumptions: * a clean tuple belongs to space, and the space implicitly holds a reference to the tuple. * a dirty tuple belongs to TX manager and a reference is held in the corresponding story. Closes #5423 --- src/box/memtx_space.c | 1 + src/box/memtx_tx.c | 54 ++++++++++++++++++++++++++++++++++++++---------- test/box/tx_man.result | 43 ++++++++++++++++++++++++++++++++++++++ test/box/tx_man.test.lua | 16 ++++++++++++++ 4 files changed, 103 insertions(+), 11 deletions(-) diff --git a/src/box/memtx_space.c b/src/box/memtx_space.c index d4b18d9..2a43e64 100644 --- a/src/box/memtx_space.c +++ b/src/box/memtx_space.c @@ -78,6 +78,7 @@ void memtx_space_update_bsize(struct space *space, struct tuple *old_tuple, struct tuple *new_tuple) { + assert(space->vtab->destroy == &memtx_space_destroy); struct memtx_space *memtx_space = (struct memtx_space *)space; ssize_t old_bsize = old_tuple ? box_tuple_bsize(old_tuple) : 0; ssize_t new_bsize = new_tuple ? box_tuple_bsize(new_tuple) : 0; diff --git a/src/box/memtx_tx.c b/src/box/memtx_tx.c index e378054..bcd4a37 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) { @@ -432,6 +432,12 @@ memtx_tx_story_gc_step() panic("failed to rollback change"); } assert(story->tuple == unused); + } else if (i == 0) { + /* + * The tuple is left clean in the space. + * It now belongs to the space and must be referenced. + */ + tuple_ref(story->tuple); } if (link->older.is_story) { @@ -655,6 +661,14 @@ memtx_tx_history_add_stmt(struct txn_stmt *stmt, struct tuple *old_tuple, &replaced) != 0) goto fail; memtx_tx_story_link_tuple(add_story, replaced, i); + if (i == 0 && replaced != NULL && !replaced->is_dirty) { + /* + * The tuple was clean and thus belonged to + * the space. Now tx manager takes ownership + * of it. + */ + tuple_unref(replaced); + } add_story_linked++; struct tuple *visible_replaced = NULL; @@ -729,15 +743,16 @@ memtx_tx_history_add_stmt(struct txn_stmt *stmt, struct tuple *old_tuple, collected_conflicts = collected_conflicts->next; } - /* - * We now reference both new and old tuple because the stmt holds - * pointers to them. - */ - if (stmt->new_tuple != NULL) - tuple_ref(stmt->new_tuple); *result = old_tuple; - if (*result != NULL) + if (*result != NULL) { + /* + * The result must be a referenced pointer. The caller must + * unreference it by itself. + * Actually now it goes only to stmt->old_tuple, and + * stmt->old_tuple is unreferenced when stmt is destroyed. + */ tuple_ref(*result); + } return 0; fail: @@ -756,6 +771,10 @@ memtx_tx_history_add_stmt(struct txn_stmt *stmt, struct tuple *old_tuple, unreachable(); panic("failed to rollback change"); } + if (i == 0 && was != NULL && !was->is_dirty) { + /* Just rollback previous tuple_unref. */ + tuple_ref(was); + } memtx_tx_story_unlink(stmt->add_story, i); @@ -787,6 +806,10 @@ memtx_tx_history_rollback_stmt(struct txn_stmt *stmt) for (uint32_t i = 0; i < story->index_count; i++) { struct memtx_story_link *link = &story->link[i]; if (link->newer_story == NULL) { + /* + * We are at top of story list and thus + * story->tuple is in index directly. + */ struct tuple *unused; struct index *index = stmt->space->index[i]; struct tuple *was = memtx_tx_story_older_tuple(link); @@ -796,6 +819,17 @@ memtx_tx_history_rollback_stmt(struct txn_stmt *stmt) unreachable(); panic("failed to rollback change"); } + if (i == 0 && was != NULL && + !link->older.is_story) { + /* + * That was the last story in history. + * The last tuple now belongs to space + * and the space must hold a reference + * to it. + */ + tuple_ref(was); + } + } else { struct memtx_story *newer = link->newer_story; assert(newer->link[i].older.is_story); @@ -813,7 +847,6 @@ memtx_tx_history_rollback_stmt(struct txn_stmt *stmt) } stmt->add_story->add_stmt = NULL; stmt->add_story = NULL; - tuple_unref(stmt->new_tuple); } if (stmt->del_story != NULL) { @@ -987,7 +1020,6 @@ memtx_tx_history_commit_stmt(struct txn_stmt *stmt) assert(stmt->del_story->del_stmt == stmt); assert(stmt->next_in_del_list == NULL); res -= stmt->del_story->tuple->bsize; - tuple_unref(stmt->del_story->tuple); stmt->del_story->del_stmt = NULL; stmt->del_story = NULL; } diff --git a/test/box/tx_man.result b/test/box/tx_man.result index 3a6375d..a294165 100644 --- a/test/box/tx_man.result +++ b/test/box/tx_man.result @@ -826,6 +826,49 @@ s:select{} | - - [1, 3] | ... +s:drop() + | --- + | ... +s2:drop() + | --- + | ... + +-- https://github.com/tarantool/tarantool/issues/5423 +s = box.schema.space.create('test') + | --- + | ... +i1 = s:create_index('pk', {parts={{1, 'uint'}}}) + | --- + | ... +i2 = s:create_index('sec', {parts={{2, 'uint'}}}) + | --- + | ... + +s:replace{1, 0} + | --- + | - [1, 0] + | ... +s:delete{1} + | --- + | - [1, 0] + | ... +collectgarbage() + | --- + | - 0 + | ... +s:replace{1, 1} + | --- + | - [1, 1] + | ... +s:replace{1, 2 } + | --- + | - [1, 2] + | ... + +s:drop() + | --- + | ... + test_run:cmd("switch default") | --- | - true diff --git a/test/box/tx_man.test.lua b/test/box/tx_man.test.lua index 60fa52b..53b0773 100644 --- a/test/box/tx_man.test.lua +++ b/test/box/tx_man.test.lua @@ -236,6 +236,22 @@ s:select{} tx3:commit() s:select{} +s:drop() +s2:drop() + +-- https://github.com/tarantool/tarantool/issues/5423 +s = box.schema.space.create('test') +i1 = s:create_index('pk', {parts={{1, 'uint'}}}) +i2 = s:create_index('sec', {parts={{2, 'uint'}}}) + +s:replace{1, 0} +s:delete{1} +collectgarbage() +s:replace{1, 1} +s:replace{1, 2 } + +s:drop() + test_run:cmd("switch default") test_run:cmd("stop server tx_man") test_run:cmd("cleanup server tx_man") -- 2.7.4