<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">пт, 20 апр. 2018 г. в 4:02, n.pettik <<a href="mailto:korablev@tarantool.org">korablev@tarantool.org</a>>:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
> Currently insertion from the table to another one<br>
> with the same schema using SELECT works wrong.<br>
> The problem lies in xfer optimization which opens cursors for<br>
> all of the indexes and inserts data excessively.<br>
> Now only PRIMARY KEY is used to handle insertion.<br>
> Moreover analyzing xfer optimization I have noticed<br>
<br>
Nitpicking: it is not worth mentioning process of exploration,<br>
just use (in this particular case) statement of facts.<br>
<br>
> diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c<br>
> index ae8dafb..ed134f4 100644<br>
> --- a/src/box/sql/insert.c<br>
> +++ b/src/box/sql/insert.c<br>
> @@ -1711,6 +1711,7 @@ xferOptimization(Parse * pParse,        /* Parser context */<br>
>       ExprList *pEList;       /* The result set of the SELECT */<br>
>       Table *pSrc;            /* The table in the FROM clause of SELECT */<br>
>       Index *pSrcIdx, *pDestIdx;      /* Source and destination indices */<br>
> +     struct index *src_idx, *dest_idx;       /* Source and destination indices */<br>
<br>
We don’t use forward var declaration: it is obsolete rule from ancient C standards.<br>
Moreover, we place comments above the code to be commented.<br>
<br>
> +     struct space *space;    /* Space pointer for pDest and pSrc */<br>
<br>
The same is here.<br>
<br>
> -     assert(destHasUniqueIdx);<br>
> -     if ((pDest->iPKey < 0 && pDest->pIndex != 0)    /* (1) */<br>
> -         ||destHasUniqueIdx  /* (2) */<br>
> -         || (onError != ON_CONFLICT_ACTION_ABORT<br>
> -             && onError != ON_CONFLICT_ACTION_ROLLBACK)      /* (3) */<br>
> -         ) {<br>
> -             /* In some circumstances, we are able to run the xfer optimization<br>
> -              * only if the destination table is initially empty.<br>
> -              * This block generates code to make<br>
> -              * that determination.<br>
> -              *<br>
> -              * Conditions under which the destination must be empty:<br>
> -              *<br>
> -              * (1) There is no INTEGER PRIMARY KEY but there are indices.<br>
> -              *<br>
> -              * (2) The destination has a unique index.  (The xfer optimization<br>
> -              *     is unable to test uniqueness.)<br>
> -              *<br>
> -              * (3) onError is something other than ON_CONFLICT_ACTION_ABORT and _ROLLBACK.<br>
> -              */<br>
> -             addr1 = sqlite3VdbeAddOp2(v, OP_Rewind, iDest, 0);<br>
> -             VdbeCoverage(v);<br>
> -             emptyDestTest = sqlite3VdbeAddOp0(v, OP_Goto);<br>
> -             sqlite3VdbeJumpHere(v, addr1);<br>
> -     }<br>
<br>
Why did you delete if-clause? The code which checks that<br>
table is empty is needed only in the cases in if-clauses,<br>
i.e. in some cases xfer optimisation can be applied even if<br>
table is not initially empty (as far as I understand).<br></blockquote><div><br></div><div>Yes, that's true this code is needed if any of the </div><div>cases in if-clause != 0. But the point is that in Tarantool<br>there's always a PRIMARY KEY that is obviously unique<br>which means that the second case is always != 0 and</div><div>the result of the boolean expression is always not 0,</div><div>hence there's no reason for this if-check.<br><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> +     /* The xfer optimization is unable to test uniqueness<br>
> +      * while we have a unique PRIMARY KEY in any existing table.<br>
> +      * This is the reason we can only run it if the destination table<br>
<br>
Nitpicking: comments should fit into 66 chars.<br>
<br>
> +      * is initially empty.<br>
> +      * This block generates code to make that determination.<br>
> +      */<br>
> +     addr1 = sqlite3VdbeAddOp2(v, OP_Rewind, iDest, 0);<br>
> +     VdbeCoverage(v);<br>
> +     emptyDestTest = sqlite3VdbeAddOp0(v, OP_Goto);<br>
> +     sqlite3VdbeJumpHere(v, addr1);<br>
> +<br>
> +     space = space_by_id(SQLITE_PAGENO_TO_SPACEID(pDest->tnum));<br>
> +     dest_idx = space_index(space, 0 /* PK */);<br>
> +     space = space_by_id(SQLITE_PAGENO_TO_SPACEID(pSrc->tnum));<br>
> +     src_idx = space_index(space, 0 /* PK */);<br>
> +     assert(src_idx);<br>
> +     assert(dest_idx);<br>
<br>
We use explicit != NULL checks.<br>
<br>
> +     pDestIdx = sqlite3PrimaryKeyIndex(pDest);<br>
> +     pSrcIdx = sqlite3PrimaryKeyIndex(pSrc);<br>
<br>
In fact, you don’t need these indexes at all.<br>
Instead of calling emit_open_cursor() you<br>
can simply add two opcodes: OP_LoadPtr and OP_OpenWrite<br>
(as it happens inside that function). Or, you can use pSrc->tnum.<br>
Table’s tnum exactly encodes PK space id (index id is 0).<br>
As for KeyInfo - you are able to remove it at all, since opcodes<br>
below don’t rely on key info (AFAIK, better check it yourself).<br>
The only issue remained - how to check PK compatibility.<br>
I would add to xferCompatibleIndex() another one check:<br>
If first index is PK, then second also must be PK.<br>
It is easy to implement, since Index struct contains appropriate flag.<br>
<br>
</blockquote></div></div>