[Tarantool-patches] [PATCH v3 0/2] vinyl: rework upsert operation

[Tarantool-patches] [PATCH v3 0/2] vinyl: rework upsert operation

[Nikita Pettik]

Issues:
https://github.com/tarantool/tarantool/issues/1622
https://github.com/tarantool/tarantool/issues/5105
https://github.com/tarantool/tarantool/issues/5092
https://github.com/tarantool/tarantool/issues/5107
Branch:
https://github.com/tarantool/tarantool/tree/np/gh-5107-dont-squash-ops

@ChangeLog:
 - Rework upsert operation in vinyl so that now (gh-5107):
   - if upsert can't be applied it is skipped and corresponding error is logged (gh-1622);
   - upserts now follow associative property: result of several upserts
     doesn't depend on the order of their application (gh-5105);
   - upserts referring to -1 fieldno are handled correctly now (gh-5087).
   - there's no more upserts squash procedure: upserts referring to the
     same field with arithmetic operations are not merged into one 
     operation since resulting upsert might not be applied - as a result
     both upserts would be ignored (meanwhile only one should be).

Changes in v3:
 - Removed upsert squashing procedure;
 - Added two additional test covering overflow error during upsert
   application;
 - Fixed NULL dereference bug in vy_apply_upsert_on_terminal_stmt().

Nikita Pettik (2):
  vinyl: rework upsert operation
  vinyl: remove squash procedures from source code

 src/box/vinyl.c                 |   2 +-
 src/box/vy_stmt.c               |  28 +-
 src/box/vy_stmt.h               |   5 +-
 src/box/vy_upsert.c             | 302 +++++++++++---------
 src/box/xrow_update.c           | 144 ----------
 src/box/xrow_update.h           |  14 -
 test/unit/vy_iterators_helper.c |   2 +-
 test/vinyl/upsert.result        | 473 ++++++++++++++++++++++++++++++++
 test/vinyl/upsert.test.lua      | 194 +++++++++++++
 9 files changed, 867 insertions(+), 297 deletions(-)

-- 
2.17.1

[Tarantool-patches] [PATCH v3 1/2] vinyl: rework upsert operation

[Nikita Pettik]

Previous upsert implementation had a few drawbacks which led to number
of various bugs and issues.

Issue #5092 (redundant update operations execution)

In a nutshell, application of upsert(s) (on top of another upsert)
consists of two actions (see vy_apply_upsert()): execute and squash.
Consider example:

insert({1, 1})  -- terminal statement, stored on disk
upsert({1}, {{'-', 2, 20}}) -- old ups1
upsert({1}, {{'+', 2, 10}}) -- new ups2

'Execute' step takes update operations from the new upsert and combines them
with key of the old upsert.  {1} + {'+', 2, 10} can't be evaluated since
key consists of only one field. Note that in case upsert doesn't fold
into insert the upsert's tuple and the tuple stored in index can be
different In our particular case, tuple stored on disk has two fields
({1, 1}), so first upsert's update operation can be applied to it:
{1, 1} + {'+', 2, 10} --> {1, 11}. If upsert's operation can't be executed
using key of old upsert, we simply continue processing squash step.
In turn 'squash' is a combination of update operations: arithmetic
operations are combined so we don't have to store actions over the same
field; the rest operations - are merged into single array. As a result,
we get one upsert with squashed operations: upsert({1}, {{'+', 2, -10}}).
Then vy_apply_upsert() is called again to apply new upsert on the top of
terminal statement - insert{1, 1}. Since now tuple has second field,
update operations can be executed and corresponding result is {1, -9}.
It is the final result of upsert application procedure.
Now imagine that we have following upserts:

upsert({1, 1}, {{'-', 2, 20}}) -- old ups1
upsert({1}, {{'+', 2, 10}}) -- new ups2

In this case execution successfully finishes and modifies old upsert's
tuple: {1, 1} + {'+', 2, 10} --> {1, 11}
However, we still have to squash/accumulate update operations since they
may be applied on tuple stored on disk later. After all, we have
following upsert: upsert({2, 11}, {{'+', 2, -10}}). Then it is applied
on the top of insert({1, 1}) and we get the same result as in the first
case - {1, -9}. The only difference is that upsert's tuple was modified.
As one can see, execution of update operations applied to upsert's tuple
is redundant in the case index already contains tuple with the same key
(i.e. when upserts turns into update). Instead, we are able to
accumulate/squash update operations only. When the last upsert is being
applied, we can either execute all update operation on tuple fetched
from index (i.e. upsert is update) OR on tuple specified in the first
upsert (i.e. first upsert is insert).

Issue #5105 (upsert doesn't follow associative property)

Secondly, current approach breaks associative property: after upsert's
update operations are merged into one array, part of them (related to
one upsert) can be skipped, meanwhile the rest - is applied. For
instance:

-- Index is over second field.
i = s:create_index('pk', {parts={2, 'uint'}})
s:replace{1, 2, 3, 'default'}
s:upsert({2, 2, 2}, {{'=', 4, 'upserted'}})
-- First update operation modifies primary key, so upsert must be ignored.
s:upsert({2, 2, 2}, {{'#', 1, 1}, {'!', 3, 1}})

After merging two upserts we get the next one:
upsert({2, 2, 2}, {{'=', 4, 'upserted'}, {'#', 1, 1}, {'!', 3, 1}}

While we executing update operations, we don't distinguish operations from
different upserts. Thus, if one operation fails, the rest are ignored
as well. As a result, first (in general case - all preceding squashed
upserts) upsert won't be applied, even despite the fact it is
absolutely correct. What is more, user gets no error/warning concerning
this fact.

Issue #1622 (no upsert result validation)

After upsert application, there's no check verifying that result
satisfies space's format: number of fields, their types, overflows etc.
Due to this tuples violating format may appear in the space, which in
turn may lead to unpredictable consequences.

To resolve these issues, let's group update operations of each upsert into
separate array. So that operations related to particular upsert are
stored in single array. In terms of previous example we will get:
upsert({2, 2, 2}, {{{'=', 4, 'upserted'}}, {{'#', 1, 1}, {'!', 3, 1}}}

Also note that we don't longer have to apply update operations on tuple
in vy_apply_upsert() when it comes for two upserts: it can be done once we
face terminal statement; or if there's no underlying statement (i.e. it is
delete statement or no statement at all) we apply all update arrays except
the first one on upsert's tuple. In case one of operations from array
fail, we skip the rest operations from this array and process to the
next array. After successful application of update operations of each
array, we check that the resulting tuple fits into space format. If they
aren't, we rollback applied operations, log error and moving to the next
group of operations.

Finally, arithmetic operations are not longer able to be combined: it is
requirement which is arises from #5105 issue.  Otherwise, result of
upserts combination might turn out to be inapplicable to the tuple
stored on disk (e.g. result applied on tuple leads to integer overflow -
in this case only last upsert leading to overflow must be ignored).

Closes #1622
Closes #5105
Closes #5092
Part of #5107
---
 src/box/vinyl.c                 |   2 +-
 src/box/vy_stmt.c               |  28 +-
 src/box/vy_stmt.h               |   5 +-
 src/box/vy_upsert.c             | 302 +++++++++++---------
 test/unit/vy_iterators_helper.c |   2 +-
 test/vinyl/upsert.result        | 473 ++++++++++++++++++++++++++++++++
 test/vinyl/upsert.test.lua      | 194 +++++++++++++
 7 files changed, 867 insertions(+), 139 deletions(-)

diff --git a/src/box/vinyl.c b/src/box/vinyl.c
index cee39c58c..9d9ee0613 100644
--- a/src/box/vinyl.c
+++ b/src/box/vinyl.c
@@ -1976,7 +1976,7 @@ vy_lsm_upsert(struct vy_tx *tx, struct vy_lsm *lsm,
 	operations[0].iov_base = (void *)expr;
 	operations[0].iov_len = expr_end - expr;
 	vystmt = vy_stmt_new_upsert(lsm->mem_format, tuple, tuple_end,
-				    operations, 1);
+				    operations, 1, false);
 	if (vystmt == NULL)
 		return -1;
 	assert(vy_stmt_type(vystmt) == IPROTO_UPSERT);
diff --git a/src/box/vy_stmt.c b/src/box/vy_stmt.c
index 92e0aa1c5..2ee82b3f2 100644
--- a/src/box/vy_stmt.c
+++ b/src/box/vy_stmt.c
@@ -313,16 +313,22 @@ vy_key_dup(const char *key)
 static struct tuple *
 vy_stmt_new_with_ops(struct tuple_format *format, const char *tuple_begin,
 		     const char *tuple_end, struct iovec *ops,
-		     int op_count, enum iproto_type type)
+		     int op_count, enum iproto_type type, bool is_ops_encoded)
 {
 	mp_tuple_assert(tuple_begin, tuple_end);
 
 	const char *tmp = tuple_begin;
 	mp_decode_array(&tmp);
 
+	/*
+	 * ops are grouped in one extra array.
+	 * See vy_apply_upsert() for details.
+	 */
 	size_t ops_size = 0;
 	for (int i = 0; i < op_count; ++i)
 		ops_size += ops[i].iov_len;
+	if (!is_ops_encoded)
+		ops_size += mp_sizeof_array(op_count);
 
 	struct tuple *stmt = NULL;
 	struct region *region = &fiber()->gc;
@@ -360,6 +366,8 @@ vy_stmt_new_with_ops(struct tuple_format *format, const char *tuple_begin,
 	field_map_build(&builder, wpos - field_map_size);
 	memcpy(wpos, tuple_begin, mpsize);
 	wpos += mpsize;
+	if (!is_ops_encoded)
+		wpos = mp_encode_array(wpos, op_count);
 	for (struct iovec *op = ops, *end = ops + op_count;
 	     op != end; ++op) {
 		memcpy(wpos, op->iov_base, op->iov_len);
@@ -374,10 +382,11 @@ end:
 struct tuple *
 vy_stmt_new_upsert(struct tuple_format *format, const char *tuple_begin,
 		   const char *tuple_end, struct iovec *operations,
-		   uint32_t ops_cnt)
+		   uint32_t ops_cnt, bool is_ops_encoded)
 {
 	return vy_stmt_new_with_ops(format, tuple_begin, tuple_end,
-				    operations, ops_cnt, IPROTO_UPSERT);
+				    operations, ops_cnt, IPROTO_UPSERT,
+				    is_ops_encoded);
 }
 
 struct tuple *
@@ -385,7 +394,7 @@ vy_stmt_new_replace(struct tuple_format *format, const char *tuple_begin,
 		    const char *tuple_end)
 {
 	return vy_stmt_new_with_ops(format, tuple_begin, tuple_end,
-				    NULL, 0, IPROTO_REPLACE);
+				    NULL, 0, IPROTO_REPLACE, true);
 }
 
 struct tuple *
@@ -393,7 +402,7 @@ vy_stmt_new_insert(struct tuple_format *format, const char *tuple_begin,
 		   const char *tuple_end)
 {
 	return vy_stmt_new_with_ops(format, tuple_begin, tuple_end,
-				    NULL, 0, IPROTO_INSERT);
+				    NULL, 0, IPROTO_INSERT, true);
 }
 
 struct tuple *
@@ -401,7 +410,7 @@ vy_stmt_new_delete(struct tuple_format *format, const char *tuple_begin,
 		   const char *tuple_end)
 {
 	return vy_stmt_new_with_ops(format, tuple_begin, tuple_end,
-				    NULL, 0, IPROTO_DELETE);
+				    NULL, 0, IPROTO_DELETE, true);
 }
 
 struct tuple *
@@ -735,19 +744,20 @@ vy_stmt_decode(struct xrow_header *xrow, struct tuple_format *format)
 		/* Always use key format for DELETE statements. */
 		stmt = vy_stmt_new_with_ops(env->key_format,
 					    request.key, request.key_end,
-					    NULL, 0, IPROTO_DELETE);
+					    NULL, 0, IPROTO_DELETE, true);
 		break;
 	case IPROTO_INSERT:
 	case IPROTO_REPLACE:
 		stmt = vy_stmt_new_with_ops(format, request.tuple,
 					    request.tuple_end,
-					    NULL, 0, request.type);
+					    NULL, 0, request.type, true);
 		break;
 	case IPROTO_UPSERT:
 		ops.iov_base = (char *)request.ops;
 		ops.iov_len = request.ops_end - request.ops;
 		stmt = vy_stmt_new_upsert(format, request.tuple,
-					  request.tuple_end, &ops, 1);
+					  request.tuple_end, &ops,
+					  1, true);
 		break;
 	default:
 		/* TODO: report filename. */
