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

[Alexander Turenko]

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.