From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id F19A920408 for ; Sat, 5 May 2018 15:14:30 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id NDBmCCjrvcUJ for ; Sat, 5 May 2018 15:14:30 -0400 (EDT) Received: from smtp34.i.mail.ru (smtp34.i.mail.ru [94.100.177.94]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 36E741F922 for ; Sat, 5 May 2018 15:14:28 -0400 (EDT) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 10.3 \(3273\)) Subject: [tarantool-patches] Re: [PATCH 1/4] sql: remove OP_AutoCommit opcode From: "n.pettik" In-Reply-To: <5f3205b3-cd3b-e50a-e185-f7ea91067f99@tarantool.org> Date: Sat, 5 May 2018 22:14:23 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <85A40D5A-FE65-4E42-A62D-AD81EB2C9B86@tarantool.org> References: <7a43fd8723207263f3531d20b305e497bcf64e44.1525368399.git.korablev@tarantool.org> <5f3205b3-cd3b-e50a-e185-f7ea91067f99@tarantool.org> Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: tarantool-patches@freelists.org Cc: Vladislav Shpilevoy > 1. OP_AutoCommit still exists in mkopcodeh.sh. Ok, removed: --- a/extra/mkopcodeh.sh +++ b/extra/mkopcodeh.sh @@ -146,7 +146,6 @@ while [ "$i" -lt "$nOp" ]; do eval "name=3D\$ARRAY_order_$i" case "$name" in # The following are the opcodes that are processed by = resolveP2Values() - OP_AutoCommit | \ >> + assert(psql_txn->pSavepoint =3D=3D 0 || box_txn()); > 2. Please, use explicit =3D=3D/!=3D NULL (pSavepoint is pointer). --- 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 =3D=3D 0 || box_txn()); + assert(psql_txn->pSavepoint =3D=3D NULL || box_txn()); >> +/* 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()) { >=20 > 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. In fifth comment you wrote that you would better return 0/-1 from = sql_txn_begin() So anyway I have to manually set return code: @@ -2997,17 +2997,9 @@ case OP_FkCheckCommit: { * Otherwise, raise an error with appropriate error message. */ case OP_TransactionBegin: { - if (!box_txn()) { - p->auto_commit =3D false; - rc =3D sql_txn_begin(p); - if (rc !=3D SQLITE_OK) - goto abort_due_to_error; - } else { - sqlite3VdbeError(p, "cannot start a transaction within " - "a transaction"); - rc =3D SQLITE_ERROR; + if (sql_txn_begin(p) !=3D 0) { + rc =3D SQL_TARANTOOL_ERROR; goto abort_due_to_error; } + p->auto_commit =3D false; break; >> + >> +/* 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() !=3D 0) { >> + rc =3D SQL_TARANTOOL_ERROR; >> + goto abort_due_to_error; >> } >> - rc =3D 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 =3D SQLITE_ERROR; >> + goto abort_due_to_error; >=20 > 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. Well, I was sentenced to investigate SQL ANSI, so I found this: (2006 standard, Part 2: Foundation) 17.7 =20 =E2=80=A2 If an atomic execution context is active, then an = exception condition is raised: invalid transaction termination.=20 *Atomic execution context, as I understand is sort of auto-commit mode:* =E2=80=A2 4.33.5 SQL-statement atomicity and statement execution = contexts=20 The following are non-atomic SQL-statements: =E2=80=A2 =E2=80=94 =E2=80=A2 =E2=80=94 =E2=80=A2 =E2=80=94 =E2=80=A2 =E2=80=94 =E2=80=A2 =E2=80=94 =E2=80=A2 =E2=80=94 =E2=80=A2 =E2=80=94 All other SQL-statements are atomic SQL-statements. The statement execution context brought into existence by the execution = of a non-atomic SQL-statement is a non-atomic execution context.=20 >> @@ -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 =3D box_txn_begin() =3D=3D 0 ? SQLITE_OK : = SQL_TARANTOOL_ERROR; >> - } >> - if (box_txn() >> - && p->autoCommit =3D=3D 0){ >> + if (!box_txn()) { >> + if (box_txn_begin() !=3D 0) { >=20 > 5. Same as 3. If you need error code, then use box_error_code() when = begin() fails. To be honest, I didn=E2=80=99t get this idea. VDBE now supports only = SQL_TARANTOOL_ERROR, which in turn results in box_error_message() displaying. Do you mean to expose VDBE error codes with enum box_error_code ? >=20 >> 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; >=20 > 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. Ok: +++ b/src/box/sql/vdbeaux.c @@ -2479,19 +2479,21 @@ sql_txn_begin(Vdbe *p) { if (in_txn()) { diag_set(ClientError, ER_ACTIVE_TRANSACTION); - return SQL_TARANTOOL_ERROR; + return -1; } struct txn *ptxn =3D txn_begin(false); if (ptxn =3D=3D NULL) - return SQLITE_NOMEM; + return -1; ptxn->psql_txn =3D region_alloc_object(&fiber()->gc, struct = sql_txn); if (ptxn->psql_txn =3D=3D NULL) { box_txn_rollback(); - return SQLITE_NOMEM; + diag_set(OutOfMemory, sizeof(struct sql_txn), "region", + "struct sql_txn"); + return -1; } memset(ptxn->psql_txn, 0, sizeof(struct sql_txn)); p->psql_txn =3D ptxn->psql_txn; - return SQLITE_OK; + return 0; }