[Tarantool-patches] [PATCH] vinyl: fix squashing set and arithmetic operations

[Tarantool-patches] [PATCH] vinyl: fix squashing set and arithmetic operations

[Nikita Pettik]

Before this patch in upsert squashing procedure (which is implemented as
xrow_upsert_squash() function) was missed the case when first (old one)
operation is set and second one is addition or subtraction. This led to
the wrong result of squashing these operations. Let's fix it and handle
such case: {'+', x} applied on the top of {'=', y} is {'=', x+y}.

Closes #5106
Part of #5107
---
Branch: https://github.com/tarantool/tarantool/tree/np/gh-5107-rework-upsert
Issue: https://github.com/tarantool/tarantool/issues/5106

@ChangeLog:
 - fix squashing set and arithmetic upsert operations (gh-5106).

 src/box/xrow_update.c       | 16 +++++++++-
 src/box/xrow_update_field.c |  2 +-
 src/box/xrow_update_field.h |  4 +++
 test/vinyl/upsert.result    | 64 +++++++++++++++++++++++++++++++++++++
 test/vinyl/upsert.test.lua  | 29 +++++++++++++++++
 5 files changed, 113 insertions(+), 2 deletions(-)

diff --git a/src/box/xrow_update.c b/src/box/xrow_update.c
index 63f7bc3da..0493c0d62 100644
--- a/src/box/xrow_update.c
+++ b/src/box/xrow_update.c
@@ -518,8 +518,22 @@ xrow_upsert_squash(const char *expr1, const char *expr1_end,
 			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], op[0]->arg.arith,
+		if (xrow_update_arith_make(op[1], arith,
 					   &res.arg.arith) != 0)
 			return NULL;
 		res_ops = mp_encode_array(res_ops, 3);
diff --git a/src/box/xrow_update_field.c b/src/box/xrow_update_field.c
index d635e82b9..1095eceda 100644
--- a/src/box/xrow_update_field.c
+++ b/src/box/xrow_update_field.c
@@ -182,7 +182,7 @@ xrow_update_mp_read_uint(struct xrow_update_op *op, const char **expr,
 	return xrow_update_err_arg_type(op, "a positive integer");
 }
 
-static inline int
+int
 xrow_mp_read_arg_arith(struct xrow_update_op *op, const char **expr,
 		       struct xrow_update_arg_arith *ret)
 {
diff --git a/src/box/xrow_update_field.h b/src/box/xrow_update_field.h
index 5a8a79881..193df5882 100644
--- a/src/box/xrow_update_field.h
+++ b/src/box/xrow_update_field.h
@@ -723,6 +723,10 @@ xrow_update_arg_arith_sizeof(const struct xrow_update_arg_arith *arg);
 int
 xrow_update_op_do_arith(struct xrow_update_op *op, const char *old);
 
+int
+xrow_mp_read_arg_arith(struct xrow_update_op *op, const char **expr,
+		       struct xrow_update_arg_arith *ret);
+
 int
 xrow_update_op_do_bit(struct xrow_update_op *op, const char *old);
 
diff --git a/test/vinyl/upsert.result b/test/vinyl/upsert.result
index c807b5b52..3a7f6629d 100644
--- a/test/vinyl/upsert.result
+++ b/test/vinyl/upsert.result
@@ -835,3 +835,67 @@ ch:get() -- should see the UPSERT and return [10, 20]
 s:drop()
 ---
 ...
+-- gh-5106: upsert squash doesn't handle arithmetic operation
+-- applied on the set operation.
+--
+s = box.schema.space.create('test', { engine = 'vinyl'})
+---
+...
+_ = s:create_index('pk')
+---
+...
+s:replace{1, 1}
+---
+- [1, 1]
+...
+box.snapshot()
+---
+- ok
+...
+s:upsert({1, 0}, {{'=', 2, 2}})
+---
+...
+s:upsert({1, 0}, {{'-', 2, 1}})
+---
+...
+box.snapshot()
+---
+- ok
+...
+s:select()
+---
+- - [1, 1]
+...
+for i = 0, 11 do if i%2 == 0 then s:upsert({1, 0}, {{'=', 2, i}}) else s:upsert({1, 0}, {{'+', 2, i}}) end end
+---
+...
+box.snapshot()
+---
+- ok
+...
+s:select()
+---
+- - [1, 21]
+...
+-- Operations won't squash (owing to incompatible types), so
+-- during applying resulting upsert on the top of replace
+-- statement we will get 'double update' error and ignored
+-- second upsert.
+--
+s:upsert({1, 0}, {{'=', 2, 'abc'}})
+---
+...
+s:upsert({1, 0}, {{'-', 2, 1}})
+---
+...
+box.snapshot()
+---
+- ok
+...
+s:select()
+---
+- - [1, 'abc']
+...
+s:drop()
+---
+...
diff --git a/test/vinyl/upsert.test.lua b/test/vinyl/upsert.test.lua
index cf2873606..1d77474da 100644
--- a/test/vinyl/upsert.test.lua
+++ b/test/vinyl/upsert.test.lua
@@ -343,3 +343,32 @@ s:upsert({10, 10}, {{'+', 2, 10}})
 test_run:cmd("setopt delimiter ''");
 ch:get() -- should see the UPSERT and return [10, 20]
 s:drop()
+
+-- gh-5106: upsert squash doesn't handle arithmetic operation
+-- applied on the set operation.
+--
+s = box.schema.space.create('test', { engine = 'vinyl'})
+_ = s:create_index('pk')
+s:replace{1, 1}
+box.snapshot()
+
+s:upsert({1, 0}, {{'=', 2, 2}})
+s:upsert({1, 0}, {{'-', 2, 1}})
+box.snapshot()
+s:select()
+
+for i = 0, 11 do if i%2 == 0 then s:upsert({1, 0}, {{'=', 2, i}}) else s:upsert({1, 0}, {{'+', 2, i}}) end end
+box.snapshot()
+s:select()
+
+-- Operations won't squash (owing to incompatible types), so
+-- during applying resulting upsert on the top of replace
+-- statement we will get 'double update' error and ignored
+-- second upsert.
+--
+s:upsert({1, 0}, {{'=', 2, 'abc'}})
+s:upsert({1, 0}, {{'-', 2, 1}})
+box.snapshot()
+s:select()
+
+s:drop()
-- 
2.17.1

Re: [Tarantool-patches] [PATCH] vinyl: fix squashing set and arithmetic operations

[Vladislav Shpilevoy]

Hi! Thanks for the patch!

LGTM.

Re: [Tarantool-patches] [PATCH] vinyl: fix squashing set and arithmetic operations

[Nikita Pettik]

On 29 Jun 22:19, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patch!
> 
> LGTM.

Pushed to master, 2.4, 2.3 and 1.10. Changelogs are updated correspondingly.