diff --git a/src/box/vy_stmt.h b/src/box/vy_stmt.h
index 25219230d..65acbecac 100644
--- a/src/box/vy_stmt.h
+++ b/src/box/vy_stmt.h
@@ -528,6 +528,8 @@ vy_stmt_new_delete(struct tuple_format *format, const char *tuple_begin,
  * @param part_count Part count from key definition.
  * @param operations Vector of update operations.
  * @param ops_cnt Length of the update operations vector.
+ * @param is_ops_encoded True, if update operations are already packed
+  *                      into extra msgpack array.
  *
  * @retval NULL     Memory allocation error.
  * @retval not NULL Success.
@@ -535,7 +537,8 @@ vy_stmt_new_delete(struct tuple_format *format, const char *tuple_begin,
 struct tuple *
 vy_stmt_new_upsert(struct tuple_format *format,
 		   const char *tuple_begin, const char *tuple_end,
-		   struct iovec *operations, uint32_t ops_cnt);
+		   struct iovec *operations, uint32_t ops_cnt,
+		   bool is_ops_encoded);
 
 /**
  * Create REPLACE statement from UPSERT statement.
diff --git a/src/box/vy_upsert.c b/src/box/vy_upsert.c
index e697b6321..2b5a16730 100644
--- a/src/box/vy_upsert.c
+++ b/src/box/vy_upsert.c
@@ -39,39 +39,150 @@
 #include "column_mask.h"
 
 /**
- * Try to squash two upsert series (msgspacked index_base + ops)
- * Try to create a tuple with squahed operations
+ * Check that key hasn't been changed after applying upsert operation.
+ */
+static bool
+vy_apply_result_does_cross_pk(struct tuple *old_stmt, const char *result,
+			      const char *result_end, struct key_def *cmp_def,
+			      uint64_t col_mask)
+{
+	if (!key_update_can_be_skipped(cmp_def->column_mask, col_mask)) {
+		struct tuple *tuple =
+			vy_stmt_new_replace(tuple_format(old_stmt), result,
+					    result_end);
+		int cmp_res = vy_stmt_compare(old_stmt, HINT_NONE, tuple,
+					      HINT_NONE, cmp_def);
+		tuple_unref(tuple);
+		return cmp_res != 0;
+	}
+	return false;
+}
+
+/**
+ * Apply update operations stored in @a upsert on tuple @a stmt. If @a stmt is
+ * void statement (i.e. it is NULL or delete statement) then operations are
+ * applied on tuple stored in @a upsert. Update operations of @a upsert which
+ * can't be applied are skipped along side with other operations from single
+ * group (i.e. packed in one msgpack array); errors may be logged depending on
+ * @a suppress_error flag.
  *
- * @retval 0 && *result_stmt != NULL : successful squash
- * @retval 0 && *result_stmt == NULL : unsquashable sources
- * @retval -1 - memory error
+ * @param upsert Upsert statement to be applied on @a stmt.
+ * @param stmt Statement to be used as base for upsert operations.
+ * @param cmp_def Key definition required to provide check of primary key
+ *                modification.
+ * @return Tuple containing result of upsert application; NULL in case OOM.
  */
-static int
-vy_upsert_try_to_squash(struct tuple_format *format,
-			const char *key_mp, const char *key_mp_end,
-			const char *old_serie, const char *old_serie_end,
-			const char *new_serie, const char *new_serie_end,
-			struct tuple **result_stmt)
+static struct tuple *
+vy_apply_upsert_on_terminal_stmt(struct tuple *upsert, struct tuple *stmt,
+				 struct key_def *cmp_def, bool suppress_error)
 {
-	*result_stmt = NULL;
+	assert(vy_stmt_type(upsert) == IPROTO_UPSERT);
+	assert(stmt == NULL || vy_stmt_type(stmt) != IPROTO_UPSERT);
 
-	size_t squashed_size;
-	const char *squashed =
-		xrow_upsert_squash(old_serie, old_serie_end,
-				   new_serie, new_serie_end, format,
-				   &squashed_size);
-	if (squashed == NULL)
-		return 0;
-	/* Successful squash! */
-	struct iovec operations[1];
-	operations[0].iov_base = (void *)squashed;
-	operations[0].iov_len = squashed_size;
+	uint32_t mp_size;
+	const char *new_ops = vy_stmt_upsert_ops(upsert, &mp_size);
+	/* Msgpack containing result of upserts application. */
+	const char *result_mp;
+	bool stmt_is_void = stmt == NULL || vy_stmt_type(stmt) == IPROTO_DELETE;
+	if (stmt_is_void)
+		result_mp = vy_upsert_data_range(upsert, &mp_size);
+	else
+		result_mp = tuple_data_range(stmt, &mp_size);
+	const char *result_mp_end = result_mp + mp_size;
+	/*
+	 * xrow_upsert_execute() allocates result using region,
+	 * so save starting point to release it later.
+	 */
+	struct region *region = &fiber()->gc;
+	size_t region_svp = region_used(region);
+	uint64_t column_mask = COLUMN_MASK_FULL;
+	struct tuple_format *format = tuple_format(upsert);
+
+	uint32_t ups_cnt = mp_decode_array(&new_ops);
+	const char *ups_ops = new_ops;
+	/*
+	 * In case upsert folds into insert, we must skip first
+	 * update operations.
+	 */
+	if (stmt_is_void) {
+		ups_cnt--;
+		mp_next(&ups_ops);
+	}
+	for (uint32_t i = 0; i < ups_cnt; ++i) {
+		assert(mp_typeof(*ups_ops) == MP_ARRAY);
+		const char *ups_ops_end = ups_ops;
+		mp_next(&ups_ops_end);
+		const char *exec_res = result_mp;
+		exec_res = xrow_upsert_execute(ups_ops, ups_ops_end, result_mp,
+					       result_mp_end, format, &mp_size,
+					       0, suppress_error, &column_mask);
+		if (exec_res == NULL) {
+			if (! suppress_error) {
+				struct error *e = diag_last_error(diag_get());
+				assert(e != NULL);
+				/* Bail out immediately in case of OOM. */
+				if (e->type != &type_ClientError) {
+					region_truncate(region, region_svp);
+					return NULL;
+				}
+				diag_log();
+			}
+			ups_ops = ups_ops_end;
+			continue;
+		}
+		/*
+		 * If it turns out that resulting tuple modifies primary
+		 * key, then simply ignore this upsert.
+		 */
+		if (stmt != NULL &&
+		    vy_apply_result_does_cross_pk(stmt, exec_res,
+						  exec_res + mp_size, cmp_def,
+						  column_mask)) {
+			if (!suppress_error) {
+				say_error("upsert operations %s are not applied"\
+					  " due to primary key modification",
+					  mp_str(ups_ops));
+			}
+			ups_ops = ups_ops_end;
+			continue;
+		}
+		ups_ops = ups_ops_end;
+		/*
+		 * Result statement must satisfy space's format. Since upsert's
+		 * tuple correctness is already checked in vy_upsert(), let's
+		 * use its format to provide result verification.
+		 */
+		struct tuple_format *format = tuple_format(upsert);
+		if (tuple_validate_raw(format, exec_res) != 0) {
+			if (! suppress_error)
+				diag_log();
+			continue;
+		}
+		result_mp = exec_res;
+		result_mp_end = exec_res + mp_size;
+	}
+	struct tuple *new_terminal_stmt = vy_stmt_new_replace(format, result_mp,
+							      result_mp_end);
+	region_truncate(region, region_svp);
+	if (new_terminal_stmt == NULL)
+		return NULL;
+	vy_stmt_set_lsn(new_terminal_stmt, vy_stmt_lsn(upsert));
+	return new_terminal_stmt;
+}
 
-	*result_stmt = vy_stmt_new_upsert(format, key_mp, key_mp_end,
-					  operations, 1);
-	if (*result_stmt == NULL)
-		return -1;
-	return 0;
+/**
+ * Unpack upsert's update operations from msgpack array
+ * into array of iovecs.
+ */
+static void
+upsert_ops_to_iovec(const char *ops, uint32_t ops_cnt, struct iovec *iov_arr)
+{
+	for (uint32_t i = 0; i < ops_cnt; ++i) {
+		assert(mp_typeof(*ops) == MP_ARRAY);
+		iov_arr[i].iov_base = (char *) ops;
+		mp_next(&ops);
+		iov_arr[i].iov_len = ops - (char *) iov_arr[i].iov_base;
+	}
 }
 
 struct tuple *
@@ -87,122 +198,59 @@ vy_apply_upsert(struct tuple *new_stmt, struct tuple *old_stmt,
 	assert(new_stmt != old_stmt);
 	assert(vy_stmt_type(new_stmt) == IPROTO_UPSERT);
 
-	if (old_stmt == NULL || vy_stmt_type(old_stmt) == IPROTO_DELETE) {
-		/*
-		 * INSERT case: return new stmt.
-		 */
-		return vy_stmt_replace_from_upsert(new_stmt);
+	struct tuple *result_stmt = NULL;
+	if (old_stmt == NULL || vy_stmt_type(old_stmt) != IPROTO_UPSERT) {
+		return vy_apply_upsert_on_terminal_stmt(new_stmt, old_stmt,
+						        cmp_def, suppress_error);
 	}
 
-	struct tuple_format *format = tuple_format(new_stmt);
-
+	assert(old_stmt != NULL);
+	assert(vy_stmt_type(old_stmt) == IPROTO_UPSERT);
 	/*
-	 * Unpack UPSERT operation from the new stmt
+	 * Unpack UPSERT operation from the old and new stmts.
 	 */
 	uint32_t mp_size;
-	const char *new_ops;
-	new_ops = vy_stmt_upsert_ops(new_stmt, &mp_size);
-	const char *new_ops_end = new_ops + mp_size;
-
+	const char *old_ops = vy_stmt_upsert_ops(old_stmt, &mp_size);
+	const char *old_stmt_mp = vy_upsert_data_range(old_stmt, &mp_size);
+	const char *old_stmt_mp_end = old_stmt_mp + mp_size;
+	const char *new_ops = vy_stmt_upsert_ops(new_stmt, &mp_size);
 	/*
-	 * Apply new operations to the old stmt
+	 * UPSERT + UPSERT case: unpack operations to iovec array and merge
+	 * them into one ops array.
 	 */
-	const char *result_mp;
-	if (vy_stmt_type(old_stmt) == IPROTO_UPSERT)
-		result_mp = vy_upsert_data_range(old_stmt, &mp_size);
-	else
-		result_mp = tuple_data_range(old_stmt, &mp_size);
-	const char *result_mp_end = result_mp + mp_size;
-	struct tuple *result_stmt = NULL;
+	struct tuple_format *format = tuple_format(old_stmt);
 	struct region *region = &fiber()->gc;
 	size_t region_svp = region_used(region);
-	uint8_t old_type = vy_stmt_type(old_stmt);
-	uint64_t column_mask = COLUMN_MASK_FULL;
-	result_mp = xrow_upsert_execute(new_ops, new_ops_end, result_mp,
-					result_mp_end, format, &mp_size,
-					0, suppress_error, &column_mask);
-	if (result_mp == NULL) {
+	uint32_t old_ops_cnt = mp_decode_array(&old_ops);
+	uint32_t new_ops_cnt = mp_decode_array(&new_ops);
+	size_t ops_size;
+	struct iovec *operations =
+		region_alloc_array(region, typeof(operations[0]),
+				   old_ops_cnt + new_ops_cnt, &ops_size);
+	if (operations == NULL) {
 		region_truncate(region, region_svp);
+		diag_set(OutOfMemory, ops_size, "region_alloc_array",
+			 "operations");
 		return NULL;
 	}
-	result_mp_end = result_mp + mp_size;
-	if (old_type != IPROTO_UPSERT) {
-		assert(old_type == IPROTO_INSERT ||
-		       old_type == IPROTO_REPLACE);
-		/*
-		 * UPDATE case: return the updated old stmt.
-		 */
-		result_stmt = vy_stmt_new_replace(format, result_mp,
-						  result_mp_end);
-		region_truncate(region, region_svp);
-		if (result_stmt == NULL)
-			return NULL; /* OOM */
-		vy_stmt_set_lsn(result_stmt, vy_stmt_lsn(new_stmt));
-		goto check_key;
-	}
-
-	/*
-	 * Unpack UPSERT operation from the old stmt
-	 */
-	assert(old_stmt != NULL);
-	const char *old_ops;
-	old_ops = vy_stmt_upsert_ops(old_stmt, &mp_size);
-	const char *old_ops_end = old_ops + mp_size;
-	assert(old_ops_end > old_ops);
-
+	upsert_ops_to_iovec(old_ops, old_ops_cnt, operations);
+	upsert_ops_to_iovec(new_ops, new_ops_cnt, &operations[old_ops_cnt]);
 	/*
-	 * UPSERT + UPSERT case: combine operations
+	 * Adding update operations. We keep order of update operations in
+	 * the array the same. It is vital since first set of operations
+	 * must be skipped in case upsert folds into insert. For instance:
+	 * old_ops = {{{op1}, {op2}}, {{op3}}}
+	 * new_ops = {{{op4}, {op5}}}
+	 * res_ops = {{{op1}, {op2}}, {{op3}}, {{op4}, {op5}}}
+	 * If upsert corresponding to old_ops becomes insert, then
+	 * {{op1}, {op2}} update operations are not applied.
 	 */
-	assert(old_ops_end - old_ops > 0);
-	if (vy_upsert_try_to_squash(format, result_mp, result_mp_end,
-				    old_ops, old_ops_end, new_ops, new_ops_end,
-				    &result_stmt) != 0) {
-		region_truncate(region, region_svp);
-		return NULL;
-	}
-	if (result_stmt != NULL) {
-		region_truncate(region, region_svp);
-		vy_stmt_set_lsn(result_stmt, vy_stmt_lsn(new_stmt));
-		goto check_key;
-	}
-
-	/* Failed to squash, simply add one upsert to another */
-	int old_ops_cnt, new_ops_cnt;
-	struct iovec operations[3];
-
-	old_ops_cnt = mp_decode_array(&old_ops);
-	operations[1].iov_base = (void *)old_ops;
-	operations[1].iov_len = old_ops_end - old_ops;
-
-	new_ops_cnt = mp_decode_array(&new_ops);
-	operations[2].iov_base = (void *)new_ops;
-	operations[2].iov_len = new_ops_end - new_ops;
-
-	char ops_buf[16];
-	char *header = mp_encode_array(ops_buf, old_ops_cnt + new_ops_cnt);
-	operations[0].iov_base = (void *)ops_buf;
-	operations[0].iov_len = header - ops_buf;
-
-	result_stmt = vy_stmt_new_upsert(format, result_mp, result_mp_end,
-					 operations, 3);
+	result_stmt = vy_stmt_new_upsert(format, old_stmt_mp, old_stmt_mp_end,
+					 operations, old_ops_cnt + new_ops_cnt,
+					 false);
 	region_truncate(region, region_svp);
 	if (result_stmt == NULL)
 		return NULL;
 	vy_stmt_set_lsn(result_stmt, vy_stmt_lsn(new_stmt));
-
-check_key:
-	/*
-	 * Check that key hasn't been changed after applying operations.
-	 */
-	if (!key_update_can_be_skipped(cmp_def->column_mask, column_mask) &&
-	    vy_stmt_compare(old_stmt, HINT_NONE, result_stmt,
-			    HINT_NONE, cmp_def) != 0) {
-		/*
-		 * Key has been changed: ignore this UPSERT and
-		 * @retval the old stmt.
-		 */
-		tuple_unref(result_stmt);
-		result_stmt = vy_stmt_dup(old_stmt);
-	}
 	return result_stmt;
 }
diff --git a/test/unit/vy_iterators_helper.c b/test/unit/vy_iterators_helper.c
index 0d20f19ef..15470920b 100644
--- a/test/unit/vy_iterators_helper.c
+++ b/test/unit/vy_iterators_helper.c
@@ -112,7 +112,7 @@ vy_new_simple_stmt(struct tuple_format *format, struct key_def *key_def,
 			ops = mp_encode_int(ops, templ->upsert_value);
 		operations[0].iov_base = tmp;
 		operations[0].iov_len = ops - tmp;
-		ret = vy_stmt_new_upsert(format, buf, pos, operations, 1);
+		ret = vy_stmt_new_upsert(format, buf, pos, operations, 1, true);
 		fail_if(ret == NULL);
 		break;
 	}
diff --git a/test/vinyl/upsert.result b/test/vinyl/upsert.result
index 3a7f6629d..a66e87be0 100644
--- a/test/vinyl/upsert.result
+++ b/test/vinyl/upsert.result
@@ -899,3 +899,476 @@ s:select()
 s:drop()
 ---
 ...
+-- gh-5107: don't squash upsert operations into one array.
+--
+-- gh-5087: test upsert execution/squash referring to fields in reversed
+-- order (via negative indexing).
+--
+s = box.schema.create_space('test', {engine = 'vinyl'})
+---
+...
+pk = s:create_index('pk')
+---
+...
+s:insert({1, 1, 1})
+---
+- [1, 1, 1]
+...
+box.snapshot()
+---
+- ok
+...
+s:upsert({1}, {{'=', 3, 100}})
+---
+...
+s:upsert({1}, {{'=', -1, 200}})
+---
+...
+box.snapshot()
+---
+- ok
+...
+s:select() -- {1, 1, 200}
+---
+- - [1, 1, 200]
+...
+s:delete({1})
+---
+...
+s:insert({1, 1, 1})
+---
+- [1, 1, 1]
+...
+box.snapshot()
+---
+- ok
+...
+s:upsert({1}, {{'=', -3, 100}})
+---
+...
+s:upsert({1}, {{'=', -1, 200}})
+---
+...
+box.snapshot()
+---
+- ok
+...
+-- gh-5105: Two upserts are NOT squashed into one, so only one (first one)
+-- is skipped, meanwhile second one is applied.
+--
+s:select() -- {1, 1, 1}
+---
+- - [1, 1, 200]
+...
+s:delete({1})
+---
+...
+box.snapshot()
+---
+- ok
+...
+s:upsert({1, 1}, {{'=', -2, 300}}) -- {1, 1}
+---
+...
+s:upsert({1}, {{'+', -1, 100}}) -- {1, 101}
+---
+...
+s:upsert({1}, {{'-', 2, 100}}) -- {1, 1}
+---
+...
+s:upsert({1}, {{'+', -1, 200}}) -- {1, 201}
+---
+...
+s:upsert({1}, {{'-', 2, 200}}) -- {1, 1}
+---
+...
+box.snapshot()
+---
+- ok
+...
+s:select() -- {1, 1}
+---
+- - [1, 1]
+...
+s:delete({1})
+---
+...
+box.snapshot()
+---
+- ok
+...
+s:upsert({1, 1, 1}, {{'!', -1, 300}}) -- {1, 1, 1}
+---
+...
+s:upsert({1}, {{'+', -2, 100}}) -- {1, 101, 1}
+---
+...
+s:upsert({1}, {{'=', -1, 100}}) -- {1, 101, 100}
+---
+...
+s:upsert({1}, {{'+', -1, 200}}) -- {1, 101, 300}
+---
+...
+s:upsert({1}, {{'-', -2, 100}}) -- {1, 1, 300}
+---
+...
+box.snapshot()
+---
+- ok
+...
+s:select()
+---
+- - [1, 1, 300]
+...
+s:drop()
+---
+...
+-- gh-1622: upsert operations which break space format are not applied.
+--
+s = box.schema.space.create('test', { engine = 'vinyl', field_count = 2 })
+---
+...
+pk = s:create_index('pk')
+---
+...
+s:replace{1, 1}
+---
+- [1, 1]
+...
+-- Error is logged, upsert is not applied.
+--
+s:upsert({1, 1}, {{'=', 3, 5}})
+---
+...
+-- During read the incorrect upsert is ignored.
+--
+s:select{}
+---
+- - [1, 1]
+...
+-- Try to set incorrect field_count in a transaction.
+--
+box.begin()
+---
+...
+s:replace{2, 2}
+---
+- [2, 2]
+...
+s:upsert({2, 2}, {{'=', 3, 2}})
+---
+...
+s:select{}
+---
+- - [1, 1]
+  - [2, 2]
+...
+box.commit()
+---
+...
+s:select{}
+---
+- - [1, 1]
+  - [2, 2]
+...
+-- Read incorrect upsert from a run: it should be ignored.
+--
+box.snapshot()
+---
+- ok
+...
+s:select{}
+---
+- - [1, 1]
+  - [2, 2]
+...
+s:upsert({2, 2}, {{'=', 3, 20}})
+---
+...
+box.snapshot()
+---
+- ok
+...
+s:select{}
+---
+- - [1, 1]
+  - [2, 2]
+...
+-- Execute replace/delete after invalid upsert.
+--
+box.snapshot()
+---
+- ok
+...
+s:upsert({2, 2}, {{'=', 3, 30}})
+---
+...
+s:replace{2, 3}
+---
+- [2, 3]
+...
+s:select{}
+---
+- - [1, 1]
+  - [2, 3]
+...
+s:upsert({1, 1}, {{'=', 3, 30}})
+---
+...
+s:delete{1}
+---
+...
+s:select{}
+---
+- - [2, 3]
+...
+-- Invalid upsert in a sequence of upserts is skipped meanwhile
+-- the rest are applied.
+--
+box.snapshot()
+---
+- ok
+...
+s:upsert({2, 2}, {{'+', 2, 5}})
+---
+...
+s:upsert({2, 2}, {{'=', 3, 40}})
+---
+...
+s:upsert({2, 2}, {{'+', 2, 5}})
+---
+...
+s:select{}
+---
+- - [2, 13]
+...
+box.snapshot()
+---
+- ok
+...
+s:select{}
+---
+- - [2, 13]
+...
+s:drop()
+---
+...
+-- Test different scenarious during which update operations squash can't
+-- take place due to format violations.
+--
+decimal = require('decimal')
+---
+...
+s = box.schema.space.create('test', { engine = 'vinyl', field_count = 5 })
+---
+...
+s:format({{name='id', type='unsigned'}, {name='u', type='unsigned'},\
+          {name='s', type='scalar'}, {name='f', type='double'},\
+          {name='d', type='decimal'}})
+---
+...
+pk = s:create_index('pk')
+---
+...
+s:replace{1, 1, 1, 1.1, decimal.new(1.1) }
+---
+- [1, 1, 1, 1.1, 1.1]
+...
+s:replace{2, 1, 1, 1.1, decimal.new(1.1)}
+---
+- [2, 1, 1, 1.1, 1.1]
+...
+box.snapshot()
+---
+- ok
+...
+-- Can't assign integer to float field. First operation is still applied.
+--
+s:upsert({1, 1, 1, 2.5, decimal.new(1.1)}, {{'+', 4, 4}})
+---
+...
+s:upsert({1, 1, 1, 2.5, decimal.new(1.1)}, {{'=', 4, 4}})
+---
+...
+-- Can't add floating point to integer (result is floating point).
+--
+s:upsert({2, 1, 1, 2.5, decimal.new(1.1)}, {{'+', 2, 5}})
+---
+...
+s:upsert({2, 1, 1, 2.5, decimal.new(1.1)}, {{'+', 2, 5.5}})
+---
+...
+box.snapshot()
+---
+- ok
+...
+s:select()
+---
+- - [1, 1, 1, 5.1, 1.1]
+  - [2, 6, 1, 1.1, 1.1]
+...
+-- Integer overflow check.
+--
+s:upsert({1, 1, 1, 2.5, decimal.new(1.1)}, {{'+', 3, 9223372036854775808}})
+---
+...
+s:upsert({1, 1, 1, 2.5, decimal.new(1.1)}, {{'+', 3, 9223372036854775808}})
+---
+...
+-- Negative result of subtraction stored in unsigned field.
+--
+s:upsert({2, 1, 1, 2.5, decimal.new(1.1)}, {{'+', 2, 2}})
+---
+...
+s:upsert({2, 1, 1, 2.5, decimal.new(1.1)}, {{'-', 2, 10}})
+---
+...
+box.snapshot()
+---
+- ok
+...
+s:select()
+---
+- - [1, 1, 9223372036854775809, 5.1, 1.1]
+  - [2, 8, 1, 1.1, 1.1]
+...
+-- Decimals do not fit into numerics and vice versa.
+--
+s:upsert({1, 1, 1, 2.5, decimal.new(1.1)}, {{'+', 5, 2}})
+---
+...
+s:upsert({1, 1, 1, 2.5, decimal.new(1.1)}, {{'-', 5, 1}})
+---
+...
+s:upsert({2, 1, 1, 2.5, decimal.new(1.1)}, {{'+', 2, decimal.new(2.1)}})
+---
+...
+s:upsert({2, 1, 1, 2.5, decimal.new(1.1)}, {{'-', 2, decimal.new(1.2)}})
+---
+...
+box.snapshot()
+---
+- ok
+...
+s:select()
+---
+- - [1, 1, 9223372036854775809, 5.1, 2.1]
+  - [2, 8, 1, 1.1, 1.1]
+...
+s:drop()
+---
+...
+-- Upserts leading to overflow are ignored.
+--
+format = {}
+---
+...
+format[1] = {name = 'f1', type = 'unsigned'}
+---
+...
+format[2] = {name = 'f2', type = 'unsigned'}
+---
+...
+s = box.schema.space.create('test', {engine = 'vinyl', format = format})
+---
+...
+_ = s:create_index('pk')
+---
+...
+uint_max = 18446744073709551615ULL
+---
+...
+s:replace{1, uint_max - 2}
+---
+- [1, 18446744073709551613]
+...
+box.snapshot()
+---
+- ok
+...
+s:upsert({1, 0}, {{'+', 2, 1}})
+---
+...
+s:upsert({1, 0}, {{'+', 2, 1}})
+---
+...
+s:upsert({1, 0}, {{'+', 2, 1}})
+---
+...
+box.snapshot()
+---
+- ok
+...
+s:select()
+---
+- - [1, 18446744073709551615]
+...
+s:delete{1}
+---
+...
+s:replace{1, uint_max - 2, 0}
+---
+- [1, 18446744073709551613, 0]
+...
+box.snapshot()
+---
+- ok
+...
+s:upsert({1, 0, 0}, {{'+', 2, 1}})
+---
+...
+s:upsert({1, 0, 0}, {{'+', 2, 1}})
+---
+...
+s:upsert({1, 0, 0}, {{'+', 2, 0.5}})
+---
+...
+s:upsert({1, 0, 0}, {{'+', 2, 1}})
+---
+...
+box.snapshot()
+---
+- ok
+...
+s:select()
+---
+- - [1, 18446744073709551615, 0]
+...
+s:drop()
+---
+...
+-- Make sure upserts satisfy associativity rule.
+--
+s = box.schema.space.create('test', {engine='vinyl'})
+---
+...
+i = s:create_index('pk', {parts={2, 'uint'}})
+---
+...
+s:replace{1, 2, 3, 'default'}
+---
+- [1, 2, 3, 'default']
+...
+box.snapshot()
+---
+- ok
+...
+s:upsert({2, 2, 2}, {{'=', 4, 'upserted'}})
+---
+...
+-- Upsert will fail and thus ignored.
+--
+s:upsert({2, 2, 2}, {{'#', 1, 1}, {'!', 3, 1}})
+---
+...
+box.snapshot()
+---
+- ok
+...
+s:select{}
+---
+- - [1, 2, 3, 'upserted']
+...
+s:drop()
+---
+...
diff --git a/test/vinyl/upsert.test.lua b/test/vinyl/upsert.test.lua
index 1d77474da..ad69f4b72 100644
--- a/test/vinyl/upsert.test.lua
+++ b/test/vinyl/upsert.test.lua
@@ -372,3 +372,197 @@ box.snapshot()
 s:select()
 
 s:drop()
+
+-- gh-5107: don't squash upsert operations into one array.
+--
+-- gh-5087: test upsert execution/squash referring to fields in reversed
+-- order (via negative indexing).
+--
+s = box.schema.create_space('test', {engine = 'vinyl'})
+pk = s:create_index('pk')
+s:insert({1, 1, 1})
+box.snapshot()
+
+s:upsert({1}, {{'=', 3, 100}})
+s:upsert({1}, {{'=', -1, 200}})
+box.snapshot()
+s:select() -- {1, 1, 200}
+
+s:delete({1})
+s:insert({1, 1, 1})
+box.snapshot()
+
+s:upsert({1}, {{'=', -3, 100}})
+s:upsert({1}, {{'=', -1, 200}})
+box.snapshot()
+-- gh-5105: Two upserts are NOT squashed into one, so only one (first one)
+-- is skipped, meanwhile second one is applied.
+--
+s:select() -- {1, 1, 1}
+
+s:delete({1})
+box.snapshot()
+
+s:upsert({1, 1}, {{'=', -2, 300}}) -- {1, 1}
+s:upsert({1}, {{'+', -1, 100}}) -- {1, 101}
+s:upsert({1}, {{'-', 2, 100}}) -- {1, 1}
+s:upsert({1}, {{'+', -1, 200}}) -- {1, 201}
+s:upsert({1}, {{'-', 2, 200}}) -- {1, 1}
+box.snapshot()
+s:select() -- {1, 1}
+
+s:delete({1})
+box.snapshot()
+
+s:upsert({1, 1, 1}, {{'!', -1, 300}}) -- {1, 1, 1}
+s:upsert({1}, {{'+', -2, 100}}) -- {1, 101, 1}
+s:upsert({1}, {{'=', -1, 100}}) -- {1, 101, 100}
+s:upsert({1}, {{'+', -1, 200}}) -- {1, 101, 300}
+s:upsert({1}, {{'-', -2, 100}}) -- {1, 1, 300}
+box.snapshot()
+s:select()
+
+s:drop()
+
+-- gh-1622: upsert operations which break space format are not applied.
+--
+s = box.schema.space.create('test', { engine = 'vinyl', field_count = 2 })
+pk = s:create_index('pk')
+s:replace{1, 1}
+-- Error is logged, upsert is not applied.
+--
+s:upsert({1, 1}, {{'=', 3, 5}})
+-- During read the incorrect upsert is ignored.
+--
+s:select{}
+
+-- Try to set incorrect field_count in a transaction.
+--
+box.begin()
+s:replace{2, 2}
+s:upsert({2, 2}, {{'=', 3, 2}})
+s:select{}
+box.commit()
+s:select{}
+
+-- Read incorrect upsert from a run: it should be ignored.
+--
+box.snapshot()
+s:select{}
+s:upsert({2, 2}, {{'=', 3, 20}})
+box.snapshot()
+s:select{}
+
+-- Execute replace/delete after invalid upsert.
+--
+box.snapshot()
+s:upsert({2, 2}, {{'=', 3, 30}})
+s:replace{2, 3}
+s:select{}
+
+s:upsert({1, 1}, {{'=', 3, 30}})
+s:delete{1}
+s:select{}
+
+-- Invalid upsert in a sequence of upserts is skipped meanwhile
+-- the rest are applied.
+--
+box.snapshot()
+s:upsert({2, 2}, {{'+', 2, 5}})
+s:upsert({2, 2}, {{'=', 3, 40}})
+s:upsert({2, 2}, {{'+', 2, 5}})
+s:select{}
+box.snapshot()
+s:select{}
+
+s:drop()
+
+-- Test different scenarious during which update operations squash can't
+-- take place due to format violations.
+--
+decimal = require('decimal')
+
+s = box.schema.space.create('test', { engine = 'vinyl', field_count = 5 })
+s:format({{name='id', type='unsigned'}, {name='u', type='unsigned'},\
+          {name='s', type='scalar'}, {name='f', type='double'},\
+          {name='d', type='decimal'}})
+pk = s:create_index('pk')
+s:replace{1, 1, 1, 1.1, decimal.new(1.1) }
+s:replace{2, 1, 1, 1.1, decimal.new(1.1)}
+box.snapshot()
+-- Can't assign integer to float field. First operation is still applied.
+--
+s:upsert({1, 1, 1, 2.5, decimal.new(1.1)}, {{'+', 4, 4}})
+s:upsert({1, 1, 1, 2.5, decimal.new(1.1)}, {{'=', 4, 4}})
+-- Can't add floating point to integer (result is floating point).
+--
+s:upsert({2, 1, 1, 2.5, decimal.new(1.1)}, {{'+', 2, 5}})
+s:upsert({2, 1, 1, 2.5, decimal.new(1.1)}, {{'+', 2, 5.5}})
+box.snapshot()
+s:select()
+-- Integer overflow check.
+--
+s:upsert({1, 1, 1, 2.5, decimal.new(1.1)}, {{'+', 3, 9223372036854775808}})
+s:upsert({1, 1, 1, 2.5, decimal.new(1.1)}, {{'+', 3, 9223372036854775808}})
+-- Negative result of subtraction stored in unsigned field.
+--
+s:upsert({2, 1, 1, 2.5, decimal.new(1.1)}, {{'+', 2, 2}})
+s:upsert({2, 1, 1, 2.5, decimal.new(1.1)}, {{'-', 2, 10}})
+box.snapshot()
+s:select()
+-- Decimals do not fit into numerics and vice versa.
+--
+s:upsert({1, 1, 1, 2.5, decimal.new(1.1)}, {{'+', 5, 2}})
+s:upsert({1, 1, 1, 2.5, decimal.new(1.1)}, {{'-', 5, 1}})
+s:upsert({2, 1, 1, 2.5, decimal.new(1.1)}, {{'+', 2, decimal.new(2.1)}})
+s:upsert({2, 1, 1, 2.5, decimal.new(1.1)}, {{'-', 2, decimal.new(1.2)}})
+box.snapshot()
+s:select()
+
+s:drop()
+
+-- Upserts leading to overflow are ignored.
+--
+format = {}
+format[1] = {name = 'f1', type = 'unsigned'}
+format[2] = {name = 'f2', type = 'unsigned'}
+s = box.schema.space.create('test', {engine = 'vinyl', format = format})
+_ = s:create_index('pk')
+uint_max = 18446744073709551615ULL
+s:replace{1, uint_max - 2}
+box.snapshot()
+
+s:upsert({1, 0}, {{'+', 2, 1}})
+s:upsert({1, 0}, {{'+', 2, 1}})
+s:upsert({1, 0}, {{'+', 2, 1}})
+box.snapshot()
+s:select()
+
+s:delete{1}
+s:replace{1, uint_max - 2, 0}
+box.snapshot()
+
+s:upsert({1, 0, 0}, {{'+', 2, 1}})
+s:upsert({1, 0, 0}, {{'+', 2, 1}})
+s:upsert({1, 0, 0}, {{'+', 2, 0.5}})
+s:upsert({1, 0, 0}, {{'+', 2, 1}})
+box.snapshot()
+s:select()
+s:drop()
+
+-- Make sure upserts satisfy associativity rule.
+--
+s = box.schema.space.create('test', {engine='vinyl'})
+i = s:create_index('pk', {parts={2, 'uint'}})
+s:replace{1, 2, 3, 'default'}
+box.snapshot()
+
+s:upsert({2, 2, 2}, {{'=', 4, 'upserted'}})
+-- Upsert will fail and thus ignored.
+--
+s:upsert({2, 2, 2}, {{'#', 1, 1}, {'!', 3, 1}})
+box.snapshot()
+
+s:select{}
+
+s:drop()
-- 
2.17.1

[Tarantool-patches] [PATCH v3 2/2] vinyl: remove squash procedures from source code

[Nikita Pettik]

After previous commit, there's no need in these functions. However,
someday we may want to reconsider it squash optimization and make it
work without breaking upsert associativity rule. So to not lost initial
squash implementation, let's put its removal in a separate patch.

Follow-up #5107
---
 src/box/xrow_update.c | 144 ------------------------------------------
 src/box/xrow_update.h |  14 ----
 2 files changed, 158 deletions(-)

diff --git a/src/box/xrow_update.c b/src/box/xrow_update.c
index 833975f03..cba182ab2 100644
--- a/src/box/xrow_update.c
+++ b/src/box/xrow_update.c
@@ -415,147 +415,3 @@ xrow_upsert_execute(const char *expr,const char *expr_end,
 
 	return xrow_update_finish(&update, format, p_tuple_len);
 }
-
-const char *
-xrow_upsert_squash(const char *expr1, const char *expr1_end,
-		   const char *expr2, const char *expr2_end,
-		   struct tuple_format *format, size_t *result_size)
-{
-	const char *expr[2] = {expr1, expr2};
-	const char *expr_end[2] = {expr1_end, expr2_end};
-	struct xrow_update update[2];
-	struct tuple_dictionary *dict = format->dict;
-	for (int j = 0; j < 2; j++) {
-		xrow_update_init(&update[j], 0);
-		if (xrow_update_read_ops(&update[j], expr[j], expr_end[j],
-					 dict, 0) != 0)
-			return NULL;
-		mp_decode_array(&expr[j]);
-		int32_t prev_field_no = -1;
-		for (uint32_t i = 0; i < update[j].op_count; i++) {
-			struct xrow_update_op *op = &update[j].ops[i];
-			if (op->opcode != '+' && op->opcode != '-' &&
-			    op->opcode != '=')
-				return NULL;
-			/*
-			 * Not terminal operation means, that the
-			 * update is not flat, and squash would
-			 * need to build a tree of operations to
-			 * find matches. That is too complex,
-			 * squash is skipped.
-			 */
-			if (!xrow_update_op_is_term(op))
-				return NULL;
-			if (op->field_no <= prev_field_no)
-				return NULL;
-			prev_field_no = op->field_no;
-		}
-	}
-	size_t possible_size = expr1_end - expr1 + expr2_end - expr2;
-	const uint32_t space_for_arr_tag = 5;
-	char *buf = (char *) region_alloc(&fiber()->gc,
-					  possible_size + space_for_arr_tag);
-	if (buf == NULL) {
-		diag_set(OutOfMemory, possible_size + space_for_arr_tag,
-			 "region_alloc", "buf");
-		return NULL;
-	}
-	/* reserve some space for mp array header */
-	char *res_ops = buf + space_for_arr_tag;
-	uint32_t res_count = 0; /* number of resulting operations */
-
-	uint32_t op_count[2] = {update[0].op_count, update[1].op_count};
-	uint32_t op_no[2] = {0, 0};
-	struct json_tree *format_tree = &format->fields;
-	struct json_token *root = &format_tree->root;
-	struct json_token token;
-	token.type = JSON_TOKEN_NUM;
-	while (op_no[0] < op_count[0] || op_no[1] < op_count[1]) {
-		res_count++;
-		struct xrow_update_op *op[2] = {update[0].ops + op_no[0],
-						update[1].ops + op_no[1]};
-		/*
-		 * from:
-		 * 0 - take op from first update,
-		 * 1 - take op from second update,
-		 * 2 - merge both ops
-		 */
-		uint32_t from;
-		uint32_t has[2] = {op_no[0] < op_count[0], op_no[1] < op_count[1]};
-		assert(has[0] || has[1]);
-		if (has[0] && has[1]) {
-			from = op[0]->field_no < op[1]->field_no ? 0 :
-			       op[0]->field_no > op[1]->field_no ? 1 : 2;
-		} else {
-			assert(has[0] != has[1]);
-			from = has[1];
-		}
-		if (from == 2 && op[1]->opcode == '=') {
-			/*
-			 * If an operation from the second upsert is '='
-			 * it is just overwrites any op from the first upsert.
-			 * So we just skip op from the first upsert and
-			 * copy op from the second
-			 */
-			mp_next(&expr[0]);
-			op_no[0]++;
-			from = 1;
-		}
-		if (from < 2) {
-			/* take op from one of upserts */
-			const char *copy = expr[from];
-			mp_next(&expr[from]);
-			size_t copy_size = expr[from] - copy;
-			memcpy(res_ops, copy, copy_size);
-			res_ops += copy_size;
-			op_no[from]++;
-			continue;
-		}
-		/* merge: apply second '+' or '-' */
-		assert(op[1]->opcode == '+' || op[1]->opcode == '-');
-		if (op[0]->opcode == '-') {
-			op[0]->opcode = '+';
-			int96_invert(&op[0]->arg.arith.int96);
-		}
-		struct xrow_update_arg_arith arith;
-		/*
-		 * If we apply + or - on top of the set operation ('='),
-		 * then resulting operation is {'=', op1.val + op2.val}.
-		 * To continue processing arithmetic operation we should
-		 * firstly extract value from set operation holder.
-		 */
-		if (op[0]->opcode == '=') {
-			if (xrow_mp_read_arg_arith(op[0], &op[0]->arg.set.value,
-					           &arith) != 0)
-				return NULL;
-		} else {
-			arith = op[0]->arg.arith;
-		}
-		struct xrow_update_op res;
-		if (xrow_update_arith_make(op[1], arith,
-					   &res.arg.arith) != 0)
-			return NULL;
-		res_ops = mp_encode_array(res_ops, 3);
-		res_ops = mp_encode_str(res_ops,
-					(const char *)&op[0]->opcode, 1);
-		token.num = op[0]->field_no;
-		res_ops = mp_encode_uint(res_ops, token.num +
-					 update[0].index_base);
-		struct json_token *this_node =
-			json_tree_lookup(format_tree, root, &token);
-		xrow_update_op_store_arith(&res, format_tree, this_node, NULL,
-					   res_ops);
-		res_ops += xrow_update_arg_arith_sizeof(&res.arg.arith);
-		mp_next(&expr[0]);
-		mp_next(&expr[1]);
-		op_no[0]++;
-		op_no[1]++;
-	}
-	assert(op_no[0] == op_count[0] && op_no[1] == op_count[1]);
-	assert(expr[0] == expr_end[0] && expr[1] == expr_end[1]);
-	char *arr_start = buf + space_for_arr_tag -
-		mp_sizeof_array(res_count);
-	mp_encode_array(arr_start, res_count);
-	*result_size = res_ops - arr_start;
-	return arr_start;
-}
diff --git a/src/box/xrow_update.h b/src/box/xrow_update.h
index ba4abb1b0..64b4cd034 100644
--- a/src/box/xrow_update.h
+++ b/src/box/xrow_update.h
@@ -63,20 +63,6 @@ xrow_upsert_execute(const char *expr, const char *expr_end,
 		    int index_base, bool suppress_error,
 		    uint64_t *column_mask);
 
-/**
- * Try to merge two update/upsert expressions to an equivalent one.
- * Due to optimization reasons resulting expression
- * is located inside a bigger allocation. There also some hidden
- * internal allocations are made in this function.
- * Thus the only allocator that can be used in this function
- * is region allocator.
- * If it isn't possible to merge expressions NULL is returned.
- */
-const char *
-xrow_upsert_squash(const char *expr1, const char *expr1_end,
-		   const char *expr2, const char *expr2_end,
-		   struct tuple_format *format, size_t *result_size);
-
 #if defined(__cplusplus)
 } /* extern "C" */
 #endif /* defined(__cplusplus) */
-- 
2.17.1

Re: [Tarantool-patches] [PATCH v3 0/2] vinyl: rework upsert operation

[Nikita Pettik]

On 03 Oct 16:28, Nikita Pettik wrote:
> Issues:
> https://github.com/tarantool/tarantool/issues/1622
> https://github.com/tarantool/tarantool/issues/5105
> https://github.com/tarantool/tarantool/issues/5092
> https://github.com/tarantool/tarantool/issues/5107
> Branch:
> https://github.com/tarantool/tarantool/tree/np/gh-5107-dont-squash-ops

Sorry guys, the real branch is https://github.com/tarantool/tarantool/tree/np/gh-5107-dont-squash-and-merge-ops
 
> @ChangeLog:
>  - Rework upsert operation in vinyl so that now (gh-5107):
>    - if upsert can't be applied it is skipped and corresponding error is logged (gh-1622);
>    - upserts now follow associative property: result of several upserts
>      doesn't depend on the order of their application (gh-5105);
>    - upserts referring to -1 fieldno are handled correctly now (gh-5087).
>    - there's no more upserts squash procedure: upserts referring to the
>      same field with arithmetic operations are not merged into one 
>      operation since resulting upsert might not be applied - as a result
>      both upserts would be ignored (meanwhile only one should be).
> 
> Changes in v3:
>  - Removed upsert squashing procedure;
>  - Added two additional test covering overflow error during upsert
>    application;
>  - Fixed NULL dereference bug in vy_apply_upsert_on_terminal_stmt().
> 
> Nikita Pettik (2):
>   vinyl: rework upsert operation
>   vinyl: remove squash procedures from source code
> 
>  src/box/vinyl.c                 |   2 +-
>  src/box/vy_stmt.c               |  28 +-
>  src/box/vy_stmt.h               |   5 +-
>  src/box/vy_upsert.c             | 302 +++++++++++---------
>  src/box/xrow_update.c           | 144 ----------
>  src/box/xrow_update.h           |  14 -
>  test/unit/vy_iterators_helper.c |   2 +-
>  test/vinyl/upsert.result        | 473 ++++++++++++++++++++++++++++++++
>  test/vinyl/upsert.test.lua      | 194 +++++++++++++
>  9 files changed, 867 insertions(+), 297 deletions(-)
> 
> -- 
> 2.17.1
> 

Re: [Tarantool-patches] [PATCH v3 0/2] vinyl: rework upsert operation

[Vladislav Shpilevoy]

Hi! Thanks for the patchset!

Does this patchset change format of vinyl statements on disk? If yes, I guess
it breaks backward compatibility. Did you check how does it behave when booted
from files of a previous version? Does it crash? Does it report an error? How
a user can upgrade his vinyl files to the new format?

Re: [Tarantool-patches] [PATCH v3 1/2] vinyl: rework upsert operation

[Vladislav Shpilevoy]

Thanks for the patch!

See 3 comments below, and my fixes on top of this branch in a
separate commit.

>  src/box/vinyl.c                 |   2 +-
>  src/box/vy_stmt.c               |  28 +-
>  src/box/vy_stmt.h               |   5 +-
>  src/box/vy_upsert.c             | 302 +++++++++++---------
>  test/unit/vy_iterators_helper.c |   2 +-
>  test/vinyl/upsert.result        | 473 ++++++++++++++++++++++++++++++++
>  test/vinyl/upsert.test.lua      | 194 +++++++++++++
>  7 files changed, 867 insertions(+), 139 deletions(-)
> 
> diff --git a/src/box/vinyl.c b/src/box/vinyl.c
> index cee39c58c..9d9ee0613 100644
> --- a/src/box/vinyl.c
> +++ b/src/box/vinyl.c
> @@ -1976,7 +1976,7 @@ vy_lsm_upsert(struct vy_tx *tx, struct vy_lsm *lsm,
>  	operations[0].iov_base = (void *)expr;
>  	operations[0].iov_len = expr_end - expr;
>  	vystmt = vy_stmt_new_upsert(lsm->mem_format, tuple, tuple_end,
> -				    operations, 1);
> +				    operations, 1, false);

1. You don't really need that flag. In the previous review I tried to
explain it, but now I added a commit on top of your branch where I
removed this parameter. Take a look.

This argument does not have much sense. First of all, the operations array
is not really operations. Iovecs here can be separate operations; it can be
a single iovec with all operations; it can be several iovecs with first
having additional MP_ARRAY encoded before a first operation. So these are
just pieces of raw data. And you can use that. You can push a header of
these operations as a part of 'operations' array always, like I did in my
commit. No need to pass a new argument for that.

>  	if (vystmt == NULL)
>  		return -1;
>  	assert(vy_stmt_type(vystmt) == IPROTO_UPSERT);
> diff --git a/src/box/vy_upsert.c b/src/box/vy_upsert.c
> index e697b6321..2b5a16730 100644
> --- a/src/box/vy_upsert.c
> +++ b/src/box/vy_upsert.c
> @@ -39,39 +39,150 @@
>  #include "column_mask.h"
>  
>  /**
> - * Try to squash two upsert series (msgspacked index_base + ops)
> - * Try to create a tuple with squahed operations
> + * Check that key hasn't been changed after applying upsert operation.
> + */
> +static bool
> +vy_apply_result_does_cross_pk(struct tuple *old_stmt, const char *result,
> +			      const char *result_end, struct key_def *cmp_def,
> +			      uint64_t col_mask)
> +{
> +	if (!key_update_can_be_skipped(cmp_def->column_mask, col_mask)) {
> +		struct tuple *tuple =
> +			vy_stmt_new_replace(tuple_format(old_stmt), result,
> +					    result_end);
> +		int cmp_res = vy_stmt_compare(old_stmt, HINT_NONE, tuple,
> +					      HINT_NONE, cmp_def);
> +		tuple_unref(tuple);
> +		return cmp_res != 0;
> +	}
> +	return false;
> +}
> +
> +/**
> + * Apply update operations stored in @a upsert on tuple @a stmt. If @a stmt is
> + * void statement (i.e. it is NULL or delete statement) then operations are
> + * applied on tuple stored in @a upsert. Update operations of @a upsert which

2. Applied on tuple stored in @a upsert? Upsert logic is that the operations are
ignored, if it appears the statement is first in its key. Did you mean, that you
apply all operation sets except the first set? Below I see a comment

	/*
	 * In case upsert folds into insert, we must skip first
	 * update operations.
	 */

So this is probably it. Just the function comment is a bit misleading.

> + * can't be applied are skipped along side with other operations from single
> + * group (i.e. packed in one msgpack array); errors may be logged depending on
> + * @a suppress_error flag.
>   *
> - * @retval 0 && *result_stmt != NULL : successful squash
> - * @retval 0 && *result_stmt == NULL : unsquashable sources
> - * @retval -1 - memory error
> + * @param upsert Upsert statement to be applied on @a stmt.
> + * @param stmt Statement to be used as base for upsert operations.
> + * @param cmp_def Key definition required to provide check of primary key
> + *                modification.

3. param suppress_error is missing.

> + * @return Tuple containing result of upsert application; NULL in case OOM.
>   */
My commit on top of the branch:

====================
    [tosquash] vinyl: remove is_ops_encoded argument
    
    It was used in function vy_stmt_new_with_ops, but wasn't really
    needed. Because the function takes an array of iovec structs,
    where the caller can put the array header.

diff --git a/src/box/vinyl.c b/src/box/vinyl.c
index 9d9ee0613..ee87a6c6e 100644
--- a/src/box/vinyl.c
+++ b/src/box/vinyl.c
@@ -1972,11 +1972,15 @@ vy_lsm_upsert(struct vy_tx *tx, struct vy_lsm *lsm,
 {
 	assert(tx == NULL || tx->state == VINYL_TX_READY);
 	struct tuple *vystmt;
-	struct iovec operations[1];
-	operations[0].iov_base = (void *)expr;
-	operations[0].iov_len = expr_end - expr;
+	struct iovec operations[2];
+	/* MP_ARRAY with size 1. */
+	char header = 0x91;
+	operations[0].iov_base = &header;
+	operations[0].iov_len = 1;
+	operations[1].iov_base = (void *)expr;
+	operations[1].iov_len = expr_end - expr;
 	vystmt = vy_stmt_new_upsert(lsm->mem_format, tuple, tuple_end,
-				    operations, 1, false);
+				    operations, 2);
 	if (vystmt == NULL)
 		return -1;
 	assert(vy_stmt_type(vystmt) == IPROTO_UPSERT);
diff --git a/src/box/vy_stmt.c b/src/box/vy_stmt.c
index 2ee82b3f2..92e0aa1c5 100644
--- a/src/box/vy_stmt.c
+++ b/src/box/vy_stmt.c
@@ -313,22 +313,16 @@ vy_key_dup(const char *key)
 static struct tuple *
 vy_stmt_new_with_ops(struct tuple_format *format, const char *tuple_begin,
 		     const char *tuple_end, struct iovec *ops,
-		     int op_count, enum iproto_type type, bool is_ops_encoded)
+		     int op_count, enum iproto_type type)
 {
 	mp_tuple_assert(tuple_begin, tuple_end);
 
 	const char *tmp = tuple_begin;
 	mp_decode_array(&tmp);
 
-	/*
-	 * ops are grouped in one extra array.
-	 * See vy_apply_upsert() for details.
-	 */
 	size_t ops_size = 0;
 	for (int i = 0; i < op_count; ++i)
 		ops_size += ops[i].iov_len;
-	if (!is_ops_encoded)
-		ops_size += mp_sizeof_array(op_count);
 
 	struct tuple *stmt = NULL;
 	struct region *region = &fiber()->gc;
@@ -366,8 +360,6 @@ vy_stmt_new_with_ops(struct tuple_format *format, const char *tuple_begin,
 	field_map_build(&builder, wpos - field_map_size);
 	memcpy(wpos, tuple_begin, mpsize);
 	wpos += mpsize;
-	if (!is_ops_encoded)
-		wpos = mp_encode_array(wpos, op_count);
 	for (struct iovec *op = ops, *end = ops + op_count;
 	     op != end; ++op) {
 		memcpy(wpos, op->iov_base, op->iov_len);
@@ -382,11 +374,10 @@ end:
 struct tuple *
 vy_stmt_new_upsert(struct tuple_format *format, const char *tuple_begin,
 		   const char *tuple_end, struct iovec *operations,
-		   uint32_t ops_cnt, bool is_ops_encoded)
+		   uint32_t ops_cnt)
 {
 	return vy_stmt_new_with_ops(format, tuple_begin, tuple_end,
-				    operations, ops_cnt, IPROTO_UPSERT,
-				    is_ops_encoded);
+				    operations, ops_cnt, IPROTO_UPSERT);
 }
 
 struct tuple *
@@ -394,7 +385,7 @@ vy_stmt_new_replace(struct tuple_format *format, const char *tuple_begin,
 		    const char *tuple_end)
 {
 	return vy_stmt_new_with_ops(format, tuple_begin, tuple_end,
-				    NULL, 0, IPROTO_REPLACE, true);
+				    NULL, 0, IPROTO_REPLACE);
 }
 
 struct tuple *
@@ -402,7 +393,7 @@ vy_stmt_new_insert(struct tuple_format *format, const char *tuple_begin,
 		   const char *tuple_end)
 {
 	return vy_stmt_new_with_ops(format, tuple_begin, tuple_end,
-				    NULL, 0, IPROTO_INSERT, true);
+				    NULL, 0, IPROTO_INSERT);
 }
 
 struct tuple *
@@ -410,7 +401,7 @@ vy_stmt_new_delete(struct tuple_format *format, const char *tuple_begin,
 		   const char *tuple_end)
 {
 	return vy_stmt_new_with_ops(format, tuple_begin, tuple_end,
-				    NULL, 0, IPROTO_DELETE, true);
+				    NULL, 0, IPROTO_DELETE);
 }
 
 struct tuple *
