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 0E09D284BE for ; Tue, 31 Jul 2018 13:04:47 -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 uzLcVE9IKJLd for ; Tue, 31 Jul 2018 13:04:46 -0400 (EDT) Received: from mail-lf1-f65.google.com (mail-lf1-f65.google.com [209.85.167.65]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id AE552284BC for ; Tue, 31 Jul 2018 13:04:46 -0400 (EDT) Received: by mail-lf1-f65.google.com with SMTP id g6-v6so11262552lfb.11 for ; Tue, 31 Jul 2018 10:04:46 -0700 (PDT) MIME-Version: 1.0 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> <20180731132935.7tt5m23wntizv3vn@tkn_work_nb> In-Reply-To: <20180731132935.7tt5m23wntizv3vn@tkn_work_nb> From: Nikita Tatunov Date: Tue, 31 Jul 2018 20:04:32 +0300 Message-ID: Subject: [tarantool-patches] Re: [PATCH] sql: xfer optimization issue Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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: Alexander Turenko Cc: tarantool-patches@freelists.org, korablev@tarantool.org Hello! Thanks for review, Diff is in the end! =D0=B2=D1=82, 31 =D0=B8=D1=8E=D0=BB. 2018 =D0=B3. =D0=B2 16:29, Alexander T= urenko : > > 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 =3D=3D ON_CONFLICT_ACTION_ABORT && > > > > + !is_err_action_default)) { > > > > > > Do you mean that: > > > > > > 1. The optimization non-empty table case correctly works only with AB= ORT > > > 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 mo= re > > > 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 rem= ove > > 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 somet= hing > > > 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 e= mpty. > > 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. > Yes, I'm on it. > > 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. > Fixed. > > +local function do_xfer_test(test, test_func, test_name, func, exp, opt= s) > > + local opts =3D opts or {} > > + local exp_xfer_count =3D opts.exp_xfer_count > > + local before =3D box.sql.debug().sql_xfer_count > > + local ok, result =3D pcall(test_func, test, test_name, func, exp) > > + local after =3D box.sql.debug().sql_xfer_count > > + if exp_xfer_count ~=3D nil then > > + ok =3D 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. > Fixed. > By the way, we have 4 spaces indent for Lua accorsing to Lua Style > Guide. Fixed. diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c index c3cf94a..041601c 100644 --- a/src/box/sql/insert.c +++ b/src/box/sql/insert.c @@ -1908,10 +1908,10 @@ xferOptimization(Parse * pParse, /* Parser context = */ sqlite3VdbeAddOp2(v, OP_IdxInsert, iDest, regData); switch (onError) { case ON_CONFLICT_ACTION_IGNORE: - sqlite3VdbeChangeP5(v, OPFLAG_OE_IGNORE); + sqlite3VdbeChangeP5(v, OPFLAG_OE_IGNORE | OPFLAG_NCHANGE); break; case ON_CONFLICT_ACTION_FAIL: - sqlite3VdbeChangeP5(v, OPFLAG_OE_FAIL); + sqlite3VdbeChangeP5(v, OPFLAG_OE_FAIL | OPFLAG_NCHANGE); break; default: sqlite3VdbeChangeP5(v, OPFLAG_NCHANGE); diff --git a/test/sql-tap/gh-3307-xfer-optimization-issue.test.lua b/test/sql-tap/gh-3307-xfer-optimization-issue.test.lua index b99de2b..7778fce 100755 --- a/test/sql-tap/gh-3307-xfer-optimization-issue.test.lua +++ b/test/sql-tap/gh-3307-xfer-optimization-issue.test.lua @@ -6,14 +6,11 @@ local function do_xfer_test(test, test_func, test_name, func, exp, opts) local opts =3D opts or {} local exp_xfer_count =3D opts.exp_xfer_count local before =3D box.sql.debug().sql_xfer_count - local ok, result =3D pcall(test_func, test, test_name, func, exp) + test_func(test, test_name, func, exp) local after =3D box.sql.debug().sql_xfer_count - if exp_xfer_count ~=3D nil then - ok =3D ok and test:is(after - before, exp_xfer_count, + return test:is(after - before, exp_xfer_count, test_name .. '-xfer-count') end - return ok -end test.do_execsql_xfer_test =3D function(test, test_name, func, exp, opts) return do_xfer_test(test, test.do_execsql_test, test_name, func, exp, = opts) diff --git a/test/sql-tap/with2.test.lua b/test/sql-tap/with2.test.lua index bd0187f..a59bac7 100755 --- a/test/sql-tap/with2.test.lua +++ b/test/sql-tap/with2.test.lua @@ -390,14 +390,11 @@ local function do_xfer_test(test, test_func, test_name, func, exp, opts) local opts =3D opts or {} local exp_xfer_count =3D opts.exp_xfer_count local before =3D box.sql.debug().sql_xfer_count - local ok, result =3D pcall(test_func, test, test_name, func, exp) + test_func(test, test_name, func, exp) local after =3D box.sql.debug().sql_xfer_count - if exp_xfer_count ~=3D nil then - ok =3D ok and test:is(after - before, exp_xfer_count, + return test:is(after - before, exp_xfer_count, test_name .. '-xfer-count') end - return ok -end test.do_execsql_xfer_test =3D function(test, test_name, func, exp, opts) return do_xfer_test(test, test.do_execsql_test, test_name, func, exp, = opts)