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 274F427280 for ; Thu, 12 Jul 2018 22:15:30 -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 I-FobDCM46u3 for ; Thu, 12 Jul 2018 22:15:30 -0400 (EDT) Received: from smtp60.i.mail.ru (smtp60.i.mail.ru [217.69.128.40]) (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 D8DEF2727E for ; Thu, 12 Jul 2018 22:15:29 -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: Remove 'BEGIN TRANSACTION' From: "n.pettik" In-Reply-To: <1530787337-18302-1-git-send-email-hollow653@gmail.com> Date: Fri, 13 Jul 2018 05:15:21 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: References: <1530787337-18302-1-git-send-email-hollow653@gmail.com> 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 Nitpicking ffter module prefix (i.e. sql:) don=E2=80=99t use upper case = for first word > Patch is aimed on making our sql closer to ANSI sql. Nitpicking: write SQL in upper case - it is an abbreviation. >=20 > 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. >=20 > Closes #2164 > --- >=20 > diff --git a/extra/mkkeywordhash.c b/extra/mkkeywordhash.c > index 990c419..1ec1538 100644 > --- a/extra/mkkeywordhash.c > +++ b/extra/mkkeywordhash.c > @@ -120,7 +120,7 @@ static Keyword aKeywordTable[] =3D { > { "ASC", "TK_ASC", ALWAYS, true = }, > { "AUTOINCREMENT", "TK_AUTOINCR", AUTOINCR, = false }, > { "BEFORE", "TK_BEFORE", TRIGGER, = false }, > - { "BEGIN", "TK_BEGIN", ALWAYS, = true }, > + { "BEGIN", "TK_BEGIN", TRIGGER, = true }, > { "BETWEEN", "TK_BETWEEN", ALWAYS, true = }, > { "BY", "TK_BY", ALWAYS, true = }, > { "CASCADE", "TK_CASCADE", FKEY, = false }, > @@ -210,6 +210,7 @@ static Keyword aKeywordTable[] =3D { > { "SAVEPOINT", "TK_SAVEPOINT", ALWAYS, true = }, > { "SELECT", "TK_SELECT", ALWAYS, true = }, > { "SET", "TK_SET", ALWAYS, true = }, > + { "START", "TK_START", ALWAYS, = true }, > { "TABLE", "TK_TABLE", ALWAYS, true = }, > { "THEN", "TK_THEN", ALWAYS, true = }, > { "TO", "TK_TO", ALWAYS, true = }, > @@ -274,7 +275,6 @@ static Keyword aKeywordTable[] =3D { > { "SIGNAL", "TK_STANDARD", RESERVED, true = }, > { "SMALLINT", "TK_ID", RESERVED, true = }, > { "SPECIFIC", "TK_STANDARD", RESERVED, true = }, > - { "START", "TK_STANDARD", RESERVED, = true }, > { "SYSTEM", "TK_STANDARD", RESERVED, true = }, > { "SQL", "TK_STANDARD", RESERVED, true = }, > { "USER", "TK_STANDARD", RESERVED, true = }, > diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c > index 3183e3d..42ab600 100644 > --- a/src/box/sql/expr.c > +++ b/src/box/sql/expr.c > @@ -4835,11 +4835,13 @@ sqlite3ExprIfFalse(Parse * pParse, Expr * = pExpr, int dest, int jumpIfNull) > * Assert()s verify that the computation is correct. > */ >=20 > - op =3D ((pExpr->op + (TK_ISNULL & 1)) ^ 1) - (TK_ISNULL & 1); > + if (pExpr->op >=3D TK_NE && pExpr->op <=3D TK_GE) > + op =3D ((pExpr->op + (TK_NE & 1)) ^ 1) - (TK_NE & 1); > + if (pExpr->op =3D=3D TK_ISNULL || pExpr->op =3D=3D TK_NOTNULL) > + op =3D ((pExpr->op + (TK_ISNULL & 1)) ^ 1) - (TK_ISNULL = & 1); Please, leave comment how this code work. It isn't even close to be obvious, really. >=20 > /* > * Verify correct alignment of TK_ and OP_ constants. > - * Tokens TK_ISNULL and TK_NE shoud have the same parity. > */ > assert(pExpr->op !=3D TK_NE || op =3D=3D OP_Eq); > assert(pExpr->op !=3D TK_EQ || op =3D=3D OP_Ne); > diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y > index b2940b7..e956dfc 100644 > --- a/src/box/sql/parse.y > +++ b/src/box/sql/parse.y > @@ -147,13 +147,11 @@ cmdx ::=3D cmd. > ///////////////////// Begin and end transactions. = //////////////////////////// > // >=20 > -cmd ::=3D BEGIN trans_opt. {sql_transaction_begin(pParse);} > +cmd ::=3D START TRANSACTION trans_opt. = {sql_transaction_begin(pParse);} > trans_opt ::=3D . > -trans_opt ::=3D TRANSACTION. > -trans_opt ::=3D TRANSACTION nm. > -cmd ::=3D COMMIT trans_opt. {sql_transaction_commit(pParse);} > -cmd ::=3D END trans_opt. {sql_transaction_commit(pParse);} > -cmd ::=3D ROLLBACK trans_opt. {sql_transaction_rollback(pParse);} > +trans_opt ::=3D nm. What is the point of named transactions, if rollback and commit don=E2=80=99t use name? I mean, now we ofc can=E2=80=99t use its name, = but what does ANSI say? > +cmd ::=3D COMMIT. {sql_transaction_commit(pParse);} > +cmd ::=3D ROLLBACK. {sql_transaction_rollback(pParse);} >=20 > savepoint_opt ::=3D SAVEPOINT. > savepoint_opt ::=3D . > @@ -163,7 +161,7 @@ cmd ::=3D SAVEPOINT nm(X). { > cmd ::=3D RELEASE savepoint_opt nm(X). { > sqlite3Savepoint(pParse, SAVEPOINT_RELEASE, &X); > } > -cmd ::=3D ROLLBACK trans_opt TO savepoint_opt nm(X). { > +cmd ::=3D ROLLBACK TO savepoint_opt nm(X). { > sqlite3Savepoint(pParse, SAVEPOINT_ROLLBACK, &X); > } >=20 > diff --git a/test/sql-tap/analyze3.test.lua = b/test/sql-tap/analyze3.test.lua > index 26f8793..5079962 100755 > --- a/test/sql-tap/analyze3.test.lua > +++ b/test/sql-tap/analyze3.test.lua > @@ -340,7 +340,7 @@ test:do_execsql_test( > "analyze3-1.3.1", > [[ > CREATE TABLE t3(id INTEGER PRIMARY KEY, y TEXT, x INTEGER); > - BEGIN; > + START TRANSACTION; > INSERT INTO t3 SELECT id, y, x FROM t1; > COMMIT; > CREATE INDEX i3 ON t3(x); > @@ -465,7 +465,7 @@ test:do_test( > -- function() > -- test:execsql([[ > -- PRAGMA case_sensitive_like=3Doff; > --- BEGIN; > +-- START TRANSACTION; I guess, you can remove it at all. It makes no sense to add dead code. > --- a/test/sql-tap/in1.test.lua > +++ b/test/sql-tap/in1.test.lua > @@ -26,7 +26,7 @@ test:do_test( > function() > test:execsql [[ > CREATE TABLE t1(a PRIMARY KEY, b); > - BEGIN; > + START TRANSACTION; > ]] > -- for _ in X(0, "X!for", [=3D[["set i 1","$i<=3D10","incr = i"]]=3D]) do > local j =3D 1 > @@ -1073,7 +1073,7 @@ test:do_execsql_test( > -- MUST_WORK_TEST > -- do_test in-13.14 { > -- execsql { > --- BEGIN TRANSACTION; > +-- START TRANSACTION; The same is here and in other places.