[tarantool-patches] Re: [PATCH v1 07/12] sql: set errors in VDBE using diag_set()
n.pettik
korablev at tarantool.org
Wed May 15 16:26:54 MSK 2019
> On 5 May 2019, at 15:17, imeevma at 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
More information about the Tarantool-patches
mailing list