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 : > 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 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; + ]], { + -- + 1, "near \"BEGIN\": syntax error" + -- + }) + +test:do_execsql_test( + "start-transaction-1.1", + [[ + SELECT * FROM t; + ]], { + -- + + -- + }) + +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; + ]], { + -- + 1, "near \"BEGIN\": syntax error" + -- + }) + +test:do_execsql_test( + "start-transaction-1.3", + [[ + SELECT * FROM t; + ]], { + -- + + -- + }) + +test:do_catchsql_test( + "start-transaction-1.4", + [[ + DELETE FROM t; + START TRANSACTION; + INSERT INTO t VALUES (1); + INSERT INTO t VALUES (2); + COMMIT; + ]], { + -- + 0 + -- + }) + +test:do_execsql_test( + "start-transaction-1.5", + [[ + SELECT * FROM t; + ]], { + -- + 1, 2 + -- + }) + +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; + ]], { + -- + 1, "keyword \"TRANSACTION\" is reserved" + -- + }) + +test:do_execsql_test( + "start-transaction-1.7", + [[ + COMMIT; + SELECT * FROM t; + ]], { + -- + 1, 2 + -- + }) + +test:do_catchsql_test( + "start-transaction-1.8", + [[ + DELETE FROM t; + START TRANSACTION; + INSERT INTO t VALUES (1); + INSERT INTO t VALUES (2); + END; + ]], { + -- + 1, "keyword \"END\" is reserved" + -- + }) + +test:do_execsql_test( + "start-transaction-1.9", + [[ + COMMIT; + SELECT * FROM t; + ]], { + -- + 1, 2 + -- + }) + +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; + ]], { + -- + 1, "keyword \"END\" is reserved" + -- + }) + +test:do_execsql_test( + "start-transaction-1.11", + [[ + COMMIT; + SELECT * FROM t; + ]], { + -- + 1, 2 + -- + }) + +test:do_catchsql_test( + "start-transaction-1.12", + [[ + DELETE FROM t; + START TRANSACTION; + INSERT INTO t VALUES (1); + INSERT INTO t VALUES (2); + ROLLBACK; + ]], { + -- + 0 + -- + }) + +test:do_execsql_test( + "start-transaction-1.13", + [[ + SELECT * FROM t; + ]], { + -- + + -- + }) + +test:do_catchsql_test( + "start-transaction-1.14", + [[ + START TRANSACTION; + INSERT INTO t VALUES (1); + INSERT INTO t VALUES (2); + ROLLBACK TRANSACTION; + COMMIT; + ]], { + -- + 1, "keyword \"TRANSACTION\" is reserved" + -- + }) + +test:do_execsql_test( + "start-transaction-1.15", + [[ + COMMIT; + SELECT * FROM t; + ]], { + -- + 1, 2 + -- + }) + +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; + ]], { + -- + 0 + -- + }) + +test:do_execsql_test( + "start-transaction-1.17", + [[ + SELECT * FROM t; + ]], { + -- + 1 + -- + }) + +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; + ]], { + -- + 1, "keyword \"TRANSACTION\" is reserved" + -- + }) + +test:do_execsql_test( + "start-transaction-1.19", + [[ + COMMIT; + SELECT * FROM t; + ]], { + -- + 1, 2 + -- + }) + +test:do_execsql_test( + "start-transaction-1.20", + [[ + drop table t; + ]], { + -- + + -- + }) + +test:finish_test()