[tarantool-patches] Re: [PATCH 1/4] sql: remove OP_AutoCommit opcode
n.pettik
korablev at tarantool.org
Sat May 5 22:14:23 MSK 2018
> 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=\$ARRAY_order_$i"
case "$name" in
# The following are the opcodes that are processed by resolveP2Values()
- OP_AutoCommit | \
>> + assert(psql_txn->pSavepoint == 0 || box_txn());
> 2. Please, use explicit ==/!= 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 == 0 || box_txn());
+ assert(psql_txn->pSavepoint == 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()) {
>
> 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 = false;
- rc = sql_txn_begin(p);
- if (rc != SQLITE_OK)
- goto abort_due_to_error;
- } else {
- sqlite3VdbeError(p, "cannot start a transaction within "
- "a transaction");
- rc = SQLITE_ERROR;
+ if (sql_txn_begin(p) != 0) {
+ rc = SQL_TARANTOOL_ERROR;
goto abort_due_to_error;
}
+ p->auto_commit = 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() != 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.
Well, I was sentenced to investigate SQL ANSI, so I found this:
(2006 standard, Part 2: Foundation)
17.7 <commit statement>
• If an atomic execution context is active, then an exception condition is raised: invalid transaction termination.
*Atomic execution context, as I understand is sort of auto-commit mode:*
• 4.33.5 SQL-statement atomicity and statement execution contexts
The following are non-atomic SQL-statements:
• — <call statement>
• — <execute statement>
• — <execute immediate statement>
• — <commit statement>
• — <return statement>
• — <rollback statement>
• — <savepoint statement>
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.
>> @@ -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.
To be honest, I didn’t 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 ?
>
>> 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.
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 = txn_begin(false);
if (ptxn == NULL)
- return SQLITE_NOMEM;
+ return -1;
ptxn->psql_txn = region_alloc_object(&fiber()->gc, struct sql_txn);
if (ptxn->psql_txn == 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 = ptxn->psql_txn;
- return SQLITE_OK;
+ return 0;
}
More information about the Tarantool-patches
mailing list