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 7DB98277FF for ; Thu, 19 Jul 2018 14:13:32 -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 F7XyZfOfl1z5 for ; Thu, 19 Jul 2018 14:13:32 -0400 (EDT) Received: from smtp53.i.mail.ru (smtp53.i.mail.ru [94.100.177.113]) (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 C2FB421146 for ; Thu, 19 Jul 2018 14:13:31 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH] sql: Remove 'BEGIN TRANSACTION' References: <1530787337-18302-1-git-send-email-hollow653@gmail.com> <40A96246-8FFD-4E6F-9FEC-5309A19D9DB8@tarantool.org> From: Vladislav Shpilevoy Message-ID: Date: Thu, 19 Jul 2018 21:13:29 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Language: en-US Content-Transfer-Encoding: 8bit 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: Nikita Tatunov Cc: tarantool-patches@freelists.org, korablev@tarantool.org LGTM. On 19/07/2018 21:08, Nikita Tatunov wrote: > > > чт, 19 июл. 2018 г. в 18:44, Vladislav Shpilevoy >: > > 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 != 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 = 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 = 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", > [[