Re: [Tarantool-patches] [PATCH] vinyl: add NULL check of xrow_upsert_execute() retval

[Nikita Pettik <korablev@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.