@@ -744,20 +735,19 @@ vy_stmt_decode(struct xrow_header *xrow, struct tuple_format *format)
 		/* Always use key format for DELETE statements. */
 		stmt = vy_stmt_new_with_ops(env->key_format,
 					    request.key, request.key_end,
-					    NULL, 0, IPROTO_DELETE, true);
+					    NULL, 0, IPROTO_DELETE);
 		break;
 	case IPROTO_INSERT:
 	case IPROTO_REPLACE:
 		stmt = vy_stmt_new_with_ops(format, request.tuple,
 					    request.tuple_end,
-					    NULL, 0, request.type, true);
+					    NULL, 0, request.type);
 		break;
 	case IPROTO_UPSERT:
 		ops.iov_base = (char *)request.ops;
 		ops.iov_len = request.ops_end - request.ops;
 		stmt = vy_stmt_new_upsert(format, request.tuple,
-					  request.tuple_end, &ops,
-					  1, true);
+					  request.tuple_end, &ops, 1);
 		break;
 	default:
 		/* TODO: report filename. */
diff --git a/src/box/vy_stmt.h b/src/box/vy_stmt.h
index 65acbecac..24c7eaad7 100644
--- a/src/box/vy_stmt.h
+++ b/src/box/vy_stmt.h
@@ -526,10 +526,11 @@ vy_stmt_new_delete(struct tuple_format *format, const char *tuple_begin,
  * @param tuple_end End of the array that begins from @param tuple_begin.
  * @param format Format of a tuple for offsets generating.
  * @param part_count Part count from key definition.
- * @param operations Vector of update operations.
+ * @param operations Vector of update operation pieces. Each iovec here may be
+ *     a part of an operation, or a whole operation, or something including
+ *     several operations. It is just a list of buffers. Each buffer is not
+ *     interpreted as an independent operation.
  * @param ops_cnt Length of the update operations vector.
- * @param is_ops_encoded True, if update operations are already packed
-  *                      into extra msgpack array.
  *
  * @retval NULL     Memory allocation error.
  * @retval not NULL Success.
@@ -537,8 +538,7 @@ vy_stmt_new_delete(struct tuple_format *format, const char *tuple_begin,
 struct tuple *
 vy_stmt_new_upsert(struct tuple_format *format,
 		   const char *tuple_begin, const char *tuple_end,
-		   struct iovec *operations, uint32_t ops_cnt,
-		   bool is_ops_encoded);
+		   struct iovec *operations, uint32_t ops_cnt);
 
 /**
  * Create REPLACE statement from UPSERT statement.
diff --git a/src/box/vy_upsert.c b/src/box/vy_upsert.c
index 2b5a16730..d73896cad 100644
--- a/src/box/vy_upsert.c
+++ b/src/box/vy_upsert.c
@@ -223,18 +223,23 @@ vy_apply_upsert(struct tuple *new_stmt, struct tuple *old_stmt,
 	size_t region_svp = region_used(region);
 	uint32_t old_ops_cnt = mp_decode_array(&old_ops);
 	uint32_t new_ops_cnt = mp_decode_array(&new_ops);
+	uint32_t total_ops_cnt = old_ops_cnt + new_ops_cnt;
 	size_t ops_size;
 	struct iovec *operations =
 		region_alloc_array(region, typeof(operations[0]),
-				   old_ops_cnt + new_ops_cnt, &ops_size);
+				   total_ops_cnt + 1, &ops_size);
 	if (operations == NULL) {
 		region_truncate(region, region_svp);
 		diag_set(OutOfMemory, ops_size, "region_alloc_array",
 			 "operations");
 		return NULL;
 	}
-	upsert_ops_to_iovec(old_ops, old_ops_cnt, operations);
-	upsert_ops_to_iovec(new_ops, new_ops_cnt, &operations[old_ops_cnt]);
+	char header[16];
+	char *header_end = mp_encode_array(header, total_ops_cnt);
+	operations[0].iov_base = header;
+	operations[0].iov_len = header_end - header;
+	upsert_ops_to_iovec(old_ops, old_ops_cnt, &operations[1]);
+	upsert_ops_to_iovec(new_ops, new_ops_cnt, &operations[old_ops_cnt + 1]);
 	/*
 	 * Adding update operations. We keep order of update operations in
 	 * the array the same. It is vital since first set of operations
@@ -246,8 +251,7 @@ vy_apply_upsert(struct tuple *new_stmt, struct tuple *old_stmt,
 	 * {{op1}, {op2}} update operations are not applied.
 	 */
 	result_stmt = vy_stmt_new_upsert(format, old_stmt_mp, old_stmt_mp_end,
-					 operations, old_ops_cnt + new_ops_cnt,
-					 false);
+					 operations, total_ops_cnt + 1);
 	region_truncate(region, region_svp);
 	if (result_stmt == NULL)
 		return NULL;
