From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp60.i.mail.ru (smtp60.i.mail.ru [217.69.128.40]) (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 47CA8469710 for ; Wed, 27 May 2020 05:56:51 +0300 (MSK) From: Nikita Pettik Date: Wed, 27 May 2020 05:56:47 +0300 Message-Id: <776e8b91b93c79dabd2932b5d665236c5da313c8.1590546551.git.korablev@tarantool.org> Subject: [Tarantool-patches] [PATCH] vinyl: add NULL check of xrow_upsert_execute() retval List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: tarantool-patches@dev.tarantool.org Cc: v.shpilevoy@tarantool.org xrow_upsert_execute() can fail and return NULL for various reasons. However, in vy_apply_upsert() the result of xrow_upsert_execute() is used unconditionally which may lead to crash. Let's fix it and in case xrow_upsert_execute() fails return from vy_apply_upsert() NULL value. Closes #4957 --- Brief problem description: if user puts a lot (more than 4000) of upserts which modify the same tuple into one transaction, it may lead to crash. Since the number of update operations exceeds the limit (BOX_UPDATE_OP_CNT_MAX == 4000), they are not allowed to be applied (still all upserts are squashed into one). So xrow_upsert_execute() can return NULL instead ofvalid result which will be dereferenced later. Note that patch is based on np/gh-1622-skip-invalid-upserts branch. If we don't skip invalid upsert which is the result of squashing 4000 other upserts, dump won't be able to finish due to raised error. As a rule, all upserts modifying the same key are squashed and/or executed during dump process. So basically users should not face scenario when a lot of upserts get stuck in disk run. The only case is invalid upserts which are not skipped (in contrast to branch containing fix for 1622) and reside until squash with DELETE statement (AFAIU). So I believe we should not bother with BOX_UPDATE_OP_CNT_MAX restriction as it is mentioned in issue. Branch: https://gitlab.com/tarantool/tarantool/pipelines/149917031 Issue: https://github.com/tarantool/tarantool/issues/4957 @ChangeLog: * Fix crash during squash of many (more than 4000) upserts modifying the same key. src/box/vy_upsert.c | 4 + test/vinyl/gh-4957-too-many-upserts.result | 118 +++++++++++++++++++ test/vinyl/gh-4957-too-many-upserts.test.lua | 48 ++++++++ 3 files changed, 170 insertions(+) create mode 100644 test/vinyl/gh-4957-too-many-upserts.result create mode 100644 test/vinyl/gh-4957-too-many-upserts.test.lua diff --git a/src/box/vy_upsert.c b/src/box/vy_upsert.c index 6855b9820..007921bb2 100644 --- a/src/box/vy_upsert.c +++ b/src/box/vy_upsert.c @@ -133,6 +133,10 @@ vy_apply_upsert(const struct tuple *new_stmt, const struct tuple *old_stmt, new_ops_end, result_mp, result_mp_end, &mp_size, 0, suppress_error, &column_mask); + if (result_mp == NULL) { + region_truncate(region, region_svp); + return NULL; + } result_mp_end = result_mp + mp_size; if (tuple_validate_raw(format, result_mp) != 0) { region_truncate(region, region_svp); diff --git a/test/vinyl/gh-4957-too-many-upserts.result b/test/vinyl/gh-4957-too-many-upserts.result new file mode 100644 index 000000000..203329788 --- /dev/null +++ b/test/vinyl/gh-4957-too-many-upserts.result @@ -0,0 +1,118 @@ +-- test-run result file version 2 +s = box.schema.create_space('test', {engine = 'vinyl'}) + | --- + | ... +pk = s:create_index('pk') + | --- + | ... +s:insert{1, 1} + | --- + | - [1, 1] + | ... +box.snapshot() + | --- + | - ok + | ... + +-- Let's test number of upserts in one transaction that exceeds +-- the limit of operations allowed in one update. +-- +ups_cnt = 5000 + | --- + | ... +box.begin() + | --- + | ... +for i = 1, ups_cnt do s:upsert({1}, {{'&', 2, 1}}) end + | --- + | ... +box.commit() + | --- + | ... +dump_count = box.stat.vinyl().scheduler.dump_count + | --- + | ... +tasks_completed = box.stat.vinyl().scheduler.tasks_completed + | --- + | ... +box.snapshot() + | --- + | - ok + | ... + +fiber = require('fiber') + | --- + | ... +while box.stat.vinyl().scheduler.tasks_inprogress > 0 do fiber.sleep(0.01) end + | --- + | ... + +assert(box.stat.vinyl().scheduler.dump_count - dump_count == 1) + | --- + | - true + | ... +-- Last :snapshot() triggers both dump and compaction processes. +-- +assert(box.stat.vinyl().scheduler.tasks_completed - tasks_completed == 2) + | --- + | - true + | ... + +s:select() + | --- + | - - [1, 1] + | ... + +s:drop() + | --- + | ... + +s = box.schema.create_space('test', {engine = 'vinyl'}) + | --- + | ... +pk = s:create_index('pk') + | --- + | ... + +tuple = {} + | --- + | ... +for i = 1, ups_cnt do tuple[i] = i end + | --- + | ... +_ = s:insert(tuple) + | --- + | ... +box.snapshot() + | --- + | - ok + | ... + +box.begin() + | --- + | ... +for k = 1, ups_cnt do s:upsert({1}, {{'+', k, 1}}) end + | --- + | ... +box.commit() + | --- + | ... +box.snapshot() + | --- + | - ok + | ... +while box.stat.vinyl().scheduler.tasks_inprogress > 0 do fiber.sleep(0.01) end + | --- + | ... + +-- All upserts are ignored since they are squashed to one update +-- operation with too many operations. +-- +assert(s:select()[1][1] == 1) + | --- + | - true + | ... + +s:drop() + | --- + | ... diff --git a/test/vinyl/gh-4957-too-many-upserts.test.lua b/test/vinyl/gh-4957-too-many-upserts.test.lua new file mode 100644 index 000000000..6c201f29e --- /dev/null +++ b/test/vinyl/gh-4957-too-many-upserts.test.lua @@ -0,0 +1,48 @@ +s = box.schema.create_space('test', {engine = 'vinyl'}) +pk = s:create_index('pk') +s:insert{1, 1} +box.snapshot() + +-- Let's test number of upserts in one transaction that exceeds +-- the limit of operations allowed in one update. +-- +ups_cnt = 5000 +box.begin() +for i = 1, ups_cnt do s:upsert({1}, {{'&', 2, 1}}) end +box.commit() +dump_count = box.stat.vinyl().scheduler.dump_count +tasks_completed = box.stat.vinyl().scheduler.tasks_completed +box.snapshot() + +fiber = require('fiber') +while box.stat.vinyl().scheduler.tasks_inprogress > 0 do fiber.sleep(0.01) end + +assert(box.stat.vinyl().scheduler.dump_count - dump_count == 1) +-- Last :snapshot() triggers both dump and compaction processes. +-- +assert(box.stat.vinyl().scheduler.tasks_completed - tasks_completed == 2) + +s:select() + +s:drop() + +s = box.schema.create_space('test', {engine = 'vinyl'}) +pk = s:create_index('pk') + +tuple = {} +for i = 1, ups_cnt do tuple[i] = i end +_ = s:insert(tuple) +box.snapshot() + +box.begin() +for k = 1, ups_cnt do s:upsert({1}, {{'+', k, 1}}) end +box.commit() +box.snapshot() +while box.stat.vinyl().scheduler.tasks_inprogress > 0 do fiber.sleep(0.01) end + +-- All upserts are ignored since they are squashed to one update +-- operation with too many operations. +-- +assert(s:select()[1][1] == 1) + +s:drop() \ No newline at end of file -- 2.17.1