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

Nikita Tatunov hollow653 at gmail.com
Mon Jul 16 15:22:41 MSK 2018


As Alexander asked I've added table content checking in tests, fixed the
causes of failing tests in Travis & fixed Nikita's remarks. Diff for the
newer version is in the end.

пт, 13 июл. 2018 г. в 5:15, n.pettik <korablev at tarantool.org>:

> Nitpicking ffter module prefix (i.e. sql:) don’t use upper case for first
> word
>
>
Fixed.


> > Patch is aimed on making our sql closer to ANSI sql.
>
> Nitpicking: write SQL in upper case - it is an abbreviation.
>
>
Fixed.


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

Fixed.


>
> >
> >       /*
> >        * 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?
>

Fixed it.
Ansi only asks for <transaction characteristics> after 'START TRANSACTION'.


>
> > +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.
>
>
Fixed.

diff --git a/extra/lempar.c b/extra/lempar.c
index 00fd79c..d043e39 100644
--- a/extra/lempar.c
+++ b/extra/lempar.c
@@ -336,8 +336,8 @@ void *ParseAlloc(void *(*mallocProc)(YYMALLOCARGTYPE)){
   if( pParser ){
 #ifdef YYTRACKMAXSTACKDEPTH
     pParser->yyhwm = 0;
-    pParser->is_fallback_failed = false;
 #endif
+    pParser->is_fallback_failed = false;
 #if YYSTACKDEPTH<=0
     pParser->yytos = NULL;
     pParser->yystack = NULL;
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 b1650cf..b26728d 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
  *        ...                ...
  *       TK_ISNULL          OP_NotNull
  *       TK_NOTNULL         OP_IsNull
  *
- * For other values of pExpr->op, op is undefined and unused.
- * The value of TK_ and OP_ constants are arranged such that we
- * can compute the mapping above using the following expression.
+ * For other values of pExpr->op, op is undefined
+ * and unused. The value of TK_ and OP_ constants
+ * are arranged such that we can compute the mapping
+ * above using the following expression. The idea
+ * is that both for OP_'s and TK_'s the first elements
+ * in the given mapping ranges of codes and tokens are
+ * 'Not equal' and 'Is null'. Moreover the 'excluding'
+ * ones (like 'Greater than' and 'Lower than or Equal')
+ * are paired and follow one each other, hence have n
+ * and n + 1 numbers.
  * 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);

  /*
  * 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 ac935fd..b4f11e5 100644
--- a/src/box/sql/parse.y
+++ b/src/box/sql/parse.y
@@ -147,13 +147,9 @@ cmdx ::= cmd.
 ///////////////////// Begin and end transactions.
////////////////////////////
 //

-cmd ::= BEGIN 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);}
+cmd ::= START TRANSACTION.  {sql_transaction_begin(pParse);}
+cmd ::= COMMIT.      {sql_transaction_commit(pParse);}
+cmd ::= ROLLBACK.    {sql_transaction_rollback(pParse);}

 savepoint_opt ::= SAVEPOINT.
 savepoint_opt ::= .
@@ -163,7 +159,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/start-transaction.test.lua
b/test/sql-tap/start-transaction.test.lua
new file mode 100755
index 0000000..eece4cd
--- /dev/null
+++ b/test/sql-tap/start-transaction.test.lua
@@ -0,0 +1,266 @@
+#!/usr/bin/env tarantool
+test = require("sqltester")
+test:plan(21)
+
+test:do_catchsql_test(
+ "start-transaction-1.0",
+ [[
+ CREATE TABLE IF NOT EXISTS t(id int PRIMARY KEY);
+ DELETE FROM t;
+ BEGIN;
+ INSERT INTO t VALUES (1);
+ INSERT INTO t VALUES (2);
+ COMMIT;
+ ]], {
+ -- <start-transaction-1.0>
+ 1, "near \"BEGIN\": syntax error"
+ -- <start-transaction-1.0>
+ })
+
+test:do_execsql_test(
+ "start-transaction-1.1",
+ [[
+ SELECT * FROM t;
+ ]], {
+ -- <start-transaction-1.1>
+
+ -- <start-transaction-1.1>
+ })
+
+test:do_catchsql_test(
+ "start-transaction-1.2",
+ [[
+ CREATE TABLE IF NOT EXISTS t(id int primary key);
+ delete from t;
+ BEGIN TRANSACTION;
+ INSERT INTO t VALUES (1);
+ INSERT INTO t VALUES (2);
+ COMMIT;
+ ]], {
+ -- <start-transaction-1.1>
+ 1, "near \"BEGIN\": syntax error"
+ -- <start-transaction-1.1>
+ })
+
+test:do_execsql_test(
+ "start-transaction-1.3",
+ [[
+ SELECT * FROM t;
+ ]], {
+ -- <start-transaction-1.3>
+
+ -- <start-transaction-1.3>
+ })
+
+test:do_catchsql_test(
+ "start-transaction-1.4",
+ [[
+ DELETE FROM t;
+ START TRANSACTION;
+ INSERT INTO t VALUES (1);
+ INSERT INTO t VALUES (2);
+ COMMIT;
+ ]], {
+ -- <start-transaction-1.4>
+ 0
+ -- <start-transaction-1.4>
+ })
+
+test:do_execsql_test(
+ "start-transaction-1.5",
+ [[
+ SELECT * FROM t;
+ ]], {
+ -- <start-transaction-1.5>
+ 1, 2
+ -- <start-transaction-1.5>
+ })
+
+test:do_catchsql_test(
+ "start-transaction-1.6",
+ [[
+ DELETE FROM t;
+ START TRANSACTION;
+ INSERT INTO t VALUES (1);
+ INSERT INTO t VALUES (2);
+ COMMIT TRANSACTION;
+ ]], {
+ -- <start-transaction-1.6>
+ 1, "keyword \"TRANSACTION\" is reserved"
+ -- <start-transaction-1.6>
+ })
+
+test:do_execsql_test(
+ "start-transaction-1.7",
+ [[
+ COMMIT;
+ SELECT * FROM t;
+ ]], {
+ -- <start-transaction-1.7>
+ 1, 2
+ -- <start-transaction-1.7>
+ })
+
+test:do_catchsql_test(
+ "start-transaction-1.8",
+ [[
+ DELETE FROM t;
+ START TRANSACTION;
+ INSERT INTO t VALUES (1);
+ INSERT INTO t VALUES (2);
+ END;
+ ]], {
+ -- <start-transaction-1.8>
+ 1, "keyword \"END\" is reserved"
+ -- <start-transaction-1.8>
+ })
+
+test:do_execsql_test(
+ "start-transaction-1.9",
+ [[
+ COMMIT;
+ SELECT * FROM t;
+ ]], {
+ -- <start-transaction-1.9>
+ 1, 2
+ -- <start-transaction-1.9>
+ })
+
+test:do_catchsql_test(
+ "start-transaction-1.10",
+ [[
+ DELETE FROM t;
+ START TRANSACTION;
+ INSERT INTO t VALUES (1);
+ INSERT INTO t VALUES (2);
+ END TRANSACTION;
+ ]], {
+ -- <start-transaction-1.10>
+ 1, "keyword \"END\" is reserved"
+ -- <start-transaction-1.10>
+ })
+
+test:do_execsql_test(
+ "start-transaction-1.11",
+ [[
+ COMMIT;
+ SELECT * FROM t;
+ ]], {
+ -- <start-transaction-1.11>
+ 1, 2
+ -- <start-transaction-1.11>
+ })
+
+test:do_catchsql_test(
+ "start-transaction-1.12",
+ [[
+ DELETE FROM t;
+ START TRANSACTION;
+ INSERT INTO t VALUES (1);
+ INSERT INTO t VALUES (2);
+ ROLLBACK;
+ ]], {
+ -- <start-transaction-1.12>
+ 0
+ -- <start-transaction-1.12>
+ })
+
+test:do_execsql_test(
+ "start-transaction-1.13",
+ [[
+ SELECT * FROM t;
+ ]], {
+ -- <start-transaction-1.13>
+
+ -- <start-transaction-1.13>
+ })
+
+test:do_catchsql_test(
+ "start-transaction-1.14",
+ [[
+ START TRANSACTION;
+ INSERT INTO t VALUES (1);
+ INSERT INTO t VALUES (2);
+ ROLLBACK TRANSACTION;
+ COMMIT;
+ ]], {
+ -- <start-transaction-1.14>
+ 1, "keyword \"TRANSACTION\" is reserved"
+ -- <start-transaction-1.14>
+ })
+
+test:do_execsql_test(
+ "start-transaction-1.15",
+ [[
+ COMMIT;
+ SELECT * FROM t;
+ ]], {
+ -- <start-transaction-1.15>
+ 1, 2
+ -- <start-transaction-1.15>
+ })
+
+test:do_catchsql_test(
+ "start-transaction-1.16",
+ [[
+ DELETE FROM t;
+ START TRANSACTION;
+ INSERT INTO t VALUES (1);
+ SAVEPOINT s1;
+ INSERT INTO t VALUES (2);
+ ROLLBACK TO s1;
+ COMMIT;
+ ]], {
+ -- <start-transaction-1.16>
+ 0
+ -- <start-transaction-1.16>
+ })
+
+test:do_execsql_test(
+ "start-transaction-1.17",
+ [[
+ SELECT * FROM t;
+ ]], {
+ -- <start-transaction-1.17>
+ 1
+ -- <start-transaction-1.17>
+ })
+
+test:do_catchsql_test(
+ "start-transaction-1.18",
+ [[
+ DELETE FROM t;
+ START TRANSACTION;
+ INSERT INTO t VALUES (1);
+ SAVEPOINT s1;
+ INSERT INTO t VALUES (2);
+ ROLLBACK TRANSACTION TO s1;
+ COMMIT;
+ ]], {
+ -- <start-transaction-1.18>
+ 1, "keyword \"TRANSACTION\" is reserved"
+ -- <start-transaction-1.18>
+ })
+
+test:do_execsql_test(
+ "start-transaction-1.19",
+ [[
+ COMMIT;
+ SELECT * FROM t;
+ ]], {
+ -- <start-transaction-1.19>
+ 1, 2
+ -- <start-transaction-1.19>
+ })
+
+test:do_execsql_test(
+ "start-transaction-1.20",
+ [[
+ drop table t;
+ ]], {
+ -- <start0transaction-1.20>
+
+ -- <start-transaction-1.20>
+ })
+
+test:finish_test()
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.tarantool.org/pipermail/tarantool-patches/attachments/20180716/118101ec/attachment.html>


More information about the Tarantool-patches mailing list