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

  • From: Nikita Tatunov <hollow653@xxxxxxxxx>
  • To: v.shpilevoy@xxxxxxxxxxxxx
  • Date: Thu, 19 Jul 2018 21:08:27 +0300

чт, 19 июл. 2018 г. в 18:44, Vladislav Shpilevoy <v.shpilevoy@xxxxxxxxxxxxx

:

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.


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",
  [[

Other related posts:

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