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

[Hollow111 <hollow653@gmail.com>]

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

пт, 20 апр. 2018 г. в 4:02, n.pettik <korablev@tarantool.org>:

>
> > Currently insertion from the table to another one
> > with the same schema using SELECT works wrong.
> > The problem lies in xfer optimization which opens cursors for
> > all of the indexes and inserts data excessively.
> > Now only PRIMARY KEY is used to handle insertion.
> > Moreover analyzing xfer optimization I have noticed
>
> Nitpicking: it is not worth mentioning process of exploration,
> just use (in this particular case) statement of facts.
>
> > diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
> > index ae8dafb..ed134f4 100644
> > --- a/src/box/sql/insert.c
> > +++ b/src/box/sql/insert.c
> > @@ -1711,6 +1711,7 @@ xferOptimization(Parse * pParse,        /* Parser
> context */
> >       ExprList *pEList;       /* The result set of the SELECT */
> >       Table *pSrc;            /* The table in the FROM clause of SELECT
> */
> >       Index *pSrcIdx, *pDestIdx;      /* Source and destination indices
> */
> > +     struct index *src_idx, *dest_idx;       /* Source and destination
> indices */
>
> We don’t use forward var declaration: it is obsolete rule from ancient C
> standards.
> Moreover, we place comments above the code to be commented.
>
> > +     struct space *space;    /* Space pointer for pDest and pSrc */
>
> The same is here.
>
> > -     assert(destHasUniqueIdx);
> > -     if ((pDest->iPKey < 0 && pDest->pIndex != 0)    /* (1) */
> > -         ||destHasUniqueIdx  /* (2) */
> > -         || (onError != ON_CONFLICT_ACTION_ABORT
> > -             && onError != ON_CONFLICT_ACTION_ROLLBACK)      /* (3) */
> > -         ) {
> > -             /* In some circumstances, we are able to run the xfer
> optimization
> > -              * only if the destination table is initially empty.
> > -              * This block generates code to make
> > -              * that determination.
> > -              *
> > -              * Conditions under which the destination must be empty:
> > -              *
> > -              * (1) There is no INTEGER PRIMARY KEY but there are
> indices.
> > -              *
> > -              * (2) The destination has a unique index.  (The xfer
> optimization
> > -              *     is unable to test uniqueness.)
> > -              *
> > -              * (3) onError is something other than
> ON_CONFLICT_ACTION_ABORT and _ROLLBACK.
> > -              */
> > -             addr1 = sqlite3VdbeAddOp2(v, OP_Rewind, iDest, 0);
> > -             VdbeCoverage(v);
> > -             emptyDestTest = sqlite3VdbeAddOp0(v, OP_Goto);
> > -             sqlite3VdbeJumpHere(v, addr1);
> > -     }
>
> Why did you delete if-clause? The code which checks that
> table is empty is needed only in the cases in if-clauses,
> i.e. in some cases xfer optimisation can be applied even if
> table is not initially empty (as far as I understand).
>

Yes, that's true this code is needed if any of the
cases in if-clause != 0. But the point is that in Tarantool
there's always a PRIMARY KEY that is obviously unique
which means that the second case is always != 0 and
the result of the boolean expression is always not 0,
hence there's no reason for this if-check.


> > +     /* The xfer optimization is unable to test uniqueness
> > +      * while we have a unique PRIMARY KEY in any existing table.
> > +      * This is the reason we can only run it if the destination table
>
> Nitpicking: comments should fit into 66 chars.
>
> > +      * is initially empty.
> > +      * This block generates code to make that determination.
> > +      */
> > +     addr1 = sqlite3VdbeAddOp2(v, OP_Rewind, iDest, 0);
> > +     VdbeCoverage(v);
> > +     emptyDestTest = sqlite3VdbeAddOp0(v, OP_Goto);
> > +     sqlite3VdbeJumpHere(v, addr1);
> > +
> > +     space = space_by_id(SQLITE_PAGENO_TO_SPACEID(pDest->tnum));
> > +     dest_idx = space_index(space, 0 /* PK */);
> > +     space = space_by_id(SQLITE_PAGENO_TO_SPACEID(pSrc->tnum));
> > +     src_idx = space_index(space, 0 /* PK */);
> > +     assert(src_idx);
> > +     assert(dest_idx);
>
> We use explicit != NULL checks.
>
> > +     pDestIdx = sqlite3PrimaryKeyIndex(pDest);
> > +     pSrcIdx = sqlite3PrimaryKeyIndex(pSrc);
>
> In fact, you don’t need these indexes at all.
> Instead of calling emit_open_cursor() you
> can simply add two opcodes: OP_LoadPtr and OP_OpenWrite
> (as it happens inside that function). Or, you can use pSrc->tnum.
> Table’s tnum exactly encodes PK space id (index id is 0).
> As for KeyInfo - you are able to remove it at all, since opcodes
> below don’t rely on key info (AFAIK, better check it yourself).
> The only issue remained - how to check PK compatibility.
> I would add to xferCompatibleIndex() another one check:
> If first index is PK, then second also must be PK.
> It is easy to implement, since Index struct contains appropriate flag.
>
>

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