diff --git a/test/unit/vy_iterators_helper.c b/test/unit/vy_iterators_helper.c
index 15470920b..0d20f19ef 100644
--- a/test/unit/vy_iterators_helper.c
+++ b/test/unit/vy_iterators_helper.c
@@ -112,7 +112,7 @@ vy_new_simple_stmt(struct tuple_format *format, struct key_def *key_def,
 			ops = mp_encode_int(ops, templ->upsert_value);
 		operations[0].iov_base = tmp;
 		operations[0].iov_len = ops - tmp;
-		ret = vy_stmt_new_upsert(format, buf, pos, operations, 1, true);
+		ret = vy_stmt_new_upsert(format, buf, pos, operations, 1);
 		fail_if(ret == NULL);
 		break;
 	}

Re: [Tarantool-patches] [PATCH v3 1/2] vinyl: rework upsert operation

[Nikita Pettik]

On 07 Oct 00:12, Vladislav Shpilevoy wrote:
> Thanks for the patch!
> 
> See 3 comments below, and my fixes on top of this branch in a
> separate commit.
> 
> >  src/box/vinyl.c                 |   2 +-
> >  src/box/vy_stmt.c               |  28 +-
> >  src/box/vy_stmt.h               |   5 +-
> >  src/box/vy_upsert.c             | 302 +++++++++++---------
> >  test/unit/vy_iterators_helper.c |   2 +-
> >  test/vinyl/upsert.result        | 473 ++++++++++++++++++++++++++++++++
> >  test/vinyl/upsert.test.lua      | 194 +++++++++++++
> >  7 files changed, 867 insertions(+), 139 deletions(-)
> > 
> > diff --git a/src/box/vinyl.c b/src/box/vinyl.c
> > index cee39c58c..9d9ee0613 100644
> > --- a/src/box/vinyl.c
> > +++ b/src/box/vinyl.c
> > @@ -1976,7 +1976,7 @@ vy_lsm_upsert(struct vy_tx *tx, struct vy_lsm *lsm,
> >  	operations[0].iov_base = (void *)expr;
> >  	operations[0].iov_len = expr_end - expr;
> >  	vystmt = vy_stmt_new_upsert(lsm->mem_format, tuple, tuple_end,
> > -				    operations, 1);
> > +				    operations, 1, false);
> 
> 1. You don't really need that flag. In the previous review I tried to
> explain it, but now I added a commit on top of your branch where I
> removed this parameter. Take a look.
> 
> This argument does not have much sense. First of all, the operations array
> is not really operations. Iovecs here can be separate operations; it can be
> a single iovec with all operations; it can be several iovecs with first
> having additional MP_ARRAY encoded before a first operation. So these are
> just pieces of raw data. And you can use that. You can push a header of
> these operations as a part of 'operations' array always, like I did in my
> commit. No need to pass a new argument for that.

