[tarantool-patches] Re: [PATCH 1/4] sql: remove OP_AutoCommit opcode

["n.pettik" <korablev@tarantool.org>]

> 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;
 }