From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 384C121703 for ; Thu, 19 Apr 2018 21:02:52 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id XkzgELQpI49f for ; Thu, 19 Apr 2018 21:02:52 -0400 (EDT) Received: from smtp5.mail.ru (smtp5.mail.ru [94.100.179.24]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 371FB216F9 for ; Thu, 19 Apr 2018 21:02:50 -0400 (EDT) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 10.3 \(3273\)) Subject: [tarantool-patches] Re: [PATCH] sql: xfer optimization issue From: "n.pettik" In-Reply-To: Date: Fri, 20 Apr 2018 04:02:40 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <5BB99B27-5F86-4664-AAD5-57A22ECED854@tarantool.org> References: <1524065531-32467-1-git-send-email-hollow653@gmail.com> <08FAE06B-F6D3-49BD-9011-B5770629AA21@tarantool.org> Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: Hollow111 Cc: tarantool-patches@freelists.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=E2=80=99t 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 !=3D 0) /* (1) */ > - ||destHasUniqueIdx /* (2) */ > - || (onError !=3D ON_CONFLICT_ACTION_ABORT > - && onError !=3D 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 =3D sqlite3VdbeAddOp2(v, OP_Rewind, iDest, 0); > - VdbeCoverage(v); > - emptyDestTest =3D 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). > + /* 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 =3D sqlite3VdbeAddOp2(v, OP_Rewind, iDest, 0); > + VdbeCoverage(v); > + emptyDestTest =3D sqlite3VdbeAddOp0(v, OP_Goto); > + sqlite3VdbeJumpHere(v, addr1); > + > + space =3D space_by_id(SQLITE_PAGENO_TO_SPACEID(pDest->tnum)); > + dest_idx =3D space_index(space, 0 /* PK */); > + space =3D space_by_id(SQLITE_PAGENO_TO_SPACEID(pSrc->tnum)); > + src_idx =3D space_index(space, 0 /* PK */); > + assert(src_idx); > + assert(dest_idx); We use explicit !=3D NULL checks. > + pDestIdx =3D sqlite3PrimaryKeyIndex(pDest); > + pSrcIdx =3D sqlite3PrimaryKeyIndex(pSrc); In fact, you don=E2=80=99t 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=E2=80=99s 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=E2=80=99t 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.