Ok, thx, squashed.
 
> >  	if (vystmt == NULL)
> >  		return -1;
> >  	assert(vy_stmt_type(vystmt) == IPROTO_UPSERT);
> > diff --git a/src/box/vy_upsert.c b/src/box/vy_upsert.c
> > index e697b6321..2b5a16730 100644
> > --- a/src/box/vy_upsert.c
> > +++ b/src/box/vy_upsert.c
> > @@ -39,39 +39,150 @@
> >  #include "column_mask.h"
> >  
> > +/**
> > + * Apply update operations stored in @a upsert on tuple @a stmt. If @a stmt is
> > + * void statement (i.e. it is NULL or delete statement) then operations are
> > + * applied on tuple stored in @a upsert. Update operations of @a upsert which
> 
> 2. Applied on tuple stored in @a upsert? Upsert logic is that the operations are
> ignored, if it appears the statement is first in its key. Did you mean, that you
> apply all operation sets except the first set? Below I see a comment
> 
> 	/*
> 	 * In case upsert folds into insert, we must skip first
> 	 * update operations.
> 	 */
> 
> So this is probably it. Just the function comment is a bit misleading.

A bit fixed to this version:

 * Apply update operations from @a upsert on tuple @a stmt.
 
> > + * can't be applied are skipped along side with other operations from single
> > + * group (i.e. packed in one msgpack array); errors may be logged depending on
> > + * @a suppress_error flag.
> >   *
> > - * @retval 0 && *result_stmt != NULL : successful squash
> > - * @retval 0 && *result_stmt == NULL : unsquashable sources
> > - * @retval -1 - memory error
> > + * @param upsert Upsert statement to be applied on @a stmt.
> > + * @param stmt Statement to be used as base for upsert operations.
> > + * @param cmp_def Key definition required to provide check of primary key
> > + *                modification.
> 
> 3. param suppress_error is missing.

