[tarantool-patches] Re: [PATCH v1 07/12] sql: set errors in VDBE using diag_set()

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

> On 5 May 2019, at 15:17, imeevma@tarantool.org wrote:
> 
> After this patch, all errors in VDBE will be set using diag_set().
> 
> Part of #4074
> ---
> src/box/execute.c     |  23 +---
> src/box/sql/vdbe.c    | 331 +++++++++++++++++++++-----------------------------
> src/box/sql/vdbeInt.h |  10 --
> src/box/sql/vdbeapi.c |  34 +-----
> src/box/sql/vdbeaux.c |  20 +--
> 5 files changed, 148 insertions(+), 270 deletions(-)

Please, remove whole sqlErrStr(), tarantoolErrorMessage() -
they are unused now. The same concerns sql_ret_code() -
I see no reason keeping it. Please, remove all legacy routines
connected with error codes.

> diff --git a/src/box/execute.c b/src/box/execute.c
> index a3d4a92..e81cc32 100644
> --- a/src/box/execute.c
> +++ b/src/box/execute.c
> 
> @@ -1335,7 +1331,8 @@ case OP_ResultRow: {
> 	 * not return the number of rows modified. And do not RELEASE the statement
> 	 * transaction. It needs to be rolled back.
> 	 */
> -	if (SQL_OK!=(rc = sqlVdbeCheckFk(p, 0))) {
> +	rc = sqlVdbeCheckFk(p, 0);
> +	if (rc != SQL_OK) {

-> if (sqlVdbeCheckFk() != 0)

> 		assert(user_session->sql_flags&SQL_CountRows);
> 		goto abort_due_to_error;
> 	}
> 
> @@ -1435,10 +1431,10 @@ case OP_Concat: {           /* same as TK_CONCAT, in1, in2, out3 */
> 	if (str_type_p1 != str_type_p2) {
> 		diag_set(ClientError, ER_INCONSISTENT_TYPES,
> 			 mem_type_to_str(pIn2), mem_type_to_str(pIn1));
> -		rc = SQL_TARANTOOL_ERROR;
> 		goto abort_due_to_error;
> 	}
> -	if (ExpandBlob(pIn1) || ExpandBlob(pIn2)) goto no_mem;
> +	if (ExpandBlob(pIn1) != SQL_OK || ExpandBlob(pIn2) != SQL_OK)
> +		goto abort_due_to_error;

!= 0
Still need to call sqlOomFault(db); by jumping to no_mem label.

> 	nByte = pIn1->n + pIn2->n;
> 	if (nByte>db->aLimit[SQL_LIMIT_LENGTH]) {
> 		goto too_big;
> 
> 
> @@ -1723,11 +1715,15 @@ case OP_Function: {
> 	/* If the function returned an error, throw an exception */
> 	if (pCtx->fErrorOrAux) {
> 		if (pCtx->isError) {
> -			sqlVdbeError(p, "%s", sql_value_text(pCtx->pOut));
> -			rc = pCtx->isError;
> +			if (pCtx->isError != SQL_TARANTOOL_ERROR) {

How it can be different from SQL_TARANTOOL_ERROR?

> +				diag_set(ClientError, ER_SQL_EXECUTE,
> +					 sql_value_text(pCtx->pOut));
> +			}
> +			rc = SQL_TARANTOOL_ERROR;
> 		}
> 		sqlVdbeDeleteAuxData(db, &p->pAuxData, pCtx->iOp, pOp->p1);
> -		if (rc) goto abort_due_to_error;
> +		if (rc != SQL_OK)
> +			goto abort_due_to_error;

This diff seems to be redundant

> @@ -1910,8 +1903,7 @@ case OP_Realify: {                  /* in1 */
>  */
> case OP_Cast: {                  /* in1 */
> 	pIn1 = &aMem[pOp->p1];
> -	rc = ExpandBlob(pIn1);
> -	if (rc != 0)
> +	if (ExpandBlob(pIn1) != SQL_OK)

!= 0
Please, don’t use SQL_OK value anywhere.

> 
> @@ -2802,10 +2784,8 @@ case OP_MakeRecord: {
> 	uint32_t tuple_size;
> 	char *tuple =
> 		sql_vdbe_mem_encode_tuple(pData0, nField, &tuple_size, region);
> -	if (tuple == NULL) {
> -		rc = SQL_TARANTOOL_ERROR;
> +	if (tuple == NULL)
> 		goto abort_due_to_error;
> -	}
> 	if ((int64_t)tuple_size > db->aLimit[SQL_LIMIT_LENGTH])
> 		goto too_big;
> 
> 
> @@ -2918,8 +2898,10 @@ case OP_Savepoint: {
> 			pSavepoint = pSavepoint->pNext
> 			);
> 		if (!pSavepoint) {
> -			sqlVdbeError(p, "no such savepoint: %s", zName);
> -			rc = SQL_ERROR;
> +			const char *err =
> +				tt_sprintf("no such savepoint: %s", zName);
> +			diag_set(ClientError, ER_SQL_EXECUTE, err);

We already have ER_NO_SUCH_SAVEPOINT.

> @@ -3685,10 +3645,11 @@ case OP_Found: {        /* jump, in3 */
> 		}
> 	}
> 	rc = sqlCursorMovetoUnpacked(pC->uc.pCursor, pIdxKey, &res);
> -	if (pFree) sqlDbFree(db, pFree);
> -	if (rc!=SQL_OK) {
> +	if (pFree)

!= NULL

> 
> @@ -5164,11 +5126,16 @@ case OP_AggStep: {
> 	(pCtx->pFunc->xSFunc)(pCtx,pCtx->argc,pCtx->argv); /* IMP: R-24505-23230 */
> 	if (pCtx->fErrorOrAux) {
> 		if (pCtx->isError) {
> -			sqlVdbeError(p, "%s", sql_value_text(&t));
> -			rc = pCtx->isError;
> +			if (pCtx->isError != SQL_TARANTOOL_ERROR) {
> +				diag_set(ClientError, ER_SQL_EXECUTE,
> +					 sql_value_text(&t));
> +			}
> +			rc = SQL_TARANTOOL_ERROR;
> 		}
> 		sqlVdbeMemRelease(&t);
> -		if (rc) goto abort_due_to_error;
> +		assert(rc == SQL_OK || rc == SQL_TARANTOOL_ERROR);
> +		if (rc != SQL_OK)
> +			goto abort_due_to_error;

Looks like redundant diff

> 	} else {
> 		assert(t.flags==MEM_Null);
> 	}
> @@ -5199,8 +5166,11 @@ case OP_AggFinal: {
> 	pMem = &aMem[pOp->p1];
> 	assert((pMem->flags & ~(MEM_Null|MEM_Agg))==0);
> 	rc = sqlVdbeMemFinalize(pMem, pOp->p4.pFunc);
> -	if (rc) {
> -		sqlVdbeError(p, "%s", sql_value_text(pMem));
> +	if (rc != SQL_OK) {
> +		if (rc != SQL_TARANTOOL_ERROR) {
> +			diag_set(ClientError, ER_SQL_EXECUTE,
> +				 sql_value_text(pMem));

Could you please clarify what does this error mean?
It would just print value of memory to string…
The same relates to error in OP_AggStep.

> @@ -5311,10 +5281,8 @@ case OP_IncMaxid: {
> 	assert(pOp->p1 > 0);
> 	pOut = &aMem[pOp->p1];
> 
> -	rc = tarantoolsqlIncrementMaxid((uint64_t*) &pOut->u.i);
> -	if (rc!=SQL_OK) {
> +	if (tarantoolsqlIncrementMaxid((uint64_t*) &pOut->u.i) != SQL_OK)
> 		goto abort_due_to_error;
> -	}
> 	pOut->flags = MEM_Int;
> 	break;
> }
> 
> too_big:
> -	sqlVdbeError(p, "string or blob too big");
> -	rc = SQL_TOOBIG;
> +	diag_set(ClientError, ER_SQL_EXECUTE, "string or blob too big”);

-> is too big

> 	goto abort_due_to_error;
> 
> 	/* Jump to here if a malloc() fails.
> 	 */
> no_mem:
> 	sqlOomFault(db);
> -	sqlVdbeError(p, "out of memory");
> -	rc = SQL_NOMEM;
> 	goto abort_due_to_error;
> }
> diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h
> index a3100e5..b655b5a 100644
> --- a/src/box/sql/vdbeInt.h
> +++ b/src/box/sql/vdbeInt.h
> @@ -248,10 +248,6 @@ struct Mem {
> #define MEM_Agg       0x4000	/* Mem.z points to an agg function context */
> #define MEM_Zero      0x8000	/* Mem.i contains count of 0s appended to blob */
> #define MEM_Subtype   0x10000	/* Mem.eSubtype is valid */
> -#ifdef SQL_OMIT_INCRBLOB
> -#undef MEM_Zero
> -#define MEM_Zero 0x0000
> -#endif
> 
> 
> @@ -550,13 +545,8 @@ void sqlVdbeMemPrettyPrint(Mem * pMem, char *zBuf);
> #endif
> int sqlVdbeMemHandleBom(Mem * pMem);
> 
> -#ifndef SQL_OMIT_INCRBLOB

Still see usage of this macro in code: vdbemem.c : 213

> int sqlVdbeMemExpandBlob(Mem *);
> #define ExpandBlob(P) (((P)->flags&MEM_Zero)?sqlVdbeMemExpandBlob(P):0)
> -#else
> -#define sqlVdbeMemExpandBlob(x) SQL_OK
> -#define ExpandBlob(P) SQL_OK
> -#endif
> 
> /**
>  * Perform comparison of two keys: one is packed and one is not.
> 
> 
> diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
> index 27fa5b2..48c2a81 100644
> --- a/src/box/sql/vdbeaux.c
> +++ b/src/box/sql/vdbeaux.c
> void
> @@ -2124,10 +2111,11 @@ sqlVdbeCheckFk(Vdbe * p, int deferred)
> 	if ((deferred && txn != NULL && txn->psql_txn != NULL &&
> 	     txn->psql_txn->fk_deferred_count > 0) ||
> 	    (!deferred && p->nFkConstraint > 0)) {
> -		p->rc = SQL_CONSTRAINT_FOREIGNKEY;
> +		p->rc = SQL_TARANTOOL_ERROR;
> 		p->errorAction = ON_CONFLICT_ACTION_ABORT;
> -		sqlVdbeError(p, "FOREIGN KEY constraint failed");
> -		return SQL_ERROR;
> +		diag_set(ClientError, ER_SQL_EXECUTE, "FOREIGN KEY constraint "\
> +			 "failed”);

Please, reserve separate error code for this violation.

> +		return SQL_TARANTOOL_ERROR;
> 	}
> 	return SQL_OK;

Return 0/-1