[tarantool-patches] Re: [PATCH] sql: Remove 'BEGIN TRANSACTION'
Nikita Tatunov
hollow653 at gmail.com
Thu Jul 19 21:08:27 MSK 2018
чт, 19 июл. 2018 г. в 18:44, Vladislav Shpilevoy <v.shpilevoy at tarantool.org
>:
> Hi! Thanks for the patch! See 2 comments below.
>
> > commit 1e031e580ca3242baa91aabf5b436ae8f3088669
> > Author: N.Tatunov <hollow653 at gmail.com>
> > 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",
[[
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.tarantool.org/pipermail/tarantool-patches/attachments/20180719/c17b46af/attachment.html>
More information about the Tarantool-patches
mailing list