[tarantool-patches] Re: [PATCH] sql: xfer optimization issue

[Nikita Tatunov <hollow653@gmail.com>]

Hello! Thanks for review, Diff is in the end!

вт, 31 июл. 2018 г. в 16:29, Alexander Turenko
<alexander.turenko@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.
>

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, 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.
>

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 = 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)
+    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,
+    return test:is(after - before, exp_xfer_count,
                    test_name .. '-xfer-count')
 end
- return ok
-end

 test.do_execsql_xfer_test = 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 = 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)
+    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,
+    return test:is(after - before, exp_xfer_count,
                    test_name .. '-xfer-count')
 end
- return ok
-end

 test.do_execsql_xfer_test = function(test, test_name, func, exp, opts)
     return do_xfer_test(test, test.do_execsql_test, test_name, func, exp, opts)