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

  • From: "n.pettik" <korablev@xxxxxxxxxxxxx>
  • To: tarantool-patches@xxxxxxxxxxxxx
  • Date: Fri, 13 Jul 2018 05:15:21 +0300

Nitpicking ffter module prefix (i.e. sql:) don’t 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.


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/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[] = {
  { "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[] = {
  { "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[] = {
  { "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.
       */

-     op = ((pExpr->op + (TK_ISNULL & 1)) ^ 1) - (TK_ISNULL & 1);
+     if (pExpr->op >= TK_NE && pExpr->op <= TK_GE)
+             op = ((pExpr->op + (TK_NE & 1)) ^ 1) - (TK_NE & 1);
+     if (pExpr->op == TK_ISNULL || pExpr->op == TK_NOTNULL)
+             op = ((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.


      /*
       * Verify correct alignment of TK_ and OP_ constants.
-      * Tokens TK_ISNULL and TK_NE shoud have the same parity.
       */
      assert(pExpr->op != TK_NE || op == OP_Eq);
      assert(pExpr->op != TK_EQ || op == 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 ::= cmd.
///////////////////// Begin and end transactions. ////////////////////////////
//

-cmd ::= BEGIN trans_opt.  {sql_transaction_begin(pParse);}
+cmd ::= START TRANSACTION trans_opt.  {sql_transaction_begin(pParse);}
trans_opt ::= .
-trans_opt ::= TRANSACTION.
-trans_opt ::= TRANSACTION nm.
-cmd ::= COMMIT trans_opt.      {sql_transaction_commit(pParse);}
-cmd ::= END trans_opt.         {sql_transaction_commit(pParse);}
-cmd ::= ROLLBACK trans_opt.    {sql_transaction_rollback(pParse);}
+trans_opt ::= nm.

What is the point of named transactions, if rollback and commit
don’t use name? I mean, now we ofc can’t use its name, but
what does ANSI say?

+cmd ::= COMMIT.      {sql_transaction_commit(pParse);}
+cmd ::= ROLLBACK.    {sql_transaction_rollback(pParse);}

savepoint_opt ::= SAVEPOINT.
savepoint_opt ::= .
@@ -163,7 +161,7 @@ cmd ::= SAVEPOINT nm(X). {
cmd ::= RELEASE savepoint_opt nm(X). {
  sqlite3Savepoint(pParse, SAVEPOINT_RELEASE, &X);
}
-cmd ::= ROLLBACK trans_opt TO savepoint_opt nm(X). {
+cmd ::= ROLLBACK TO savepoint_opt nm(X). {
  sqlite3Savepoint(pParse, SAVEPOINT_ROLLBACK, &X);
}

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=off;
---             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", [=[["set i 1","$i<=10","incr i"]]=]) do
        local j = 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.


Other related posts: