Re: [Tarantool-patches] [PATCH V2] vinyl: rework upsert operation

[Nikita Pettik <korablev@tarantool.org>]

On 12 Aug 23:23, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patch!
> 
> > Closes #5105
> > ---
> > Issues:
> > https://github.com/tarantool/tarantool/issues/5105
> > 
> > @ChangeLog:
> >    - upserts now follow associative property: result of several upserts
> >      doesn't depend on the order of their application (gh-5105);
> 
> In my previous review I tried to explain that any squash now won't work, if we want
> to fix 5105. It does not matter if you check for type match. Because still there are
> overflows. Consider this test:
> 
> 	box.cfg{}
> 	format = {}
> 	format[1] = {name = 'f1', type = 'unsigned'}
> 	format[2] = {name = 'f2', type = 'unsigned'}
> 	s = box.schema.space.create('test', {engine = 'vinyl', format = format})
> 	_ = s:create_index('pk')
> 	uint_max = 18446744073709551615ULL
> 	s:replace{1, uint_max - 2}
> 	box.snapshot()
> 
> 	s:upsert({1, 0}, {{'+', 2, 1}})
> 	s:upsert({1, 0}, {{'+', 2, 1}})
> 	s:upsert({1, 0}, {{'+', 2, 1}})
> 	box.snapshot()
> 	s:select()
> 
> 	---
> 	- - [1, 18446744073709551614]
> 	...
> 
> Last 2 upserts were ignored. Even though only the last one should have been.
> Indeed, if I comment it out, the result becomes correct:

Hi, Vlad. Sorry for quite late response. Turns out that you are right and
upsert squashing procedure (at least in its current state) seems to be
invalid if we don't want to break associativity property (#5105 issue).
So I've removed this optimization in v3 patch. 
 
> 	box.cfg{}
> 	format = {}
> 	format[1] = {name = 'f1', type = 'unsigned'}
> 	format[2] = {name = 'f2', type = 'unsigned'}
> 	s = box.schema.space.create('test', {engine = 'vinyl', format = format})
> 	_ = s:create_index('pk')
> 	uint_max = 18446744073709551615ULL
> 	s:replace{1, uint_max - 2}
> 	box.snapshot()
> 
> 	s:upsert({1, 0}, {{'+', 2, 1}})
> 	s:upsert({1, 0}, {{'+', 2, 1}})
> 	-- s:upsert({1, 0}, {{'+', 2, 1}})
> 	box.snapshot()
> 	s:select()
> 
> 	---
> 	- - [1, 18446744073709551615]
> 	...
> 
> Also I tried to make it depend on order, not on being applied or not. And
> used this test:
> 
> 	box.cfg{}
> 	format = {}
> 	format[1] = {name = 'f1', type = 'unsigned'}
> 	format[2] = {name = 'f2', type = 'unsigned'}
> 	s = box.schema.space.create('test', {engine = 'vinyl', format = format})
> 	_ = s:create_index('pk')
> 	uint_max = 18446744073709551615ULL
> 	s:replace{1, uint_max - 2, 0}
> 	box.snapshot()
> 
> 	s:upsert({1, 0, 0}, {{'+', 2, 1}})
> 	s:upsert({1, 0, 0}, {{'+', 2, 1}})
> 	-- I wanted to check, if +0.5 will prevent squashing, because
> 	-- it does not satisfy format. And that if I make it after all
> 	-- the other upserts, the bug will appear again.
> 	s:upsert({1, 0, 0}, {{'+', 2, 0.5}})
> 	s:upsert({1, 0, 0}, {{'+', 2, 1}})
> 	box.snapshot()
> 	s:select()
> 
> But got another bug instead:
> 
> 	main/103/interactive I> vinyl checkpoint started
> 	main/105/checkpoint_daemon I> scheduled next checkpoint for Thu Aug 13 00:52:21 2020
> 	main/107/vinyl.scheduler I> 512/0: dump started
> 	vinyl.dump.0/103/task vy_stmt.c:174 E> ER_VINYL_MAX_TUPLE_SIZE: Failed to allocate 1347440769 bytes for tuple: tuple is too large. Check 'vinyl_max_tuple_size' configuration option.
> 	main/107/vinyl.scheduler vy_stmt.c:174 E> ER_VINYL_MAX_TUPLE_SIZE: Failed to allocate 1347440769 bytes for tuple: tuple is too large. Check 'vinyl_max_tuple_size' configuration option.
> 	main/107/vinyl.scheduler vy_scheduler.c:1292 E> 512/0: dump failed
> 	main/107/vinyl.scheduler vy_scheduler.c:2058 W> throttling scheduler for 1 second(s)
> 	snapshot/101/main I> saving snapshot `./00000000000000000008.snap.inprogress'
> 	snapshot/101/main I> done
> 	main/103/interactive vy_scheduler.c:740 E> vinyl checkpoint failed: Failed to allocate 1347440769 bytes for tuple: tuple is too large. Check 'vinyl_max_tuple_size' configuration option.
> 	---
> 	- error: 'Failed to allocate 1347440769 bytes for tuple: tuple is too large. Check
> 	    ''vinyl_max_tuple_size'' configuration option.'
> 	...
> 
> Seems the tuple is corrupted somewhere, because a few numbers clearly can't
> occupy 1347440769 bytes.