From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp53.i.mail.ru (smtp53.i.mail.ru [94.100.177.113]) (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 B115942EF5C for ; Fri, 12 Jun 2020 01:18:46 +0300 (MSK) From: Ilya Kosarev Date: Fri, 12 Jun 2020 01:18:44 +0300 Message-Id: <20200611221844.6511-1-i.kosarev@tarantool.org> Subject: [Tarantool-patches] [PATCH v2] memtx: fix tuples references on concurrent replaces List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: tarantool-patches@dev.tarantool.org Since 527b02a2ee6a9a205d8e2c8f38bbb84edf0d6557 (memtx: add yields during index build) memtx_build_on_replace was introduced to handle concurrent updates. The problem here was that the tuples being handled with this trigger did not get reference counter promotion, leading to a number of wrong behavior cases. Now this problem is solved. This problem was found through primary index altering with updates in background fiber. Corresponding test is introduced. Closes #4973 --- Branch: https://github.com/tarantool/tarantool/tree/i.kosarev/gh-4973-segfaults-on-alter Issue: https://github.com/tarantool/tarantool/issues/4973 @ChangeLog: * Fix concurrent replaces on index building. Tuples are now referenced on all needed execution paths. Changes in v2: - stmt->new_tuple NULL case considered - respectively extended test src/box/memtx_space.c | 8 +- test/engine/engine.cfg | 5 +- .../gh-4973-concurrent-alter-fails.result | 74 +++++++++++++++++++ .../gh-4973-concurrent-alter-fails.test.lua | 32 ++++++++ 4 files changed, 117 insertions(+), 2 deletions(-) create mode 100644 test/engine/gh-4973-concurrent-alter-fails.result create mode 100644 test/engine/gh-4973-concurrent-alter-fails.test.lua diff --git a/src/box/memtx_space.c b/src/box/memtx_space.c index 09f0de4cec..8755920266 100644 --- a/src/box/memtx_space.c +++ b/src/box/memtx_space.c @@ -975,10 +975,16 @@ memtx_build_on_replace(struct trigger *trigger, void *event) DUP_REPLACE_OR_INSERT; state->rc = index_replace(state->index, stmt->old_tuple, stmt->new_tuple, mode, &delete); - if (state->rc != 0) { diag_move(diag_get(), &state->diag); + return 0; } + /* + * All tuples stored in a memtx space must be + * referenced by the primary index. + */ + if (state->index->def->iid == 0 && stmt->new_tuple != NULL) + tuple_ref(stmt->new_tuple); return 0; } diff --git a/test/engine/engine.cfg b/test/engine/engine.cfg index f1e7de2745..f017a03ec2 100644 --- a/test/engine/engine.cfg +++ b/test/engine/engine.cfg @@ -5,7 +5,10 @@ }, "func_index.test.lua": { "memtx": {"engine": "memtx"} - } + }, + "gh-4973-concurrent-alter-fails.test.lua": { + "memtx": {"engine": "memtx"} + } } diff --git a/test/engine/gh-4973-concurrent-alter-fails.result b/test/engine/gh-4973-concurrent-alter-fails.result new file mode 100644 index 0000000000..1a6624a479 --- /dev/null +++ b/test/engine/gh-4973-concurrent-alter-fails.result @@ -0,0 +1,74 @@ +-- test-run result file version 2 +test_run = require('test_run').new() + | --- + | ... +errinj = box.error.injection + | --- + | ... + +space = box.schema.space.create('gh-4973-alter', {engine = 'memtx'}) + | --- + | ... +space:format({ {'key', 'unsigned'}, {'value', 'string'}, {'key_new', 'unsigned'} }) + | --- + | ... +index = space:create_index('primary', {parts = {'key'}}) + | --- + | ... + +N = 10000 + | --- + | ... +value = string.rep('a', 10) + | --- + | ... +box.atomic(function() for i = 1, N do space:insert({i, value, i}) end end) + | --- + | ... + +test_run:cmd("setopt delimiter ';'") + | --- + | - true + | ... +function random_update() + local x = space.index.primary:random(math.random(N)) + local op = math.random(10) + if op < 10 then space:update({x[1]}, {{'=', 2, string.rep('b', 10)}}) end + if op == 10 then space:delete({x[1]}) end +end; + | --- + | ... + +finished_updates = false; + | --- + | ... +fiber = require('fiber'); + | --- + | ... +updater = fiber.create(function() + for _ = 1, N do random_update() end + finished_updates = true +end) +test_run:cmd("setopt delimiter ''"); + | --- + | ... + +space.index.primary:alter({parts = {'key_new'}}) + | --- + | ... +errinj.set('ERRINJ_BUILD_INDEX_DELAY', true) + | --- + | - ok + | ... +test_run:wait_cond(function() return finished_updates end, 5) + | --- + | - true + | ... +errinj.set('ERRINJ_BUILD_INDEX_DELAY', false) + | --- + | - ok + | ... +box.snapshot() + | --- + | - ok + | ... diff --git a/test/engine/gh-4973-concurrent-alter-fails.test.lua b/test/engine/gh-4973-concurrent-alter-fails.test.lua new file mode 100644 index 0000000000..4fd1a848d4 --- /dev/null +++ b/test/engine/gh-4973-concurrent-alter-fails.test.lua @@ -0,0 +1,32 @@ +test_run = require('test_run').new() +errinj = box.error.injection + +space = box.schema.space.create('gh-4973-alter', {engine = 'memtx'}) +space:format({ {'key', 'unsigned'}, {'value', 'string'}, {'key_new', 'unsigned'} }) +index = space:create_index('primary', {parts = {'key'}}) + +N = 10000 +value = string.rep('a', 10) +box.atomic(function() for i = 1, N do space:insert({i, value, i}) end end) + +test_run:cmd("setopt delimiter ';'") +function random_update() + local x = space.index.primary:random(math.random(N)) + local op = math.random(10) + if op < 10 then space:update({x[1]}, {{'=', 2, string.rep('b', 10)}}) end + if op == 10 then space:delete({x[1]}) end +end; + +finished_updates = false; +fiber = require('fiber'); +updater = fiber.create(function() + for _ = 1, N do random_update() end + finished_updates = true +end) +test_run:cmd("setopt delimiter ''"); + +space.index.primary:alter({parts = {'key_new'}}) +errinj.set('ERRINJ_BUILD_INDEX_DELAY', true) +test_run:wait_cond(function() return finished_updates end, 5) +errinj.set('ERRINJ_BUILD_INDEX_DELAY', false) +box.snapshot() -- 2.17.1