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

  • From: Vladislav Shpilevoy <v.shpilevoy@xxxxxxxxxxxxx>
  • To: tarantool-patches@xxxxxxxxxxxxx, "n.pettik" <korablev@xxxxxxxxxxxxx>
  • Date: Thu, 19 Jul 2018 18:44:22 +0300

Hi! Thanks for the patch! See 2 comments below.

commit 1e031e580ca3242baa91aabf5b436ae8f3088669
Author: N.Tatunov <hollow653@xxxxxxxxx>
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.

         *        ...                ...
         *       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.

Other related posts:

  1. Home
  2. tarantool-patches
  3. Archive
  4. 07-2018