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 1AAF522972 for ; Mon, 23 Apr 2018 19:40: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 1xXaKYpohHjH for ; Mon, 23 Apr 2018 19:40:52 -0400 (EDT) Received: from smtp44.i.mail.ru (smtp44.i.mail.ru [94.100.177.104]) (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 745D42296F for ; Mon, 23 Apr 2018 19:40:51 -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: Tue, 24 Apr 2018 02:40:42 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <93E4DAEA-EF90-479D-9F62-3D1CEB3CBE3F@tarantool.org> References: <1524065531-32467-1-git-send-email-hollow653@gmail.com> <08FAE06B-F6D3-49BD-9011-B5770629AA21@tarantool.org> <5BB99B27-5F86-4664-AAD5-57A22ECED854@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: tarantool-patches@freelists.org Cc: Hollow111 Hello. Consider comments below. > diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c > index ae8dafb..87e1bad 100644 > --- a/src/box/sql/insert.c > +++ b/src/box/sql/insert.c > @@ -1645,6 +1645,9 @@ xferCompatibleIndex(Index * pDest, Index * pSrc) > assert(pDest->pTable !=3D pSrc->pTable); > uint32_t nDestCol =3D index_column_count(pDest); > uint32_t nSrcCol =3D index_column_count(pSrc); > + /* One of them is PK while the other isn't */ > + if ((pDest->idxType =3D=3D 2 && pSrc->idxType !=3D 2) > + || (pDest->idxType !=3D 2 && pSrc->idxType =3D=3D 2)) It is better to use SQLITE_IDXTYPE_PRIMARYKEY macros, then hardcoded constants. But the main thing: you forgot to add =E2=80=98return 0;=E2=80=99 = statement here. Codestyle nitpicking: put binary operator to previous line. Also, put dots at the end of sentences. > if (nDestCol !=3D nSrcCol) { > return 0; /* Different number of columns */ > } > @@ -1711,16 +1714,21 @@ 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 */ > + /* Source and destination indices */ > + struct index *src_idx, *dest_idx; You didn=E2=80=99t fixed remark from previous review: >We don=E2=80=99t use forward var declaration: it is obsolete rule from = ancient C standards. Although it is not mandatory, but better fix it. > struct SrcList_item *pItem; /* An element of pSelect->pSrc = */ > int i; /* Loop counter */ > int iSrc, iDest; /* Cursors from source and destination = */ > int addr1; /* Loop addresses */ > int emptyDestTest =3D 0; /* Address of test for empty = pDest */ > int emptySrcTest =3D 0; /* Address of test for empty pSrc */ > + /* Number of memory cell for cursors */ > + int space_ptr_reg; The same is here. > Vdbe *v; /* The VDBE we are building */ > - int destHasUniqueIdx =3D 0; /* True if pDest has a UNIQUE = index */ > int regData, regTupleid; /* Registers holding data and = tupleid */ > struct session *user_session =3D current_session(); > + /* Space pointer for pDest and pSrc */ > + struct space *space; And here. > + /* The xfer optimization is unable to test > + * uniqueness while we have a unique > + * PRIMARY KEY in any existing table. Rephrase this sentence. Or I have missed smth: uniqness is checked while tuple is inserted in space by Tarantool facilities, isn=E2=80=99t it? For this reason, you can try to = return those if-checks for additional code-generation. The only concern I have is about =E2=80=98on conflict replace' error action: if source table isn=E2=80=99t initially empty, features = 'on replace' error action and you insert table which violates uniqueness -=20 this replace action might fail. I advise you to investigate such behaviour. > + * This is the reason we can only run it > + * if the destination table 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); > + > + pDestIdx =3D sqlite3PrimaryKeyIndex(pDest); > + pSrcIdx =3D sqlite3PrimaryKeyIndex(pSrc); > + space =3D space_by_id(SQLITE_PAGENO_TO_SPACEID(pSrc->tnum)); > + if((src_idx =3D space_index(space, 0 /* PK */)) =3D=3D NULL) Codestyle nitpicking: put space after =E2=80=98if=E2=80=99. Also, do not =E2=80=98inline=E2=80=99 comments into code. And why space can=E2=80=99t feature primary index? AFAIK, only views don=E2=80=99t have PK, but src table is tested to be real table: if (space_is_view(pSrc)) { return 0; /* tab2 may not be a view */ }=20 > + return 0; > + space_ptr_reg =3D ++pParse->nMem; > + sqlite3VdbeAddOp4Ptr(v, OP_LoadPtr, 0, space_ptr_reg, 0, > + (void *) space); > + sqlite3VdbeAddOp3(v, OP_OpenWrite, iSrc, > + pSrc->tnum, space_ptr_reg); > + VdbeComment((v, "%s", src_idx->def->name)); > + space =3D space_by_id(SQLITE_PAGENO_TO_SPACEID(pDest->tnum)); Don=E2=80=99t be afraid of introducing new variables. It would be better, if you use src_space and dest_space separately.