From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org> To: Nikita Pettik <korablev@tarantool.org>, tarantool-patches@freelists.org Subject: [tarantool-patches] Re: [PATCH 1/4] sql: remove OP_AutoCommit opcode Date: Fri, 4 May 2018 17:12:25 +0300 [thread overview] Message-ID: <5f3205b3-cd3b-e50a-e185-f7ea91067f99@tarantool.org> (raw) In-Reply-To: <7a43fd8723207263f3531d20b305e497bcf64e44.1525368399.git.korablev@tarantool.org> Hello. Thanks for contributing! See 6 comments below. 1. OP_AutoCommit still exists in mkopcodeh.sh. On 03/05/2018 21:49, Nikita Pettik wrote: > In SQLite OP_AutoCommit opcode used to set transaction operation: > BEGIN, ROLLBACK and COMMIT, switching auto-commit flag in VDBE. > As for Tarantool, it is confusing, since there are some differences > between auto-commit modes: 'INSERT ... VALUES (1), (2), (3)' is one > indivisible operation for SQLite, and three operations in real > auto-commit mode for Tarantool. To simulate SQLite auto-commit mode, > these three insertions are wrapped into one SEPARATE transaction, > which is, in fact, not real autocommit mode. > So, lets add separate explicit opcodes to BEGIN, ROLLBACK and COMMIT > transactions as user's operations. Auto-commit mode is set once at VDBE > creation and can be changed only by implicit opcode OP_TTransaction, > which is added to each DML statement, or by 'BEGIN' SQL statement. > --- > src/box/sql/build.c | 51 +++++------------- > src/box/sql/main.c | 3 +- > src/box/sql/parse.y | 11 ++-- > src/box/sql/sqliteInt.h | 31 +++++++++-- > src/box/sql/vdbe.c | 136 +++++++++++++++++++++++++----------------------- > src/box/sql/vdbeInt.h | 3 +- > src/box/sql/vdbeapi.c | 5 +- > src/box/sql/vdbeaux.c | 34 ++++++------ > 8 files changed, 138 insertions(+), 136 deletions(-) > > diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c > index 013460f79..cf2b769c7 100644 > --- a/src/box/sql/vdbe.c > +++ b/src/box/sql/vdbe.c > @@ -2875,7 +2875,7 @@ case OP_Savepoint: { > /* Assert that the p1 parameter is valid. Also that if there is no open > * transaction, then there cannot be any savepoints. > */ > - assert(psql_txn->pSavepoint == 0 || p->autoCommit == 0); > + assert(psql_txn->pSavepoint == 0 || box_txn()); 2. Please, use explicit ==/!= NULL (pSavepoint is pointer). > +/* Opcode: TransactionBegin * * * * * > + * > + * Start Tarantool's transaction. > + * Only do that if there is no other active transactions. > + * Otherwise, raise an error with appropriate error message. > + */ > +case OP_TransactionBegin: { > + if (!box_txn()) { 3. sql_txn_begin() does this check for you. Here you can always call sql_txn_begin() and check result code. After this you can remove manual error message creating, and just goto abort_due_to_error. Error code is always SQL_TARANTOOL_ERROR. > + > +/* Opcode: TransactionCommit * * * * * > + * > + * Commit Tarantool's transaction. > + * If there is no active transaction, raise an error. > + */ > +case OP_TransactionCommit: { > + if (box_txn()) { > + if (box_txn_commit() != 0) { > + rc = SQL_TARANTOOL_ERROR; > + goto abort_due_to_error; > } > - rc = SQLITE_DONE; > - goto vdbe_return; > } else { > - sqlite3VdbeError(p, > - (!desiredAutoCommit)?"cannot start a transaction within a transaction":( > - (iRollback)?"cannot rollback - no transaction is active": > - "cannot commit - no transaction is active")); > + sqlite3VdbeError(p, "cannot commit - no transaction is active"); > + rc = SQLITE_ERROR; > + goto abort_due_to_error; 4. I am not sure, that we must throw on commit/rollback with no transaction. In Lua it is ok. Possibly you should ask in the server team chat. > @@ -3052,18 +3052,26 @@ case OP_AutoCommit: { > > /* Opcode: TTransaction * * * * * > * > - * Start Tarantool's transaction. > - * Only do that if auto commit mode is on. This should be no-op > - * if this opcode was emitted inside a transaction. > + * Start Tarantool's transaction, if there is no active > + * transactions. Otherwise, create anonymous savepoint, > + * which is used to correctly process ABORT statement inside > + * outer transaction. > + * > + * In contrast to OP_TransactionBegin, this is service opcode, > + * generated automatically alongside with DML routine. > */ > case OP_TTransaction: { > - if (p->autoCommit) { > - rc = box_txn_begin() == 0 ? SQLITE_OK : SQL_TARANTOOL_ERROR; > - } > - if (box_txn() > - && p->autoCommit == 0){ > + if (!box_txn()) { > + if (box_txn_begin() != 0) { 5. Same as 3. If you need error code, then use box_error_code() when begin() fails. > diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c > index b3998ea27..0bd0d455b 100644 > --- a/src/box/sql/vdbeaux.c > +++ b/src/box/sql/vdbeaux.c > @@ -2477,31 +2477,27 @@ sqlite3VdbeCheckFk(Vdbe * p, int deferred) > int > sql_txn_begin(Vdbe *p) > { > - struct txn *ptxn; > - > if (in_txn()) { > diag_set(ClientError, ER_ACTIVE_TRANSACTION); > - return -1; > + return SQL_TARANTOOL_ERROR; 6. It is step backwards to sqlite result codes style. Lets return 0/-1, and set errors via diag_set. If sql_txn_begin() returns -1, it is always SQL_TARANTOOL_ERROR.
next prev parent reply other threads:[~2018-05-04 14:12 UTC|newest] Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-05-03 18:49 [tarantool-patches] [PATCH 0/4] Rework SQL transaction processing Nikita Pettik 2018-05-03 18:49 ` [tarantool-patches] [PATCH 1/4] sql: remove OP_AutoCommit opcode Nikita Pettik 2018-05-04 14:12 ` Vladislav Shpilevoy [this message] 2018-05-05 19:14 ` [tarantool-patches] " n.pettik 2018-05-03 18:49 ` [tarantool-patches] [PATCH 2/4] sql: allow transitive Lua <-> SQL transactions Nikita Pettik 2018-05-04 14:12 ` [tarantool-patches] " Vladislav Shpilevoy 2018-05-05 19:14 ` n.pettik 2018-05-03 18:49 ` [tarantool-patches] [PATCH 3/4] sql: allow SAVEPOINT statement outside transaction Nikita Pettik 2018-05-04 14:12 ` [tarantool-patches] " Vladislav Shpilevoy 2018-05-05 19:15 ` n.pettik 2018-05-03 18:49 ` [tarantool-patches] [PATCH 4/4] sql: fix SAVEPOINT RELEASE statement Nikita Pettik 2018-05-04 14:12 ` [tarantool-patches] " Vladislav Shpilevoy 2018-05-05 19:16 ` n.pettik 2018-05-07 13:31 ` [tarantool-patches] Re: [PATCH 0/4] Rework SQL transaction processing Vladislav Shpilevoy 2018-05-11 7:17 ` Kirill Yukhin 2018-05-11 10:08 ` Kirill Yukhin
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=5f3205b3-cd3b-e50a-e185-f7ea91067f99@tarantool.org \ --to=v.shpilevoy@tarantool.org \ --cc=korablev@tarantool.org \ --cc=tarantool-patches@freelists.org \ --subject='[tarantool-patches] Re: [PATCH 1/4] sql: remove OP_AutoCommit opcode' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox