From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <korablev@tarantool.org>
Received: from smtp49.i.mail.ru (smtp49.i.mail.ru [94.100.177.109])
 (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 5B0A8445320
 for <tarantool-patches@dev.tarantool.org>;
 Thu,  9 Jul 2020 14:56:30 +0300 (MSK)
Date: Thu, 9 Jul 2020 11:56:29 +0000
From: Nikita Pettik <korablev@tarantool.org>
Message-ID: <20200709115629.GB814@tarantool.org>
References: <776e8b91b93c79dabd2932b5d665236c5da313c8.1590546551.git.korablev@tarantool.org>
 <7445b38c-f664-ca79-bb05-73a73ddc4d6d@tarantool.org>
 <20200708125327.GE26461@tarantool.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20200708125327.GE26461@tarantool.org>
Subject: Re: [Tarantool-patches] [PATCH] vinyl: add NULL check of
 xrow_upsert_execute() retval
List-Id: Tarantool development patches <tarantool-patches.dev.tarantool.org>
List-Unsubscribe: <https://lists.tarantool.org/mailman/options/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=unsubscribe>
List-Archive: <https://lists.tarantool.org/pipermail/tarantool-patches/>
List-Post: <mailto:tarantool-patches@dev.tarantool.org>
List-Help: <mailto:tarantool-patches-request@dev.tarantool.org?subject=help>
List-Subscribe: <https://lists.tarantool.org/mailman/listinfo/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=subscribe>
To: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org

On 08 Jul 12:53, Nikita Pettik wrote:
> On 29 May 23:24, Vladislav Shpilevoy wrote:
> > Hi! Thanks for the patch!
> > 
> > While the patch is obviously correct (we need to check NULL
> > for sure), it solves the problem only partially, and creates
> > another.
> 
> Okay, I suggest following. Let's push patch with minor test
> changes as is (in scope of 1.10.7 release), but leave issue open.
> As a result, we will get rid of crash, and postpone a bit
> reconsideration of upsert application till 1.10.8.
> We are going to rework upsert (according to the plan defined in #5107
> https://github.com/tarantool/tarantool/issues/5107). Here's changed
> test (https://github.com/tarantool/tarantool/tree/np/gh-4957-master):
> 
> diff --git a/test/vinyl/gh-4957-too-many-upserts.test.lua b/test/vinyl/gh-4957-too-many-upserts.test.lua
> new file mode 100644
> index 000000000..e5adfe41c
> --- /dev/null
> +++ b/test/vinyl/gh-4957-too-many-upserts.test.lua
> @@ -0,0 +1,38 @@
> +s = box.schema.create_space('test', {engine = 'vinyl'})
> +pk = s:create_index('pk')
> +s:insert{1, 1}
> +box.snapshot()
> +
> +-- Let's test number of upserts in one transaction that exceeds
> +-- the limit of operations allowed in one update.
> +--
> +ups_cnt = 5000
> +box.begin()
> +for i = 1, ups_cnt do s:upsert({1}, {{'&', 2, 1}}) end
> +box.commit()
> +-- Upserts are not able to squash, so scheduler will get stuck.
> +-- So let's not waste much time here, just check that no crash
> +-- takes place.
> +--
> +box.snapshot()
> +
> +fiber = require('fiber')
> +fiber.sleep(0.01)
> +
> +s:drop()
> +
> +s = box.schema.create_space('test', {engine = 'vinyl'})
> +pk = s:create_index('pk')
> +
> +tuple = {}
> +for i = 1, ups_cnt do tuple[i] = i end
> +_ = s:insert(tuple)
> +box.snapshot()
> +
> +box.begin()
> +for k = 1, ups_cnt do s:upsert({1}, {{'+', k, 1}}) end
> +box.commit()
> +box.snapshot()
> +fiber.sleep(0.01)
> +
> 
> Are you guys okay with this suggestion?
>

Pushed to master, 2.4, 2.3 and 1.10. Branch is dropped, changelogs are
updated correspondingly. Also I had to slightly modify test for 2.4, 2.3
and 1.10 versions, since we have to unthrottle scheduler manually to
process snapshot. As a result test has become release disabled.