[tarantool-patches] Re: [PATCH] sql: Remove 'BEGIN TRANSACTION'

[Vladislav Shpilevoy]

LGTM.

On 19/07/2018 21:08, Nikita Tatunov wrote:
> 
> 
> чт, 19 июл. 2018 г. в 18:44, Vladislav Shpilevoy <v.shpilevoy at tarantool.org <mailto:v.shpilevoy at tarantool.org>>:
> 
>     Hi! Thanks for the patch! See 2 comments below.
> 
>      > commit 1e031e580ca3242baa91aabf5b436ae8f3088669
>      > Author: N.Tatunov <hollow653 at gmail.com <mailto:hollow653 at gmail.com>>
>      > 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",
> [[