Added description:

 * @param suppress_error If true, do not raise/log any errors.

> 
> > + * @return Tuple containing result of upsert application; NULL in case OOM.
> >   */

Re: [Tarantool-patches] [PATCH v3 0/2] vinyl: rework upsert operation

[Nikita Pettik]

On 07 Oct 00:12, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patchset!
> 
> Does this patchset change format of vinyl statements on disk? If yes, I guess
> it breaks backward compatibility. Did you check how does it behave when booted
> from files of a previous version? Does it crash? Does it report an error? How
> a user can upgrade his vinyl files to the new format?

No, everything goes fine except the fact that upsert statements restored
from disk have ops encoded into single array and as a result being applied
may break associative property (like it was before this patch). I've tried
to add a test, but since squash operation modifies content of run/xlog files,
such test can be run only once. If it is vital, I guess it is possible to
copy data directory before each test run..

Re: [Tarantool-patches] [PATCH v3 0/2] vinyl: rework upsert operation

[Vladislav Shpilevoy]

>> Does this patchset change format of vinyl statements on disk? If yes, I guess
>> it breaks backward compatibility. Did you check how does it behave when booted
>> from files of a previous version? Does it crash? Does it report an error? How
>> a user can upgrade his vinyl files to the new format?
> 
> No, everything goes fine except the fact that upsert statements restored
> from disk have ops encoded into single array and as a result being applied
> may break associative property (like it was before this patch). I've tried
> to add a test, but since squash operation modifies content of run/xlog files,
> such test can be run only once. If it is vital, I guess it is possible to
> copy data directory before each test run..

Yes, it is possible. At least it would be better than not having a test at
all.

Re: [Tarantool-patches] [PATCH v3 0/2] vinyl: rework upsert operation

[Vladislav Shpilevoy]

The patchset looks good except for not having a backward compatibility test.

Re: [Tarantool-patches] [PATCH v3 0/2] vinyl: rework upsert operation

[Nikita Pettik]

On 11 Oct 17:35, Vladislav Shpilevoy wrote:
> The patchset looks good except for not having a backward compatibility test.

Here's a test:

