From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp61.i.mail.ru (smtp61.i.mail.ru [217.69.128.41]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 3EA2D4429E1 for ; Sat, 27 Jun 2020 21:46:39 +0300 (MSK) From: Nikita Pettik Date: Sat, 27 Jun 2020 21:46:36 +0300 Message-Id: <8c9a820a302e6e206305d85719808c1bf73c0188.1593265600.git.korablev@tarantool.org> Subject: [Tarantool-patches] [PATCH] vinyl: fix squashing set and arithmetic operations List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: tarantool-patches@dev.tarantool.org Cc: v.shpilevoy@tarantool.org 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