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 1B00824F9B for ; Mon, 16 Jul 2018 08:54:31 -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 g1ZBt0sKUT2a for ; Mon, 16 Jul 2018 08:54:30 -0400 (EDT) Received: from mail-lf0-f68.google.com (mail-lf0-f68.google.com [209.85.215.68]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 7DB7821571 for ; Mon, 16 Jul 2018 08:54:30 -0400 (EDT) Received: by mail-lf0-f68.google.com with SMTP id m13-v6so32165384lfb.12 for ; Mon, 16 Jul 2018 05:54:30 -0700 (PDT) MIME-Version: 1.0 References: <5BB99B27-5F86-4664-AAD5-57A22ECED854@tarantool.org> <93E4DAEA-EF90-479D-9F62-3D1CEB3CBE3F@tarantool.org> <20180628101839.fhnijezdpwviohop@tkn_work_nb> <20180709155006.fwrikbznqk23ger5@tkn_work_nb> In-Reply-To: <20180709155006.fwrikbznqk23ger5@tkn_work_nb> From: Nikita Tatunov Date: Mon, 16 Jul 2018 15:54:18 +0300 Message-ID: Subject: [tarantool-patches] Re: [PATCH] sql: xfer optimization issue Content-Type: multipart/alternative; boundary="0000000000007ce01705711d5432" 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: alexander.turenko@tarantool.org Cc: tarantool-patches@freelists.org, korablev@tarantool.org, kyukhin@tarantool.org --0000000000007ce01705711d5432 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable =D0=BF=D0=BD, 9 =D0=B8=D1=8E=D0=BB. 2018 =D0=B3. =D0=B2 18:50, Alexander Tu= renko < alexander.turenko@tarantool.org>: > Hi Nikita! > > Please, consider comments below. > > WBR, Alexander Turenko. > > > +test:do_execsql_test( > > + "xfer-oprimization-1.8", > > ... > > +test:do_execsql_test( > > + "xfer-oprimization-1.10", > > ... > > +test:do_execsql_test( > > + "xfer-oprimization-1.12", > > ... > > +test:do_execsql_test( > > + "xfer-oprimization-1.16", > > +test:do_execsql_test( > > + "xfer-oprimization-1.18", > > oprimization -> optimization > Is already fixed. > It seems that review questions from the last Nikita email in the thread > was not fixed or answered (at least some of them). > > > + sqlite3VdbeAddOp3(v, OP_OpenWrite, iSrc, > > + pSrc->tnum, space_ptr_reg); > > Why do you open source space for write? > > I changed it to 'vdbe_emit_open_cursor' which now is used everywhere for both reading and writing (internally it uses 'OP_OpenWrite') How your changes to xferCompatibleIndex and empty check enabling > condition is motivated? It is hard to understand it without more > detailed description. > > We open cursors on PK for both tables, additional check that we have opened 'em on PK isn't redundunt. What's about empty check enabling: I only want to add some cases where we don't demand empty destination table for xferOptimization to be used. Thus it will be used a little bit more frequently (it's faster than inserting non-raw data for about 13%). > --- > EOF. > > On Thu, Jun 28, 2018 at 01:18:39PM +0300, Alexander Turenko wrote: > > On Fri, May 04, 2018 at 12:54:30PM +0000, Hollow111 wrote: > > > > @@ -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? > > > Yes, it is, but there's a little difference between directly > specified > > > ABORT for an > > > insert stmt (INSERT OR ABORT) and just INSERT without any specifie= d > > > error action > > > (ABORT specified by the internals). When you directly specify it > ABORT > > > is a higher priority > > > action than in case there's a column with REPLACE error action. > Thus we > > > can even insert > > > not in the empty destination table. > > > > If an user asks for ABORT explicitly we should make abort, I think. > > > > As I understood the extra variable appears due to the fact than we can > > have per-column conflict clauses in CREATE TABLE and per-table clause > > with INSERT OR ABORT. The latter should have precedence, I think. > > > > I don't sure whether something (behaviour? code?) should be different > > from SQLite here in light of #2963 changes. Kirill, Nikita, can you > > comment, please? > > > > WBR, Alexander Turenko. > > > --0000000000007ce01705711d5432 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


=D0=BF= =D0=BD, 9 =D0=B8=D1=8E=D0=BB. 2018 =D0=B3. =D0=B2 18:50, Alexander Turenko = <alexander.turenko@ta= rantool.org>:
Hi Nikita!

Please, consider comments below.

WBR, Alexander Turenko.

> +test:do_execsql_test(
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0"xfer-oprimization-1.8",
> ...
> +test:do_execsql_test(
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0"xfer-oprimization-1.10",
> ...
> +test:do_execsql_test(
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0"xfer-oprimization-1.12",
> ...
> +test:do_execsql_test(
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0"xfer-oprimization-1.16",
> +test:do_execsql_test(
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0"xfer-oprimization-1.18",

oprimization -> optimization

Is alre= ady fixed.=C2=A0
=C2=A0
It seems that review questions from the last Nikita email in the thread
was not fixed or answered (at least some of them).

> +=C2=A0 =C2=A0 =C2=A0 =C2=A0sqlite3VdbeAddOp3(v, OP_OpenWrite, iSrc, > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0pSrc->tnum, space_ptr_reg);

Why do you open source space for write?


I changed it to 'vdbe_emit_open_cu= rsor' which now is used everywhere for both reading and writing (intern= ally it uses 'OP_OpenWrite')=C2=A0

How your changes to xferCompatibleIndex and empty check enabling
condition is motivated? It is hard to understand it without more
detailed description.


We open cursors on PK for both tables,= additional check that we have opened 'em on PK isn't redundunt.
What's about empty check enabling: I only want to add some case= s where we don't demand empty destination table for xferOptimization to= be used. Thus it will be used a little bit more frequently (it's faste= r than inserting non-raw data for about 13%).
=C2=A0
---
EOF.

On Thu, Jun 28, 2018 at 01:18:39PM +0300, Alexander Turenko wrote:
> On Fri, May 04, 2018 at 12:54:30PM +0000, Hollow111 wrote:
> >=C2=A0 =C2=A0 > @@ -1737,8 +1744,10 @@ xferOptimization(Parse *= pParse,=C2=A0 =C2=A0 =C2=A0 =C2=A0/*
> >=C2=A0 =C2=A0 Parser context */
> >=C2=A0 =C2=A0 >=C2=A0 =C2=A0 =C2=A0 =C2=A0if (onError =3D=3D ON= _CONFLICT_ACTION_DEFAULT) {
> >=C2=A0 =C2=A0 >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0if (pDest->iPKey >=3D 0)
> >=C2=A0 =C2=A0 >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0onError =3D pDest->keyConf;
> >=C2=A0 =C2=A0 > -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0if (onError =3D=3D ON_CONFLICT_ACTION_DEFAULT)
> >=C2=A0 =C2=A0 > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0if (onError =3D=3D ON_CONFLICT_ACTION_DEFAULT) {
> >=C2=A0 =C2=A0 >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0onError =3D ON_CONFLICT_ACTION_ABORT; > >=C2=A0 =C2=A0 > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0confl_action_default =3D 1;
> >=C2=A0 =C2=A0 Why do you need this variable at all? I mean, DEFAUL= T always
> >=C2=A0 =C2=A0 is an alias to ABORT, isn=E2=80=99t it?
> >=C2=A0 =C2=A0 Yes, it is, but there's a little difference betw= een directly specified
> >=C2=A0 =C2=A0 ABORT for an
> >=C2=A0 =C2=A0 insert stmt (INSERT OR ABORT) and just INSERT withou= t any specified
> >=C2=A0 =C2=A0 error action
> >=C2=A0 =C2=A0 (ABORT specified by the internals). When you directl= y specify it ABORT
> >=C2=A0 =C2=A0 is a higher priority
> >=C2=A0 =C2=A0 action than in case there's a column with REPLAC= E error action. Thus we
> >=C2=A0 =C2=A0 can even insert
> >=C2=A0 =C2=A0 not in the empty destination table.
>
> If an user asks for ABORT explicitly we should make abort, I think. >
> As I understood the extra variable appears due to the fact than we can=
> have per-column conflict clauses in CREATE TABLE and per-table clause<= br> > with INSERT OR ABORT. The latter should have precedence, I think.
>
> I don't sure whether something (behaviour? code?) should be differ= ent
> from SQLite here in light of #2963 changes. Kirill, Nikita, can you > comment, please?
>
> WBR, Alexander Turenko.
>
--0000000000007ce01705711d5432--