diff --git a/test/vinyl/upgrade/upsert/00000000000000000000.vylog b/test/vinyl/upgrade/upsert/00000000000000000000.vylog
new file mode 100644
index 000000000..581ad9b53
Binary files /dev/null and b/test/vinyl/upgrade/upsert/00000000000000000000.vylog differ
diff --git a/test/vinyl/upgrade/upsert/00000000000000000004.vylog b/test/vinyl/upgrade/upsert/00000000000000000004.vylog
new file mode 100644
index 000000000..5194b1457
Binary files /dev/null and b/test/vinyl/upgrade/upsert/00000000000000000004.vylog differ
diff --git a/test/vinyl/upgrade/upsert/00000000000000000004.xlog b/test/vinyl/upgrade/upsert/00000000000000000004.xlog
new file mode 100644
index 000000000..5725d09ce
Binary files /dev/null and b/test/vinyl/upgrade/upsert/00000000000000000004.xlog differ
diff --git a/test/vinyl/upgrade/upsert/00000000000000000010.vylog b/test/vinyl/upgrade/upsert/00000000000000000010.vylog
new file mode 100644
index 000000000..954a51a4c
Binary files /dev/null and b/test/vinyl/upgrade/upsert/00000000000000000010.vylog differ
diff --git a/test/vinyl/upgrade/upsert/00000000000000000010.xlog b/test/vinyl/upgrade/upsert/00000000000000000010.xlog
new file mode 100644
index 000000000..f5921223d
--- /dev/null
+++ b/test/vinyl/upgrade/upsert/00000000000000000010.xlog
@@ -0,0 +1,8 @@
+XLOG
+0.13
+Version: 2.6.0-153-g3bc455f7d
+Instance: 4b1aba36-0cbc-4d51-9968-53da3908a43d
+VClock: {1: 10}
+PrevVClock: {1: 4}
+
+<D5>^P<AD><ED>
\ No newline at end of file
diff --git a/test/vinyl/upgrade/upsert/512/0/00000000000000000002.index b/test/vinyl/upgrade/upsert/512/0/00000000000000000002.index
new file mode 100644
index 000000000..156c23dd7
Binary files /dev/null and b/test/vinyl/upgrade/upsert/512/0/00000000000000000002.index differ
diff --git a/test/vinyl/upgrade/upsert/512/0/00000000000000000002.run b/test/vinyl/upgrade/upsert/512/0/00000000000000000002.run
new file mode 100644
index 000000000..9757ea341
Binary files /dev/null and b/test/vinyl/upgrade/upsert/512/0/00000000000000000002.run differ
diff --git a/test/vinyl/upgrade/upsert/512/0/00000000000000000004.index b/test/vinyl/upgrade/upsert/512/0/00000000000000000004.index
new file mode 100644
index 000000000..1c3896547
Binary files /dev/null and b/test/vinyl/upgrade/upsert/512/0/00000000000000000004.index differ
diff --git a/test/vinyl/upgrade/upsert/512/0/00000000000000000004.run b/test/vinyl/upgrade/upsert/512/0/00000000000000000004.run
new file mode 100644
index 000000000..469e2abab
Binary files /dev/null and b/test/vinyl/upgrade/upsert/512/0/00000000000000000004.run differ
diff --git a/test/vinyl/upgrade/upsert/512/0/00000000000000000006.index b/test/vinyl/upgrade/upsert/512/0/00000000000000000006.index
new file mode 100644
index 000000000..9202ec7e7
Binary files /dev/null and b/test/vinyl/upgrade/upsert/512/0/00000000000000000006.index differ
diff --git a/test/vinyl/upgrade/upsert/512/0/00000000000000000006.run b/test/vinyl/upgrade/upsert/512/0/00000000000000000006.run
new file mode 100644
index 000000000..d4fcd7599
Binary files /dev/null and b/test/vinyl/upgrade/upsert/512/0/00000000000000000006.run differ
diff --git a/test/vinyl/upsert_upgrade.result b/test/vinyl/upsert_upgrade.result
new file mode 100644
index 000000000..8882a8b63
--- /dev/null
+++ b/test/vinyl/upsert_upgrade.result
@@ -0,0 +1,59 @@
+-- test-run result file version 2
+test_run = require('test_run').new()
+ | ---
+ | ...
+
+-- Upsert's internal format have changed: now update operations are stored
+-- with additional map package. Let's test backward compatibility.
+-- Snapshot (i.e. run files) contain following statements:
+
+-- s = box.schema.create_space('test', {engine = 'vinyl'})
+-- pk = s:create_index('pk')
+-- s:insert({1, 2})
+-- box.snapshot()
+-- s:upsert({1, 0}, {{'+', 2, 1}})
+-- s:upsert({1, 0}, {{'-', 2, 2}})
+-- s:upsert({2, 0}, {{'+', 2, 1}})
+-- s:upsert({2, 0}, {{'-', 2, 2}})
+-- s:upsert({1, 0}, {{'=', 2, 2}})
+-- s:upsert({1, 0}, {{'-', 2, 2}})
+-- box.snapshot()
+--
+-- Make sure that upserts will be parsed and squashed correctly.
+--
+
+dst_dir = 'vinyl/upgrade/upsert/'
+ | ---
+ | ...
+
+test_run:cmd('create server upgrade with script="vinyl/upgrade.lua", workdir="' .. dst_dir .. '"')
+ | ---
+ | - true
+ | ...
+test_run:cmd('start server upgrade')
+ | ---
+ | - true
+ | ...
+test_run:switch('upgrade')
+ | ---
+ | - true
+ | ...
+
+box.space.test:select()
+ | ---
+ | - - [1, 0]
+ |   - [2, -2]
+ | ...
+
+test_run:switch('default')
+ | ---
+ | - true
+ | ...
+test_run:cmd('stop server upgrade')
+ | ---
+ | - true
+ | ...
+test_run:cmd('cleanup server upgrade')
+ | ---
+ | - true
+ | ...
diff --git a/test/vinyl/upsert_upgrade.test.lua b/test/vinyl/upsert_upgrade.test.lua
new file mode 100644
index 000000000..db409e2dd
--- /dev/null
+++ b/test/vinyl/upsert_upgrade.test.lua
@@ -0,0 +1,32 @@
+test_run = require('test_run').new()
+
+-- Upsert's internal format have changed: now update operations are stored
+-- with additional map package. Let's test backward compatibility.
+-- Snapshot (i.e. run files) contain following statements:
+
+-- s = box.schema.create_space('test', {engine = 'vinyl'})
+-- pk = s:create_index('pk')
+-- s:insert({1, 2})
+-- box.snapshot()
+-- s:upsert({1, 0}, {{'+', 2, 1}})
+-- s:upsert({1, 0}, {{'-', 2, 2}})
+-- s:upsert({2, 0}, {{'+', 2, 1}})
+-- s:upsert({2, 0}, {{'-', 2, 2}})
+-- s:upsert({1, 0}, {{'=', 2, 2}})
+-- s:upsert({1, 0}, {{'-', 2, 2}})
+-- box.snapshot()
+--
+-- Make sure that upserts will be parsed and squashed correctly.
+--
+
+dst_dir = 'vinyl/upgrade/upsert/'
+
+test_run:cmd('create server upgrade with script="vinyl/upgrade.lua", workdir="' .. dst_dir .. '"')
+test_run:cmd('start server upgrade')
+test_run:switch('upgrade')
+
+box.space.test:select()
+
+test_run:switch('default')
+test_run:cmd('stop server upgrade')
+test_run:cmd('cleanup server upgrade')

Re: [Tarantool-patches] [PATCH v3 1/2] vinyl: rework upsert operation

[Aleksandr Lyapunov]

Hi, thanks for the patch! Se my 2 comments below:
(the second seems to be important)
Once they resolved, the whole patch is LGTM!

On 10/3/20 4:28 PM, Nikita Pettik wrote:
> into insert the upsert's tuple and the tuple stored in index can be
> different In our particular case, tuple stored on disk has two fields
Dot before `In` is missing.
> +		/*
> +		 * If it turns out that resulting tuple modifies primary
> +		 * key, then simply ignore this upsert.
> +		 */
> +		if (stmt != NULL &&
> +		    vy_apply_result_does_cross_pk(stmt, exec_res,
> +						  exec_res + mp_size, cmp_def,
> +						  column_mask)) {
I guess in case if `stmt_is_void = true` we must compare with `upsert` 
tuple,
not with `stmt` tuple.
> +			if (!suppress_error) {
> +				say_error("upsert operations %s are not applied"\
> +					  " due to primary key modification",
> +					  mp_str(ups_ops));
> +			}
> +			ups_ops = ups_ops_end;
> +			continue;
> +		}
> +		ups_ops = ups_ops_end;
> +		/*
> +		 * Result statement must satisfy space's format. Since upsert's
> +		 * tuple correctness is already checked in vy_upsert(), let's
> +		 * use its format to provide result verification.
> +		 */
> +		struct tuple_format *format = tuple_format(upsert);
> +		if (tuple_validate_raw(format, exec_res) != 0) {
> +			if (! suppress_error)
> +				diag_log();
> +			continue;
> +		}
> +		result_mp = exec_res;
> +		result_mp_end = exec_res + mp_size;
> +	}
> +	struct tuple *new_terminal_stmt = vy_stmt_new_replace(format, result_mp,
> +							      result_mp_end);
> +	region_truncate(region, region_svp);
> +	if (new_terminal_stmt == NULL)
> +		return NULL;
> +	vy_stmt_set_lsn(new_terminal_stmt, vy_stmt_lsn(upsert));
> +	return new_terminal_stmt;
> +}

Re: [Tarantool-patches] [PATCH v3 1/2] vinyl: rework upsert operation

[Nikita Pettik]

On 13 Oct 22:00, Aleksandr Lyapunov wrote:
> Hi, thanks for the patch! Se my 2 comments below:
> (the second seems to be important)
> Once they resolved, the whole patch is LGTM!
> 
> On 10/3/20 4:28 PM, Nikita Pettik wrote:
> > into insert the upsert's tuple and the tuple stored in index can be
> > different In our particular case, tuple stored on disk has two fields
> Dot before `In` is missing.

Fixed.

