From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 40B892811D for ; Tue, 31 Jul 2018 09:29:38 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id Gb5sTJ2boDle for ; Tue, 31 Jul 2018 09:29:38 -0400 (EDT) Received: from smtp58.i.mail.ru (smtp58.i.mail.ru [217.69.128.38]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id F312D280DB for ; Tue, 31 Jul 2018 09:29:37 -0400 (EDT) Date: Tue, 31 Jul 2018 16:29:36 +0300 From: Alexander Turenko Subject: [tarantool-patches] Re: [PATCH] sql: xfer optimization issue Message-ID: <20180731132935.7tt5m23wntizv3vn@tkn_work_nb> References: <605B15EF-BD1C-4B03-8A9F-6E6225076812@tarantool.org> <12B62C73-9BEC-49FA-B3FD-590C445CF25B@tarantool.org> <2123605D-8D6C-43A3-846F-735E4C2C7FC2@tarantool.org> <20180729011251.eitp7cisv6jv5opj@tkn_work_nb> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: Nikita Tatunov Cc: tarantool-patches@freelists.org, korablev@tarantool.org Hi! Thanks for updates. Answered inline. WBR, Alexander Turenko. > > > + /* > > > + * Xfer optimization is unable to correctly insert data > > > + * in case there's a conflict action other than > > > + * explicit *_ABORT. This is the reason we want to only > > > + * run it if the destination table is initially empty. > > > + * That block generates code to make that determination. > > > + */ > > > + if (!(onError == ON_CONFLICT_ACTION_ABORT && > > > + !is_err_action_default)) { > > > > Do you mean that: > > > > 1. The optimization non-empty table case correctly works only with ABORT > > conflict action (default or explicit). > > 2. The default conflict action can be overwritten on per-column basis, so > > 'default abort' can really be replace or other conflict action. > > > > If so, your description doesn't give this information. > > > > To more on that, can we check per-column conflict actions instead of check > > whether the conflict action default or explicit? This would enable more > > cases > > with non-empty tables for the optimization. And this would look less > > confusing, > > IMHO. > > > > Well, basically, you're right. But the thing is that we're going to remove > column conflict actions, thus, I suppose, it doesn't make sense. > Nikita, Correct me if I'm wrong please. I recommend to comment such things in the code or commit message (as you think it is more appropriate). It allows to decrease review iterations at least :) I don't insist, though. > > I have one more question on that. It seems that SQLite has this > > optimization > > working with ROLLBACK conflict action. We cannot doing so, because of some > > problem? Did this problem described / trackerized somewhere? Or something > > changes underhood and makes this impossible? Are we know exact reason or > > just > > observing it does not work? > > > > I investigated that a little. OP_IdxInsert was changed so that we can > process > ABORT, FAIL or IGNORE. I took it into account in newer version hence > it is also used in these cases even when the destination table is not empty. So I understand it as follows: sqlite3's OP_IdxInsert just copy rows and cannot handle non-trivial conflict actions, but tarantool's OP_IdxInsert can handle it. However we have some non-investigated problems with ROLLBACK and REPLACE as well as per-column conflict actions. I think that is is worth to investigate it a little deeper and file issue(s), but this is not the blocker for this patch. > sqlite3VdbeAddOp2(v, OP_IdxInsert, iDest, regData); > - sqlite3VdbeChangeP5(v, OPFLAG_NCHANGE); > + switch (onError) { > + case ON_CONFLICT_ACTION_IGNORE: > + sqlite3VdbeChangeP5(v, OPFLAG_OE_IGNORE); > + break; > + case ON_CONFLICT_ACTION_FAIL: > + sqlite3VdbeChangeP5(v, OPFLAG_OE_FAIL); > + break; > + default: > + sqlite3VdbeChangeP5(v, OPFLAG_NCHANGE); > + break; > + } OPFLAG_NCHANGE is independent with other flags I think. > +local function do_xfer_test(test, test_func, test_name, func, exp, opts) > + local opts = opts or {} > + local exp_xfer_count = opts.exp_xfer_count > + local before = box.sql.debug().sql_xfer_count > + local ok, result = pcall(test_func, test, test_name, func, exp) > + local after = box.sql.debug().sql_xfer_count > + if exp_xfer_count ~= nil then > + ok = ok and test:is(after - before, exp_xfer_count, > + test_name .. '-xfer-count') > + end > + return ok > end I missed that do_execsql_test don't return a result and do_catchsql_test returns {0} or {1}. So just don't same `ok` and `result` and remove pcall. By the way, we have 4 spaces indent for Lua accorsing to Lua Style Guide.