[tarantool-patches] Re: [PATCH] sql: xfer optimization issue
Hollow111
hollow653 at gmail.com
Fri Apr 20 18:09:57 MSK 2018
пт, 20 апр. 2018 г. в 4:02, n.pettik <korablev at 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.
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.tarantool.org/pipermail/tarantool-patches/attachments/20180420/132ea5d6/attachment.html>
More information about the Tarantool-patches
mailing list