Tarantool development patches archive
 help / color / mirror / Atom feed
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.

  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