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

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

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.

We discussed that verbally, and here is a short resume of what
is happening in the patch, and where we have a tricky problem:
if there are 2 perfectly valid upserts, each with 2.5k operations,
and they are merged into one, both of them are skipped, because
after merge they become too fat - opcount > 4k.

It looks at first that this can only happen when field count > 4k,
because otherwise all the operations would be squashed into something
smaller or equal than field count, but it is not. There are a few
cases, when even after squash total operation count will be bigger
than field count:

1) operations are complex - ':', '&', '|', '^', '#', '!'. The last
two operations are actually used by people. These operations are not
squashed. The last one - '!' - can't be squashed even in theory.

2) operations have negative field number. For example, {'=', -1, ...} -
assign a value to the last field in the tuple. But honestly I don't
remember. Perhaps they are merged, if in both squashed upserts the
field number is the same. But imagine this: {'=', -1, 100}, and
{'=', 5, 100}. They look different, but if the tuple has only 5 fields,
they operate on the same field.

That means it is not safe to drop any upsert having more than 4k
operations. Because it can consist of many small valid upserts.

I don't know how to fix it in a simple way. The only thing I could
come up with is probably don't squash such fat upserts. Just keep
them all on the disk, until they eventually meet bottom of their key,
or a terminal statement like REPLACE/INSERT/DELETE.

This is not only about disk, btw. 2 fat upserts could be inserted into
the memory level, turn into an invalid upsert, and that will be skipped.

Here is a test. Create a tuple, and dump it on disk so as it would
disappear from the memory level and from the cache:

	box.cfg{}
	s = box.schema.create_space('test', {engine = 'vinyl'})
	pk = s:create_index('pk')
	s:insert({1, 1})
	box.snapshot()

Then restart (to ensure the cache is clear), and create 2 upserts:

	box.cfg{}
	s = box.space.test
	ops = {}
	op = {'=', 2, 100}
	for i = 1, 2500 do table.insert(ops, op) end
	s:upsert({1}, ops)
	op = {'=', -1, 200}
	ops = {}
	for i = 1, 2500 do table.insert(ops, op) end
	s:upsert({1}, ops)

Now if I do select, I get

	tarantool> s:select{}
	---
	- - [1, 200]
	...

But if I do dump + select, I get:

	tarantool> box.snapshot()
	---
	- ok
	...

	tarantool> s:select{}
	---
	- - [1, 100]
	...

During dump the second upsert was skipped even though it was valid.