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 AA8602798E for ; Thu, 19 Jul 2018 14:08:40 -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 2F8GNKSs-hDJ for ; Thu, 19 Jul 2018 14:08:40 -0400 (EDT) Received: from mail-lj1-f195.google.com (mail-lj1-f195.google.com [209.85.208.195]) (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 2313023837 for ; Thu, 19 Jul 2018 14:08:39 -0400 (EDT) Received: by mail-lj1-f195.google.com with SMTP id u7-v6so8804945lji.3 for ; Thu, 19 Jul 2018 11:08:39 -0700 (PDT) MIME-Version: 1.0 References: <1530787337-18302-1-git-send-email-hollow653@gmail.com> <40A96246-8FFD-4E6F-9FEC-5309A19D9DB8@tarantool.org> In-Reply-To: From: Nikita Tatunov Date: Thu, 19 Jul 2018 21:08:27 +0300 Message-ID: Subject: [tarantool-patches] Re: [PATCH] sql: Remove 'BEGIN TRANSACTION' Content-Type: multipart/alternative; boundary="000000000000888adf05715e114b" 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: v.shpilevoy@tarantool.org Cc: tarantool-patches@freelists.org, korablev@tarantool.org --000000000000888adf05715e114b Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable =D1=87=D1=82, 19 =D0=B8=D1=8E=D0=BB. 2018 =D0=B3. =D0=B2 18:44, Vladislav S= hpilevoy : > Hi! Thanks for the patch! See 2 comments below. > > > commit 1e031e580ca3242baa91aabf5b436ae8f3088669 > > Author: N.Tatunov > > Date: Wed Jul 4 20:21:06 2018 +0300 > > > > sql: remove 'BEGIN TRANSACTION' > > > > Previously "BEGIN" / "BEGIN TRANSACTION", "COMMIT TRANSACTION" / > > "END" / "END TRANSACTION", "ROLLBACK TRANSACTION" could be used > > to handle transactions. By changing these commands syntax in > > parser we're aiming on getting closer to ANSI SQL. Also found > > initialization in ifdef that caused some problems with error > > messages in occasions when the wrong syntax was used. > > > > With the patch applied only following commands can be used: > > - "START TRANSACTION" to begin transaction. > > - "COMMIT" to end transaction. > > - "ROLLBACK" to rollback transaction without savepoints. > > - "ROLLBACK TO .." to rollback transaction to savepoint. > > > > Closes #2164 > > > > diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c > > index b1650cfa3..b26728d70 100644 > > --- a/src/box/sql/expr.c > > +++ b/src/box/sql/expr.c > > @@ -4826,23 +4826,32 @@ sqlite3ExprIfFalse(Parse * pParse, Expr * pExpr= , > int dest, int jumpIfNull) > > * TK_EQ OP_Ne > > * TK_GT OP_Le > > * TK_LE OP_Gt > > - * TK_GE OP_Lt > > * TK_LT OP_Ge > > + * TK_GE OP_Lt > > 1. The table now does not match the actual values of > OP_... and TK_... values. TK_NE(13) now !=3D OP_Eq(14), > OP_Ne < OP_Eq etc. > > Speaking in general, I understand the original optimization - > it allowed to convert TK_ into OP_ not via if's, but via single > expression ((pExpr->op + (TK_ISNULL & 1)) ^ 1) - (TK_ISNULL & 1). > > The only purpose was avoid if-ing. But now we have ifs. I think, > that you should either try to do not break this dependency, > or remove it completely in a separate patch. For example, > the expression above can be inlined into switch (pExpr->op) below, > that anyway checks all TK_... value. On switch (pExpr->op) > case TK_NE you can manually set op to OP_Ne. Same for other > LT, LE, GT etc. > > The table only shows the relation between pExpr->op and op. It shouldn't match mapping between OP_* and TK_* values indicating the same operation. > > * ... ... > > * TK_ISNULL OP_NotNull > > * TK_NOTNULL OP_IsNull > > * > > diff --git a/test/sql-tap/start-transaction.test.lua > b/test/sql-tap/start-transaction.test.lua > > new file mode 100755 > > index 000000000..98ca531aa > > --- /dev/null > > +++ b/test/sql-tap/start-transaction.test.lua > > @@ -0,0 +1,266 @@ > > +#!/usr/bin/env tarantool > > +test =3D require("sqltester") > > +test:plan(21) > > 2. Please, put here the issue number and a description > in a comment. > Done. Diff for changes in this patch: diff --git a/test/sql-tap/start-transaction.test.lua b/test/sql-tap/start-transaction.test.lua index 98ca531..82356ae 100755 --- a/test/sql-tap/start-transaction.test.lua +++ b/test/sql-tap/start-transaction.test.lua @@ -2,6 +2,14 @@ test =3D require("sqltester") test:plan(21) +----------------------------------------------------------------- +-- Previously "BEGIN" / "BEGIN TRANSACTION", "COMMIT TRANSACTION" +-- / "END" / "END TRANSACTION", "ROLLBACK TRANSACTION" could be +-- used to handle transactions. By changing these commands syntax +-- in parser we're aiming on getting closer to ANSI SQL. +----------------------------------------------------------------- +-- Tarantool issue: #2164. + test:do_catchsql_test( "start-transaction-1.0", [[ --000000000000888adf05715e114b Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


=D1=87= =D1=82, 19 =D0=B8=D1=8E=D0=BB. 2018 =D0=B3. =D0=B2 18:44, Vladislav Shpilev= oy <v.shpilevoy@tarantool.o= rg>:
Hi! = Thanks for the patch! See 2 comments below.

> commit 1e031e580ca3242baa91aabf5b436ae8f3088669
> Author: N.Tatunov <hollow653@gmail.com>
> Date:=C2=A0 =C2=A0Wed Jul 4 20:21:06 2018 +0300
>
>=C2=A0 =C2=A0 =C2=A0sql: remove 'BEGIN TRANSACTION'
>=C2=A0 =C2=A0 =C2=A0
>=C2=A0 =C2=A0 =C2=A0Previously "BEGIN" / "BEGIN TRANSACT= ION", "COMMIT TRANSACTION" /
>=C2=A0 =C2=A0 =C2=A0"END" / "END TRANSACTION", &quo= t;ROLLBACK TRANSACTION" could be used
>=C2=A0 =C2=A0 =C2=A0to handle transactions. By changing these commands = syntax in
>=C2=A0 =C2=A0 =C2=A0parser we're aiming on getting closer to ANSI S= QL. Also found
>=C2=A0 =C2=A0 =C2=A0initialization in ifdef that caused some problems w= ith error
>=C2=A0 =C2=A0 =C2=A0messages in occasions when the wrong syntax was use= d.
>=C2=A0 =C2=A0 =C2=A0
>=C2=A0 =C2=A0 =C2=A0With the patch applied only following commands can = be used:
>=C2=A0 =C2=A0 =C2=A0 - "START TRANSACTION" to begin transacti= on.
>=C2=A0 =C2=A0 =C2=A0 - "COMMIT" to end transaction.
>=C2=A0 =C2=A0 =C2=A0 - "ROLLBACK" to rollback transaction wit= hout savepoints.
>=C2=A0 =C2=A0 =C2=A0 - "ROLLBACK TO .." to rollback transacti= on to savepoint.
>=C2=A0 =C2=A0 =C2=A0
>=C2=A0 =C2=A0 =C2=A0Closes #2164
>
> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
> index b1650cfa3..b26728d70 100644
> --- a/src/box/sql/expr.c
> +++ b/src/box/sql/expr.c
> @@ -4826,23 +4826,32 @@ sqlite3ExprIfFalse(Parse * pParse, Expr * pExp= r, int dest, int jumpIfNull)
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 *=C2=A0 =C2=A0 =C2=A0 =C2=A0TK_EQ=C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 OP_Ne
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 *=C2=A0 =C2=A0 =C2=A0 =C2=A0TK_GT=C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 OP_Le
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 *=C2=A0 =C2=A0 =C2=A0 =C2=A0TK_LE=C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 OP_Gt
> -=C2=A0 =C2=A0 =C2=A0 *=C2=A0 =C2=A0 =C2=A0 =C2=A0TK_GE=C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 OP_Lt
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 *=C2=A0 =C2=A0 =C2=A0 =C2=A0TK_LT=C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 OP_Ge
> +=C2=A0 =C2=A0 =C2=A0 *=C2=A0 =C2=A0 =C2=A0 =C2=A0TK_GE=C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 OP_Lt

1. The table now does not match the actual values of
OP_... and TK_... values. TK_NE(13) now !=3D OP_Eq(14),
OP_Ne < OP_Eq etc.

Speaking in general, I understand the original optimization -
it allowed to convert TK_ into OP_ not via if's, but via single
expression ((pExpr->op + (TK_ISNULL & 1)) ^ 1) - (TK_ISNULL & 1)= .

The only purpose was avoid if-ing. But now we have ifs. I think,
that you should either try to do not break this dependency,
or remove it completely in a separate patch. For example,
the expression above can be inlined into switch (pExpr->op) below,
that anyway checks all TK_... value. On switch (pExpr->op)
case TK_NE you can manually set op to OP_Ne. Same for other
LT, LE, GT etc.


The table only shows the relation betw= een pExpr->op and op.
It shouldn't match mapping between O= P_* and TK_* values
indicating the same operation.
=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=A0 =C2=A0 =C2=A0 ...
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 *=C2=A0 =C2=A0 =C2=A0 =C2=A0TK_ISNULL=C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 OP_NotNull
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 *=C2=A0 =C2=A0 =C2=A0 =C2=A0TK_NOTNULL=C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0OP_IsNull
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 *
> diff --git a/test/sql-tap/start-transaction.test.lua b/test/sql-tap/st= art-transaction.test.lua
> new file mode 100755
> index 000000000..98ca531aa
> --- /dev/null
> +++ b/test/sql-tap/start-transaction.test.lua
> @@ -0,0 +1,266 @@
> +#!/usr/bin/env tarantool
> +test =3D require("sqltester")
> +test:plan(21)

2. Please, put here the issue number and a description
in a comment.

Done.

Diff for changes in this patch:

diff --git a/test/s= ql-tap/start-transaction.test.lua b/test/sql-tap/start-transaction.test.lua=
index 98ca531..82356ae 100755
--- a/test/sql-tap/start= -transaction.test.lua
+++ b/test/sql-tap/start-transaction.test.l= ua
@@ -2,6 +2,14 @@
=C2=A0test =3D require("sqltes= ter")
=C2=A0test:plan(21)
=C2=A0
+------= -----------------------------------------------------------
+-- P= reviously "BEGIN" / "BEGIN TRANSACTION", "COMMIT T= RANSACTION"
+-- / "END" / "END TRANSACTION&qu= ot;, "ROLLBACK TRANSACTION" could be
+-- used to handle= transactions. By changing these commands syntax
+-- in parser we= 're aiming on getting closer to ANSI SQL.
+------------------= -----------------------------------------------
+-- Tarantool iss= ue: #2164.
+
=C2=A0test:do_catchsql_test(
=C2= =A0 "start-transaction-1.0"= ;,
=C2=A0 [[=C2=A0
--000000000000888adf05715e114b--