> > +		/*
> > +		 * If it turns out that resulting tuple modifies primary
> > +		 * key, then simply ignore this upsert.
> > +		 */
> > +		if (stmt != NULL &&
> > +		    vy_apply_result_does_cross_pk(stmt, exec_res,
> > +						  exec_res + mp_size, cmp_def,
> > +						  column_mask)) {
> I guess in case if `stmt_is_void = true` we must compare with `upsert`
> tuple,
> not with `stmt` tuple.

Yep, thanks for input. Fixed and addded corresponding tests:

diff --git a/src/box/vy_upsert.c b/src/box/vy_upsert.c
index 090f3304b..0b4baf6ff 100644
--- a/src/box/vy_upsert.c
+++ b/src/box/vy_upsert.c
@@ -102,11 +102,15 @@ vy_apply_upsert_on_terminal_stmt(struct tuple *upsert, struct tuple *stmt,
        const char *ups_ops = new_ops;
        /*
         * In case upsert folds into insert, we must skip first
-        * update operations.
+        * update operations. Moreover, we should create (in case of delete
+        * statement - re-create since delete contains only key) tuple with
+        * format to use it for PK modification check.
         */
        if (stmt_is_void) {
                ups_cnt--;
                mp_next(&ups_ops);
+               stmt = vy_stmt_new_replace(tuple_format(upsert), result_mp,
+                                          result_mp_end);
        }
        for (uint32_t i = 0; i < ups_cnt; ++i) {
                assert(mp_typeof(*ups_ops) == MP_ARRAY);
@@ -122,6 +126,8 @@ vy_apply_upsert_on_terminal_stmt(struct tuple *upsert, struct tuple *stmt,
                                assert(e != NULL);
                                /* Bail out immediately in case of OOM. */
                                if (e->type != &type_ClientError) {
+                                       if (stmt_is_void)
+                                               tuple_unref(stmt);
                                        region_truncate(region, region_svp);
                                        return NULL;
                                }
@@ -134,8 +140,7 @@ vy_apply_upsert_on_terminal_stmt(struct tuple *upsert, struct tuple *stmt,
                 * If it turns out that resulting tuple modifies primary
                 * key, then simply ignore this upsert.
                 */
-               if (stmt != NULL &&
-                   vy_apply_result_does_cross_pk(stmt, exec_res,
+               if (vy_apply_result_does_cross_pk(stmt, exec_res,
                                                  exec_res + mp_size, cmp_def,
                                                  column_mask)) {
                        if (!suppress_error) {
@@ -164,6 +169,8 @@ vy_apply_upsert_on_terminal_stmt(struct tuple *upsert, struct tuple *stmt,
        struct tuple *new_terminal_stmt = vy_stmt_new_replace(format, result_mp,
                                                              result_mp_end);
        region_truncate(region, region_svp);
+       if (stmt_is_void)
+               tuple_unref(stmt);
        if (new_terminal_stmt == NULL)
                return NULL;
        vy_stmt_set_lsn(new_terminal_stmt, vy_stmt_lsn(upsert));
diff --git a/test/vinyl/upsert.result b/test/vinyl/upsert.result
index a66e87be0..fe673ad6f 100644
--- a/test/vinyl/upsert.result
+++ b/test/vinyl/upsert.result
@@ -1372,3 +1372,97 @@ s:select{}
 s:drop()
 ---
 ...
+-- Combination of upserts and underlying void (i.e. delete or null)
+-- statement on disk. Upsert modifying PK is skipped.
+--
+s = box.schema.space.create('test', {engine = 'vinyl'})
+---
+...
+i = s:create_index('test', { run_count_per_level = 20 })
+---
+...
+for i = 101, 110 do s:replace{i, i} end
+---
+...
+s:replace({1, 1})
+---
+- [1, 1]
+...
+box.snapshot()
+---
+- ok
+...
+s:delete({1})
+---
+...
+box.snapshot()
+---
+- ok
+...
+s:upsert({1, 1}, {{'=', 2, 2}})
+---
+...
+s:upsert({1, 1}, {{'=', 1, 0}})
+---
+...
+box.snapshot()
+---
+- ok
+...
+s:select()
+---
+- - [1, 1]
+  - [101, 101]
+  - [102, 102]
+  - [103, 103]
+  - [104, 104]
+  - [105, 105]
+  - [106, 106]
+  - [107, 107]
+  - [108, 108]
+  - [109, 109]
+  - [110, 110]
+...
+s:drop()
+---
+...
+s = box.schema.space.create('test', {engine = 'vinyl'})
+---
+...
+i = s:create_index('test', { run_count_per_level = 20 })
+---
+...
+for i = 101, 110 do s:replace{i, i} end
+---
+...
+box.snapshot()
+---
+- ok
+...
+s:upsert({1, 1}, {{'=', 2, 2}})
+---
+...
+s:upsert({1, 1}, {{'=', 1, 0}})
+---
+...
+box.snapshot()
+---
+- ok
+...
+s:select()
+---
+- - [1, 1]
+  - [101, 101]
+  - [102, 102]
+  - [103, 103]
+  - [104, 104]
+  - [105, 105]
+  - [106, 106]
+  - [107, 107]
+  - [108, 108]
+  - [109, 109]
+  - [110, 110]
+...
+s:drop()
+---
+...
diff --git a/test/vinyl/upsert.test.lua b/test/vinyl/upsert.test.lua
index ad69f4b72..b62c19978 100644
--- a/test/vinyl/upsert.test.lua
+++ b/test/vinyl/upsert.test.lua
@@ -566,3 +566,33 @@ box.snapshot()
 s:select{}
 
 s:drop()
+
+-- Combination of upserts and underlying void (i.e. delete or null)
+-- statement on disk. Upsert modifying PK is skipped.
+--
+s = box.schema.space.create('test', {engine = 'vinyl'})
+i = s:create_index('test', { run_count_per_level = 20 })
+
+for i = 101, 110 do s:replace{i, i} end
+s:replace({1, 1})
+box.snapshot()
+s:delete({1})
+box.snapshot()
+s:upsert({1, 1}, {{'=', 2, 2}})
+s:upsert({1, 1}, {{'=', 1, 0}})
+box.snapshot()
+s:select()
+
+s:drop()
+
+s = box.schema.space.create('test', {engine = 'vinyl'})
+i = s:create_index('test', { run_count_per_level = 20 })
+
+for i = 101, 110 do s:replace{i, i} end
+box.snapshot()
+s:upsert({1, 1}, {{'=', 2, 2}})
+s:upsert({1, 1}, {{'=', 1, 0}})
+box.snapshot()
+s:select()
+
+s:drop()


> > +			if (!suppress_error) {
> > +				say_error("upsert operations %s are not applied"\
> > +					  " due to primary key modification",
> > +					  mp_str(ups_ops));
> > +			}
> > +			ups_ops = ups_ops_end;
> > +			continue;
> > +		}
> > +		ups_ops = ups_ops_end;
> > +		/*
> > +		 * Result statement must satisfy space's format. Since upsert's
> > +		 * tuple correctness is already checked in vy_upsert(), let's
> > +		 * use its format to provide result verification.
> > +		 */
> > +		struct tuple_format *format = tuple_format(upsert);
> > +		if (tuple_validate_raw(format, exec_res) != 0) {
> > +			if (! suppress_error)
> > +				diag_log();
> > +			continue;
> > +		}
> > +		result_mp = exec_res;
> > +		result_mp_end = exec_res + mp_size;
> > +	}
> > +	struct tuple *new_terminal_stmt = vy_stmt_new_replace(format, result_mp,
> > +							      result_mp_end);
> > +	region_truncate(region, region_svp);
> > +	if (new_terminal_stmt == NULL)
> > +		return NULL;
> > +	vy_stmt_set_lsn(new_terminal_stmt, vy_stmt_lsn(upsert));
> > +	return new_terminal_stmt;
> > +}

Re: [Tarantool-patches] [PATCH v3 1/2] vinyl: rework upsert operation

[Aleksandr Lyapunov]

>          if (stmt_is_void) {
>                  ups_cnt--;
>                  mp_next(&ups_ops);
> +               stmt = vy_stmt_new_replace(tuple_format(upsert), result_mp,
> +                                          result_mp_end);
Why can't you just use `upsert` tuple itself instead of created tuple by 
its format and data?

Re: [Tarantool-patches] [PATCH v3 1/2] vinyl: rework upsert operation

[Nikita Pettik]

On 14 Oct 11:58, Aleksandr Lyapunov wrote:
> 
> >          if (stmt_is_void) {
> >                  ups_cnt--;
> >                  mp_next(&ups_ops);
> > +               stmt = vy_stmt_new_replace(tuple_format(upsert), result_mp,
> > +                                          result_mp_end);
> Why can't you just use `upsert` tuple itself instead of created tuple by its
> format and data?
>

For sure we can. Sorry it was late night so I didn't pay attention to this fact.
Diff:

diff --git a/src/box/vy_upsert.c b/src/box/vy_upsert.c
index 0b4baf6ff..fdae931f6 100644
--- a/src/box/vy_upsert.c
+++ b/src/box/vy_upsert.c
@@ -102,15 +102,13 @@ vy_apply_upsert_on_terminal_stmt(struct tuple *upsert, struct tuple *stmt,
        const char *ups_ops = new_ops;
        /*
         * In case upsert folds into insert, we must skip first
-        * update operations. Moreover, we should create (in case of delete
-        * statement - re-create since delete contains only key) tuple with
-        * format to use it for PK modification check.
+        * update operations. Moreover, we should use upsert's tuple
+        * to provide PK modification check.
         */
        if (stmt_is_void) {
                ups_cnt--;
                mp_next(&ups_ops);
-               stmt = vy_stmt_new_replace(tuple_format(upsert), result_mp,
-                                          result_mp_end);
+               stmt = upsert;
        }
        for (uint32_t i = 0; i < ups_cnt; ++i) {
                assert(mp_typeof(*ups_ops) == MP_ARRAY);
@@ -126,8 +124,6 @@ vy_apply_upsert_on_terminal_stmt(struct tuple *upsert, struct tuple *stmt,
                                assert(e != NULL);
                                /* Bail out immediately in case of OOM. */
                                if (e->type != &type_ClientError) {
-                                       if (stmt_is_void)
-                                               tuple_unref(stmt);
                                        region_truncate(region, region_svp);
                                        return NULL;
                                }
@@ -169,8 +165,6 @@ vy_apply_upsert_on_terminal_stmt(struct tuple *upsert, struct tuple *stmt,
        struct tuple *new_terminal_stmt = vy_stmt_new_replace(format, result_mp,
                                                              result_mp_end);
        region_truncate(region, region_svp);
-       if (stmt_is_void)
-               tuple_unref(stmt);
        if (new_terminal_stmt == NULL)
                return NULL;
        vy_stmt_set_lsn(new_terminal_stmt, vy_stmt_lsn(upsert));

 

Re: [Tarantool-patches] [PATCH v3 1/2] vinyl: rework upsert operation

[Aleksandr Lyapunov]

Great, LGTM!

On 10/14/20 1:42 PM, Nikita Pettik wrote:
> On 14 Oct 11:58, Aleksandr Lyapunov wrote:
>>>           if (stmt_is_void) {
>>>                   ups_cnt--;
>>>                   mp_next(&ups_ops);
>>> +               stmt = vy_stmt_new_replace(tuple_format(upsert), result_mp,
>>> +                                          result_mp_end);
>> Why can't you just use `upsert` tuple itself instead of created tuple by its
>> format and data?
>>
> For sure we can. Sorry it was late night so I didn't pay attention to this fact.
> Diff:
>
> diff --git a/src/box/vy_upsert.c b/src/box/vy_upsert.c
> index 0b4baf6ff..fdae931f6 100644
> --- a/src/box/vy_upsert.c
> +++ b/src/box/vy_upsert.c
> @@ -102,15 +102,13 @@ vy_apply_upsert_on_terminal_stmt(struct tuple *upsert, struct tuple *stmt,
>          const char *ups_ops = new_ops;
>          /*
>           * In case upsert folds into insert, we must skip first
> -        * update operations. Moreover, we should create (in case of delete
> -        * statement - re-create since delete contains only key) tuple with
> -        * format to use it for PK modification check.
> +        * update operations. Moreover, we should use upsert's tuple
> +        * to provide PK modification check.
>           */
>          if (stmt_is_void) {
>                  ups_cnt--;
>                  mp_next(&ups_ops);
> -               stmt = vy_stmt_new_replace(tuple_format(upsert), result_mp,
> -                                          result_mp_end);
> +               stmt = upsert;
>          }
>          for (uint32_t i = 0; i < ups_cnt; ++i) {
>                  assert(mp_typeof(*ups_ops) == MP_ARRAY);
> @@ -126,8 +124,6 @@ vy_apply_upsert_on_terminal_stmt(struct tuple *upsert, struct tuple *stmt,
>                                  assert(e != NULL);
>                                  /* Bail out immediately in case of OOM. */
>                                  if (e->type != &type_ClientError) {
> -                                       if (stmt_is_void)
> -                                               tuple_unref(stmt);
>                                          region_truncate(region, region_svp);
>                                          return NULL;
>                                  }
> @@ -169,8 +165,6 @@ vy_apply_upsert_on_terminal_stmt(struct tuple *upsert, struct tuple *stmt,
>          struct tuple *new_terminal_stmt = vy_stmt_new_replace(format, result_mp,
>                                                                result_mp_end);
>          region_truncate(region, region_svp);
> -       if (stmt_is_void)
> -               tuple_unref(stmt);
>          if (new_terminal_stmt == NULL)
>                  return NULL;
>          vy_stmt_set_lsn(new_terminal_stmt, vy_stmt_lsn(upsert));
>
>   

Re: [Tarantool-patches] [PATCH v3 0/2] vinyl: rework upsert operation

[Vladislav Shpilevoy]

Hi! Thanks for the patch!

The test looks fine, but hard to extend. I would better try to
organize a more extendible folder structure, like test/xlog/upgrade
does. With the current 'flat' structure we will have problems with
adding more tests for other versions in the future.

I see you already pushed it, so I am too late.

Re: [Tarantool-patches] [PATCH v3 1/2] vinyl: rework upsert operation

[Nikita Pettik]

On 14 Oct 14:47, Aleksandr Lyapunov wrote:
> Great, LGTM!

Pushed to master, changelog is updated, branch is dropped.
 
> On 10/14/20 1:42 PM, Nikita Pettik wrote:
> > On 14 Oct 11:58, Aleksandr Lyapunov wrote:
> > > >           if (stmt_is_void) {
> > > >                   ups_cnt--;
> > > >                   mp_next(&ups_ops);
> > > > +               stmt = vy_stmt_new_replace(tuple_format(upsert), result_mp,
> > > > +                                          result_mp_end);
> > > Why can't you just use `upsert` tuple itself instead of created tuple by its
> > > format and data?
> > > 
> > For sure we can. Sorry it was late night so I didn't pay attention to this fact.
> > Diff:
> > 
> > diff --git a/src/box/vy_upsert.c b/src/box/vy_upsert.c
> > index 0b4baf6ff..fdae931f6 100644
> > --- a/src/box/vy_upsert.c
> > +++ b/src/box/vy_upsert.c
> > @@ -102,15 +102,13 @@ vy_apply_upsert_on_terminal_stmt(struct tuple *upsert, struct tuple *stmt,
> >          const char *ups_ops = new_ops;
> >          /*
> >           * In case upsert folds into insert, we must skip first
> > -        * update operations. Moreover, we should create (in case of delete
> > -        * statement - re-create since delete contains only key) tuple with
> > -        * format to use it for PK modification check.
> > +        * update operations. Moreover, we should use upsert's tuple
> > +        * to provide PK modification check.
> >           */
> >          if (stmt_is_void) {
> >                  ups_cnt--;
> >                  mp_next(&ups_ops);
> > -               stmt = vy_stmt_new_replace(tuple_format(upsert), result_mp,
> > -                                          result_mp_end);
> > +               stmt = upsert;
> >          }
> >          for (uint32_t i = 0; i < ups_cnt; ++i) {
> >                  assert(mp_typeof(*ups_ops) == MP_ARRAY);
> > @@ -126,8 +124,6 @@ vy_apply_upsert_on_terminal_stmt(struct tuple *upsert, struct tuple *stmt,
> >                                  assert(e != NULL);
> >                                  /* Bail out immediately in case of OOM. */
> >                                  if (e->type != &type_ClientError) {
> > -                                       if (stmt_is_void)
> > -                                               tuple_unref(stmt);
> >                                          region_truncate(region, region_svp);
> >                                          return NULL;
> >                                  }
> > @@ -169,8 +165,6 @@ vy_apply_upsert_on_terminal_stmt(struct tuple *upsert, struct tuple *stmt,
> >          struct tuple *new_terminal_stmt = vy_stmt_new_replace(format, result_mp,
> >                                                                result_mp_end);
> >          region_truncate(region, region_svp);
> > -       if (stmt_is_void)
> > -               tuple_unref(stmt);
> >          if (new_terminal_stmt == NULL)
> >                  return NULL;
> >          vy_stmt_set_lsn(new_terminal_stmt, vy_stmt_lsn(upsert));
> > 

Re: [Tarantool-patches] [PATCH v3 0/2] vinyl: rework upsert operation

[Nikita Pettik]

On 15 Oct 01:44, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patch!
> 
> The test looks fine, but hard to extend. I would better try to
> organize a more extendible folder structure, like test/xlog/upgrade
> does. With the current 'flat' structure we will have problems with
> adding more tests for other versions in the future.
> 
> I see you already pushed it, so I am too late.

Np, I'll do it tomorrow as follow-up. I've also found that
vinyl/upgrade.test.lua is disabled and seems there's no corresponding
snap/vylog files to run this test at all..