[Tarantool-patches] [PATCH] vinyl: add NULL check of xrow_upsert_execute() retval

[Nikita Pettik <korablev@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