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

[Alexander Turenko]

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

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?

How your changes to xferCompatibleIndex and empty check enabling
condition is motivated? It is hard to understand it without more
detailed description.

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