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 8896F22506 for ; Thu, 3 May 2018 18:57:45 -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 P63xdK_wsTZT for ; Thu, 3 May 2018 18:57:45 -0400 (EDT) Received: from smtp39.i.mail.ru (smtp39.i.mail.ru [94.100.177.99]) (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 D4985224B1 for ; Thu, 3 May 2018 18:57:44 -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, 4 May 2018 01:57:11 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: References: <1524065531-32467-1-git-send-email-hollow653@gmail.com> <08FAE06B-F6D3-49BD-9011-B5770629AA21@tarantool.org> <5BB99B27-5F86-4664-AAD5-57A22ECED854@tarantool.org> <93E4DAEA-EF90-479D-9F62-3D1CEB3CBE3F@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: "N. Tatunov" > diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c > index ae8dafb..734ff34 100644 > --- a/src/box/sql/insert.c > +++ b/src/box/sql/insert.c > @@ -1645,6 +1645,12 @@ 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 SQLITE_IDXTYPE_PRIMARYKEY && > + pSrc->idxType !=3D SQLITE_IDXTYPE_PRIMARYKEY) || > + (pDest->idxType !=3D SQLITE_IDXTYPE_PRIMARYKEY && > + pSrc->idxType =3D=3D SQLITE_IDXTYPE_PRIMARYKEY)) > + return 0; Why don=E2=80=99t just compare their types? Wouldn=E2=80=99t it be = easier? Yep, such condition would be little bit stronger, but I think it is = worth it. > @@ -1718,9 +1724,10 @@ xferOptimization(Parse * pParse, /* = Parser context */ > int emptyDestTest =3D 0; /* Address of test for empty = pDest */ > int emptySrcTest =3D 0; /* Address of test for empty pSrc */ > 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(); > + int dest_has_replace_action =3D 0; > + int confl_action_default =3D 0; Call it error action. Also, it is better to use bool type (even if bool might be really int underhood). And bool values usually comes with =E2=80=98is_=E2=80=99 or =E2=80=98has_' prefix. > @@ -1737,8 +1744,10 @@ xferOptimization(Parse * pParse, /* = Parser context */ > if (onError =3D=3D ON_CONFLICT_ACTION_DEFAULT) { > if (pDest->iPKey >=3D 0) > onError =3D pDest->keyConf; > - if (onError =3D=3D ON_CONFLICT_ACTION_DEFAULT) > + if (onError =3D=3D ON_CONFLICT_ACTION_DEFAULT) { > onError =3D ON_CONFLICT_ACTION_ABORT; > + confl_action_default =3D 1; Why do you need this variable at all? I mean, DEFAULT always is an alias to ABORT, isn=E2=80=99t it? > + } > } > assert(pSelect->pSrc); /* allocated even if there is no FROM = clause */ > if (pSelect->pSrc->nSrc !=3D 1) { > @@ -1828,15 +1837,16 @@ xferOptimization(Parse * pParse, /* = Parser context */ > return 0; /* Default values must = be the same for all columns */ > } > } > + if (pDestCol->notNull =3D=3D ON_CONFLICT_ACTION_REPLACE) > + dest_has_replace_action =3D 1; Don=E2=80=99t confuse NOT NULL error action for field and conflict = action for index: Index->onError is not the same as Column->notNull. > for (pDestIdx =3D pDest->pIndex; pDestIdx; pDestIdx =3D = pDestIdx->pNext) { > - if (index_is_unique(pDestIdx)) { > - destHasUniqueIdx =3D 1; > - } > for (pSrcIdx =3D pSrc->pIndex; pSrcIdx; pSrcIdx =3D = pSrcIdx->pNext) { > if (xferCompatibleIndex(pDestIdx, pSrcIdx)) > break; > } > + if (pDestIdx->onError !=3D ON_CONFLICT_ACTION_REPLACE) > + dest_has_replace_action =3D 1; Wait, you assign =E2=80=99true=E2=80=99 to variable =E2=80=98has_replace=E2= =80=99 when it is not really replace=E2=80=A6 It looks very strange. > @@ -1875,58 +1885,63 @@ xferOptimization(Parse * pParse, /* = Parser context */ > + > + /* Xfer optimization is unable to correctly insert > + * data in case there's a conflict action > + * other than ON_CONFLICT_ACTION_ROLLBACK or there's > + * ON_CONFLICT_ACTION_DEFAULT which was transformed into > + * ON_CONFLICT_ACTION_ABORT for insertion while we have a > + * ON_CONFLICT_ACTION_REPLACE for any of constraints. I would use only names of actions, these enum names are too long. But it is up to you. > + if (onError !=3D ON_CONFLICT_ACTION_ROLLBACK || > + (confl_action_default =3D=3D 1 && > + dest_has_replace_action =3D=3D 1)) { You can fit two lines above into one... > + int space_ptr_reg =3D ++pParse->nMem; > + struct space *src_space =3D > + space_by_id(SQLITE_PAGENO_TO_SPACEID(pSrc->tnum)); > + struct space *dest_space =3D > + space_by_id(SQLITE_PAGENO_TO_SPACEID(pDest->tnum)); > + struct index *src_idx =3D space_index(src_space, 0); > + struct index *dest_idx; Why don=E2=80=99t make index lookup for destination space right here? > + pDestIdx =3D sqlite3PrimaryKeyIndex(pDest); > + pSrcIdx =3D sqlite3PrimaryKeyIndex(pSrc); These variables are unused. > diff --git a/test/sql-tap/gh-3307-xfer-optimization-issue.test.lua = b/test/sql-tap/gh-3307-xfer-optimization-issue.test.lua > new file mode 100755 > index 0000000..860b4c3 > --- /dev/null > +++ b/test/sql-tap/gh-3307-xfer-optimization-issue.test.lua > @@ -0,0 +1,233 @@ > +#!/usr/bin/env tarantool > +test =3D require("sqltester") > +test:plan(18) The only one thing I don=E2=80=99t really like in these tests is the = absence of verification of occurred optimisation. Could you come up with the way how to check it?