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

[Nikita Tatunov <hollow653@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3383 bytes --]

пн, 9 июл. 2018 г. в 18:50, Alexander Turenko <
alexander.turenko@tarantool.org>:

> Hi Nikita!
>
> Please, consider comments below.
>
> WBR, Alexander Turenko.
>
> > +test:do_execsql_test(
> > +       "xfer-oprimization-1.8",
> > ...
> > +test:do_execsql_test(
> > +       "xfer-oprimization-1.10",
> > ...
> > +test:do_execsql_test(
> > +       "xfer-oprimization-1.12",
> > ...
> > +test:do_execsql_test(
> > +       "xfer-oprimization-1.16",
> > +test:do_execsql_test(
> > +       "xfer-oprimization-1.18",
>
> oprimization -> optimization
>

Is already fixed.


> It seems that review questions from the last Nikita email in the thread
> was not fixed or answered (at least some of them).
>
> > +       sqlite3VdbeAddOp3(v, OP_OpenWrite, iSrc,
> > +                         pSrc->tnum, space_ptr_reg);
>
> Why do you open source space for write?
>
>
I changed it to 'vdbe_emit_open_cursor' which now is used everywhere for
both reading and writing (internally it uses 'OP_OpenWrite')

How your changes to xferCompatibleIndex and empty check enabling
> condition is motivated? It is hard to understand it without more
> detailed description.
>
>
We open cursors on PK for both tables, additional check that we have opened
'em on PK isn't redundunt.
What's about empty check enabling: I only want to add some cases where we
don't demand empty destination table for xferOptimization to be used. Thus
it will be used a little bit more frequently (it's faster than inserting
non-raw data for about 13%).


> ---
> EOF.
>
> On Thu, Jun 28, 2018 at 01:18:39PM +0300, Alexander Turenko wrote:
> > On Fri, May 04, 2018 at 12:54:30PM +0000, Hollow111 wrote:
> > >    > @@ -1737,8 +1744,10 @@ xferOptimization(Parse * pParse,       /*
> > >    Parser context */
> > >    >       if (onError == ON_CONFLICT_ACTION_DEFAULT) {
> > >    >               if (pDest->iPKey >= 0)
> > >    >                       onError = pDest->keyConf;
> > >    > -             if (onError == ON_CONFLICT_ACTION_DEFAULT)
> > >    > +             if (onError == ON_CONFLICT_ACTION_DEFAULT) {
> > >    >                       onError = ON_CONFLICT_ACTION_ABORT;
> > >    > +                     confl_action_default = 1;
> > >    Why do you need this variable at all? I mean, DEFAULT always
> > >    is an alias to ABORT, isn’t it?
> > >    Yes, it is, but there's a little difference between directly
> specified
> > >    ABORT for an
> > >    insert stmt (INSERT OR ABORT) and just INSERT without any specified
> > >    error action
> > >    (ABORT specified by the internals). When you directly specify it
> ABORT
> > >    is a higher priority
> > >    action than in case there's a column with REPLACE error action.
> Thus we
> > >    can even insert
> > >    not in the empty destination table.
> >
> > If an user asks for ABORT explicitly we should make abort, I think.
> >
> > As I understood the extra variable appears due to the fact than we can
> > have per-column conflict clauses in CREATE TABLE and per-table clause
> > with INSERT OR ABORT. The latter should have precedence, I think.
> >
> > I don't sure whether something (behaviour? code?) should be different
> > from SQLite here in light of #2963 changes. Kirill, Nikita, can you
> > comment, please?
> >
> > WBR, Alexander Turenko.
> >
>

[-- Attachment #2: Type: text/html, Size: 4715 bytes --]