From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp51.i.mail.ru (smtp51.i.mail.ru [94.100.177.111]) (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 0ACA8469710 for ; Sat, 30 May 2020 00:24:28 +0300 (MSK) References: <776e8b91b93c79dabd2932b5d665236c5da313c8.1590546551.git.korablev@tarantool.org> From: Vladislav Shpilevoy Message-ID: <7445b38c-f664-ca79-bb05-73a73ddc4d6d@tarantool.org> Date: Fri, 29 May 2020 23:24:26 +0200 MIME-Version: 1.0 In-Reply-To: <776e8b91b93c79dabd2932b5d665236c5da313c8.1590546551.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: add NULL check of xrow_upsert_execute() retval 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! 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.