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 16F4B2783D for ; Fri, 20 Jul 2018 07:57:12 -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 h0-3NaaJ_aop for ; Fri, 20 Jul 2018 07:57:11 -0400 (EDT) Received: from mail-lf1-f46.google.com (mail-lf1-f46.google.com [209.85.167.46]) (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 7B33C2781E for ; Fri, 20 Jul 2018 07:57:11 -0400 (EDT) Received: by mail-lf1-f46.google.com with SMTP id u14-v6so1618159lfu.0 for ; Fri, 20 Jul 2018 04:57:11 -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> <79D03E96-0BD0-418D-9DB2-45318C734628@tarantool.org> <8B8D5501-075D-4BEB-B282-35B0B81CD555@tarantool.org> <605B15EF-BD1C-4B03-8A9F-6E6225076812@tarantool.org> <12B62C73-9BEC-49FA-B3FD-590C445CF25B@tarantool.org> In-Reply-To: From: Nikita Tatunov Date: Fri, 20 Jul 2018 14:56:58 +0300 Message-ID: Subject: [tarantool-patches] Re: [PATCH] sql: xfer optimization issue Content-Type: multipart/alternative; boundary="000000000000dde29805716cfe75" 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: korablev@tarantool.org Cc: tarantool-patches@freelists.org --000000000000dde29805716cfe75 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Hello, Nikita! Thank you for review! =D0=BF=D1=82, 20 =D0=B8=D1=8E=D0=BB. 2018 =D0=B3. =D0=B2 6:20, n.pettik : > Several minor remarks. > > > @@ -1885,12 +1881,11 @@ xferOptimization(Parse * pParse, /* Parser > context */ > > > > /* > > * Xfer optimization is unable to correctly insert data > > - * in case there's a conflict action other than *_ABORT. > > - * This is the reason we want to only run it if the > > - * destination table is initially empty. > > + * in case there's a conflict action other than > > + * explicit *_ABORT. This is the reason we want to only > > + * run it if the destination table is initially empty. > > * That block generates code to make that determination. > > */ > > - > > if (!(onError =3D=3D ON_CONFLICT_ACTION_ABORT && > > is_err_action_default =3D=3D false)) { > > AFAIK we don=E2=80=99t use =3D=3D false comparison. Instead simply use ! > is_rr_action_default. > > Done. > > addr1 =3D sqlite3VdbeAddOp2(v, OP_Rewind, iDest, 0); > > diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c > > index 5f9bc13..bc169d9 100644 > > --- a/src/box/sql/vdbe.c > > +++ b/src/box/sql/vdbe.c > > @@ -4009,7 +4009,7 @@ case OP_RowData: { > > u32 n; > > > > #ifdef SQLITE_TEST > > - if (pOp->p5 =3D=3D 1) { > > + if (pOp->p5 =3D=3D OPFLAG_XFER_OPT) { > > Flags are usually tested with & operation: > > if (pOp->p5 & OPFLAG_XFER_OPT !=3D 0) > > Fixed it. > > pOp->p5 =3D 0; > > sql_xfer_count++; > > } > > diff --git a/test/sql-tap/gh-3307-xfer-optimization-issue.test.lua > b/test/sql-tap/gh-3307-xfer-optimization-issue.test.lua > > index 34f603f..29f0efe 100755 > > --- a/test/sql-tap/gh-3307-xfer-optimization-issue.test.lua > > +++ b/test/sql-tap/gh-3307-xfer-optimization-issue.test.lua > > @@ -1,9 +1,29 @@ > > > > +local function do_xfer_test(test_number, return_code) > > + test_name =3D string.format("xfer-optimization-1.%d", test_number= ) > > + test:do_test( > > + test_name, > > + function() > > + if (aftr - bfr =3D=3D 1) then > > + return {1} > > + end > > + if (aftr =3D=3D bfr) then > > + return {0} > > + end > > You can simplify it to: > > return aftr - bfr; > > :) > > True. Fixed it. > Also > > >+ /* The Vdbe we're building*/ > >+ Vdbe *v =3D sqlite3GetVdbe(pParse); > > Use =E2=80=99struct=E2=80=99 modifier for complex types. > And put dot at the end of comment. > > Fixed. diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c index 3c3bf37..4f52fa5 100644 --- a/src/box/sql/insert.c +++ b/src/box/sql/insert.c @@ -1869,7 +1869,7 @@ xferOptimization(Parse * pParse, /* Parser context */ * table (tab1) is initially empty. */ - /* The Vdbe we're building*/ + /* The Vdbe struct we're building. */ Vdbe *v =3D sqlite3GetVdbe(pParse); iSrc =3D pParse->nTab++; iDest =3D pParse->nTab++; @@ -1887,7 +1887,7 @@ xferOptimization(Parse * pParse, /* Parser context */ * That block generates code to make that determination. */ if (!(onError =3D=3D ON_CONFLICT_ACTION_ABORT && - is_err_action_default =3D=3D false)) { + !is_err_action_default)) { addr1 =3D sqlite3VdbeAddOp2(v, OP_Rewind, iDest, 0); VdbeCoverage(v); emptyDestTest =3D sqlite3VdbeAddOp0(v, OP_Goto); diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c index bc169d9..aa7f250 100644 --- a/src/box/sql/vdbe.c +++ b/src/box/sql/vdbe.c @@ -4009,7 +4009,7 @@ case OP_RowData: { u32 n; #ifdef SQLITE_TEST - if (pOp->p5 =3D=3D OPFLAG_XFER_OPT) { + if ((pOp->p5 & OPFLAG_XFER_OPT) !=3D 0) { pOp->p5 =3D 0; sql_xfer_count++; } diff --git a/test/sql-tap/gh-3307-xfer-optimization-issue.test.lua b/test/sql-tap/gh-3307-xfer-optimization-issue.test.lua index 29f0efe..05d30c6 100755 --- a/test/sql-tap/gh-3307-xfer-optimization-issue.test.lua +++ b/test/sql-tap/gh-3307-xfer-optimization-issue.test.lua @@ -9,12 +9,7 @@ local function do_xfer_test(test_number, return_code) test:do_test( test_name, function() - if (aftr - bfr =3D=3D 1) then - return {1} - end - if (aftr =3D=3D bfr) then - return {0} - end + return {aftr - bfr} end, { -- return_code --000000000000dde29805716cfe75 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable

Hello, Nikita! Thank you for review!

=D0=BF=D1=82, 20 =D0=B8=D1=8E=D0=BB. 2018= =D0=B3. =D0=B2 6:20, n.pettik <korablev@tarantool.org>:
Several minor remarks.

> @@ -1885,12 +1881,11 @@ xferOptimization(Parse * pParse,=C2=A0 =C2=A0 = =C2=A0 /* Parser context */
>=C2=A0
>=C2=A0 =C2=A0 =C2=A0 =C2=A0/*
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 * Xfer optimization is unable to correctly = insert data
> -=C2=A0 =C2=A0 =C2=A0 * in case there's a conflict action other th= an *_ABORT.
> -=C2=A0 =C2=A0 =C2=A0 * This is the reason we want to only run it if t= he
> -=C2=A0 =C2=A0 =C2=A0 * destination table is initially empty.
> +=C2=A0 =C2=A0 =C2=A0 * in case there's a conflict action other th= an
> +=C2=A0 =C2=A0 =C2=A0 * explicit *_ABORT. This is the reason we want t= o only
> +=C2=A0 =C2=A0 =C2=A0 * run it if the destination table is initially e= mpty.
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 * That block generates code to make that de= termination.
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 */
> -
>=C2=A0 =C2=A0 =C2=A0 =C2=A0if (!(onError =3D=3D ON_CONFLICT_ACTION_ABOR= T &&
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0is_err_action_default =3D=3D f= alse)) {

AFAIK we don=E2=80=99t use =3D=3D false comparison. Instead simply use ! is= _rr_action_default.


Done.
=C2=A0
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0addr1 =3D sqlite= 3VdbeAddOp2(v, OP_Rewind, iDest, 0);
> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index 5f9bc13..bc169d9 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -4009,7 +4009,7 @@ case OP_RowData: {
>=C2=A0 =C2=A0 =C2=A0 =C2=A0u32 n;
>=C2=A0
>=C2=A0 #ifdef SQLITE_TEST
> -=C2=A0 =C2=A0 =C2=A0if (pOp->p5 =3D=3D 1) {
> +=C2=A0 =C2=A0 =C2=A0if (pOp->p5 =3D=3D OPFLAG_XFER_OPT) {

Flags are usually tested with & operation:

if (pOp->p5 & OPFLAG_XFER_OPT !=3D 0)


Fixed it.
=C2=A0
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0pOp->p5 =3D 0= ;
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0sql_xfer_count++= ;
>=C2=A0 =C2=A0 =C2=A0 =C2=A0}
> diff --git a/test/sql-tap/gh-3307-xfer-optimization-issue.test.lua b/t= est/sql-tap/gh-3307-xfer-optimization-issue.test.lua
> index 34f603f..29f0efe 100755
> --- a/test/sql-tap/gh-3307-xfer-optimization-issue.test.lua
> +++ b/test/sql-tap/gh-3307-xfer-optimization-issue.test.lua
> @@ -1,9 +1,29 @@
>=C2=A0
> +local function do_xfer_test(test_number, return_code)
> +=C2=A0 =C2=A0 =C2=A0test_name =3D string.format("xfer-optimizati= on-1.%d", test_number)
> +=C2=A0 =C2=A0 =C2=A0test:do_test(
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0test_name,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0function()
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0if (aftr - bfr =3D=3D 1) then
> +=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=A0 =C2=A0return {1}
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0end
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0if (aftr =3D=3D bfr) then
> +=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=A0 =C2=A0return {0}
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0end

You can simplify it to:

return aftr - bfr;

:)


True. Fixed it.
=C2=A0
=
Also

>+=C2=A0 =C2=A0 =C2=A0 /* The Vdbe we're building*/
>+=C2=A0 =C2=A0 =C2=A0 Vdbe *v =3D sqlite3GetVdbe(pParse);

Use =E2=80=99struct=E2=80=99 modifier for complex types.
And put dot at the end of comment.


Fixed.=C2=A0

=
diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
ind= ex 3c3bf37..4f52fa5 100644
--- a/src/box/sql/insert.c
+= ++ b/src/box/sql/insert.c
@@ -1869,7 +1869,7 @@ xferOptimization(= Parse * pParse, /* Parser context */=
=C2=A0 * table (tab1) is= initially empty.
=C2=A0 = */
=C2=A0
- /* T= he Vdbe we're building*/
+ /* The Vdbe struct we're building. */
=C2=A0 Vdbe *v =3D sqlite3GetVdbe(pParse);
= =C2=A0 iSrc =3D pParse->nTab++;
=C2=A0 iDest =3D pParse->= ;nTab++;
@@ -1887,7 +1887,7 @@ xferOptimization(Parse * pParse, /* Parser context */
=C2=A0= * That block generates code to mak= e that determination.
=C2=A0 */
=C2=A0 if (!(onErro= r =3D=3D ON_CONFLICT_ACTION_ABORT &&
- =C2=A0 =C2=A0 is_err_action_default =3D=3D false)) {
+ =C2=A0 =C2=A0 !is_err_act= ion_default)) {
=C2=A0 ad= dr1 =3D sqlite3VdbeAddOp2(v, OP_Rewind, iDest, 0);
=C2=A0 VdbeCoverage(v);
=C2=A0 emptyDestTest =3D sqlite3VdbeAddOp0(v, OP_G= oto);
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
<= div>index bc169d9..aa7f250 100644
--- a/src/box/sql/vdbe.c
<= div>+++ b/src/box/sql/vdbe.c
@@ -4009,7 +4009,7 @@ case OP_RowDat= a: {
=C2=A0 u32 n;
=C2=A0
=C2=A0#ifdef SQLITE_TEST
- if (pOp->p5 =3D=3D OPFLAG_XFER_OPT) {
+ if ((pOp->p5 & OPFLAG_XFER_OPT= ) !=3D 0) {
=C2=A0 pOp-&g= t;p5 =3D 0;
=C2=A0 sql_xf= er_count++;
=C2=A0 }
=
diff --git a/test/sql-tap/gh-3307-xfer-optimization-issue.test.lua b/t= est/sql-tap/gh-3307-xfer-optimization-issue.test.lua
index 29f0ef= e..05d30c6 100755
--- a/test/sql-tap/gh-3307-xfer-optimization-is= sue.test.lua
+++ b/test/sql-tap/gh-3307-xfer-optimization-issue.t= est.lua
@@ -9,12 +9,7 @@ local function do_xfer_test(test_number,= return_code)
=C2=A0 test:= do_test(
=C2=A0 test_name= ,
=C2=A0 function()
=
- if (aftr - bfr =3D=3D 1) th= en
- return {1}
- end
- if (aftr =3D=3D bfr) then
- return {0}
- end
+ return {aftr - bfr}
=C2=A0 <= /span>end, {
=C2=A0 -- &= lt;test_name>
=C2=A0 = return_code

--000000000000dde29805716cfe75--