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

[n.pettik]

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.