From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (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 0C8C341C5DB for ; Sat, 20 Jun 2020 20:32:29 +0300 (MSK) References: <7c98fabb09bef52546b592ce5603e7e69b3ffca1.1592480615.git.korablev@tarantool.org> From: Vladislav Shpilevoy Message-ID: <80cedca6-159a-6c6a-6970-9891cdfa0c9c@tarantool.org> Date: Sat, 20 Jun 2020 19:32:27 +0200 MIME-Version: 1.0 In-Reply-To: <7c98fabb09bef52546b592ce5603e7e69b3ffca1.1592480615.git.korablev@tarantool.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH] vinyl: check upsert doesn't modify PK right after execution List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Nikita Pettik , tarantool-patches@dev.tarantool.org Hi! Thanks for the patch! See 2 comments below. On 18/06/2020 14:10, Nikita Pettik wrote: > vy_upsert_apply() consists of two parts: execution and squash. > Assume we have two upserts passing to vy_apply_upsert(): > s1(key1, ops1) (old one) and s2(key1, ops2) (new one). > To execute upsert s2 we fetch key1 from s1 and attempt at applying ops2 > on key1. Note that in case of memtx engine key1 is always whole tuple > fetched from index. Meanwhile in vinyl for the sake of performance we > don't seek tuple in index. As a result key1 can contain less fields than > the whole tuple stored in index. So when passing -1 as field index of > upsert operation, tuple update routine may modify key value. > For instance: > > s:insert{1, 1} -- stored on disk; pk is the first field > s:upsert({1}, {{'+', 2, 200}}) -- s1 > s:upsert({1}, {{'=', -1, 100}}) -- s2 > > In this case {'=', -1, 100} operation is applied to {1} key. > Result of operation is {100} - in other words - primary key has been > modified. As far as upserts modifying PK are ignored, this upsert is > skipped. > > To resolve this problem let's check if PK is changed right after > tuple execution procedure. If it is, 'rollback' effects of execution and > continue processing upserts squash using unmodified key: > > squash(s1, s2) --> s3(key1, {ops1, ops2}) > > The only drawback of this approach is that if one of upserts *really* > modifies PK of tuple stored on disk, all squashed operations will be > skipped as well. For example: > > s:upsert({1}, {{'+', 2, 100}}) > s:upsert({1}, {{'=', -2, 200}}) > > These two upserts are squashed into one: ({1}, {{'+', 2, 100}, {'=', -2, 200}} > The latter fails to be applied so neither of upserts will be executed > (despite the fact that the first one is absolutely OK). > > Closes #5087 > --- > Branch: https://github.com/tarantool/tarantool/tree/np/gh-5087-upsert-negative-index > Issue: https://github.com/tarantool/tarantool/issues/5087 > > src/box/vy_upsert.c | 67 +++++---- > .../gh-5087-upsert-negative-fieldno.result | 131 ++++++++++++++++++ > .../gh-5087-upsert-negative-fieldno.test.lua | 48 +++++++ > 3 files changed, 221 insertions(+), 25 deletions(-) > create mode 100644 test/vinyl/gh-5087-upsert-negative-fieldno.result > create mode 100644 test/vinyl/gh-5087-upsert-negative-fieldno.test.lua > > diff --git a/src/box/vy_upsert.c b/src/box/vy_upsert.c > index 007921bb2..210b7e3a4 100644 > --- a/src/box/vy_upsert.c > +++ b/src/box/vy_upsert.c > @@ -118,33 +118,63 @@ vy_apply_upsert(const struct tuple *new_stmt, const struct tuple *old_stmt, > /* > * Apply new operations to the old stmt > */ > - const char *result_mp; > + const char *old_mp = NULL; > + uint32_t old_mp_size = 0; > if (vy_stmt_type(old_stmt) == IPROTO_UPSERT) > - result_mp = vy_upsert_data_range(old_stmt, &mp_size); > + old_mp = vy_upsert_data_range(old_stmt, &old_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; > + old_mp = tuple_data_range(old_stmt, &old_mp_size); > + const char *old_mp_end = old_mp + old_mp_size; > 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 = tuple_upsert_execute(vy_update_alloc, region, new_ops, > - new_ops_end, result_mp, result_mp_end, > - &mp_size, 0, suppress_error, > - &column_mask); > + uint32_t result_mp_size = 0; > + const char *result_mp = > + tuple_upsert_execute(vy_update_alloc, region, new_ops, > + new_ops_end, old_mp, old_mp_end, > + &result_mp_size, 0, suppress_error, > + &column_mask); > if (result_mp == NULL) { > region_truncate(region, region_svp); > return NULL; > } > - result_mp_end = result_mp + mp_size; > + > + const char *result_mp_end = result_mp + result_mp_size; > + /* > + * tuple_upsert_execute() may modify PK field. > + * So before moving to further upsert processing, let's > + * check if it's true. If so, rollback *_execute effects > + * and ignore this UPSERT. > + */ > + if (unlikely(!key_update_can_be_skipped(cmp_def->column_mask, > + column_mask))) { > + if (vy_tuple_compare_with_raw_key(old_stmt, result_mp, > + cmp_def) != 0) { 1. You probably wanted to compare two tuples, not tuple and a key. Because result_mp is not a key. I wrote a test on it which works differently on memtx and vinyl, uses only valid upserts. box.cfg{} engine = 'vinyl' s = box.schema.create_space('test', {engine = engine}) pk = s:create_index('pk', {parts = {{2, 'unsigned'}}}) s:insert({1, 1, 1}) box.snapshot() os.exit() box.cfg{} s = box.space.test s:upsert({1, 1}, {{'=', 1, 100}}) s:upsert({2, 1}, {{'=', 2, 1}}) box.snapshot() s:select() Vinyl output: --- - - [1, 1, 1] ... Change engine to 'memtx': --- - - [100, 1, 1] ... In some cases it may crash, if tuple_compare_with_key_slowpath would be used, because it validates, that part_count <= key_def->part_count in an assertion. > + result_mp = old_mp; > + result_mp_end = old_mp_end; > + region_truncate(region, region_svp); > + if (!suppress_error) { > + say_error("UPSERT operation failed: " > + "primary key is not allowed to be " > + "modified"); 2. The error is printed even when the upserts are perfectly valid, like in the ticket. I don't think it is ok.