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 32DFB2E908 for ; Wed, 15 May 2019 09:26:57 -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 TB8KsFBqwHXb for ; Wed, 15 May 2019 09:26:57 -0400 (EDT) Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (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 E4B552BBF9 for ; Wed, 15 May 2019 09:26:56 -0400 (EDT) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 12.4 \(3445.104.8\)) Subject: [tarantool-patches] Re: [PATCH v1 07/12] sql: set errors in VDBE using diag_set() From: "n.pettik" In-Reply-To: <8b2d92d24172d1cd5934afa638b92232a93fba19.1557056617.git.imeevma@gmail.com> Date: Wed, 15 May 2019 16:26:54 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <8C6EC917-D1A6-4171-B203-97CA432CF8F3@tarantool.org> References: <8b2d92d24172d1cd5934afa638b92232a93fba19.1557056617.git.imeevma@gmail.com> 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: Imeev Mergen > On 5 May 2019, at 15:17, imeevma@tarantool.org wrote: >=20 > After this patch, all errors in VDBE will be set using diag_set(). >=20 > 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 >=20 > @@ -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!=3D(rc =3D sqlVdbeCheckFk(p, 0))) { > + rc =3D sqlVdbeCheckFk(p, 0); > + if (rc !=3D SQL_OK) { -> if (sqlVdbeCheckFk() !=3D 0) > assert(user_session->sql_flags&SQL_CountRows); > goto abort_due_to_error; > } >=20 > @@ -1435,10 +1431,10 @@ case OP_Concat: { /* same as = TK_CONCAT, in1, in2, out3 */ > if (str_type_p1 !=3D str_type_p2) { > diag_set(ClientError, ER_INCONSISTENT_TYPES, > mem_type_to_str(pIn2), mem_type_to_str(pIn1)); > - rc =3D SQL_TARANTOOL_ERROR; > goto abort_due_to_error; > } > - if (ExpandBlob(pIn1) || ExpandBlob(pIn2)) goto no_mem; > + if (ExpandBlob(pIn1) !=3D SQL_OK || ExpandBlob(pIn2) !=3D = SQL_OK) > + goto abort_due_to_error; !=3D 0 Still need to call sqlOomFault(db); by jumping to no_mem label. > nByte =3D pIn1->n + pIn2->n; > if (nByte>db->aLimit[SQL_LIMIT_LENGTH]) { > goto too_big; >=20 >=20 > @@ -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 =3D pCtx->isError; > + if (pCtx->isError !=3D SQL_TARANTOOL_ERROR) { How it can be different from SQL_TARANTOOL_ERROR? > + diag_set(ClientError, ER_SQL_EXECUTE, > + sql_value_text(pCtx->pOut)); > + } > + rc =3D SQL_TARANTOOL_ERROR; > } > sqlVdbeDeleteAuxData(db, &p->pAuxData, pCtx->iOp, = pOp->p1); > - if (rc) goto abort_due_to_error; > + if (rc !=3D 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 =3D &aMem[pOp->p1]; > - rc =3D ExpandBlob(pIn1); > - if (rc !=3D 0) > + if (ExpandBlob(pIn1) !=3D SQL_OK) !=3D 0 Please, don=E2=80=99t use SQL_OK value anywhere. >=20 > @@ -2802,10 +2784,8 @@ case OP_MakeRecord: { > uint32_t tuple_size; > char *tuple =3D > sql_vdbe_mem_encode_tuple(pData0, nField, &tuple_size, = region); > - if (tuple =3D=3D NULL) { > - rc =3D SQL_TARANTOOL_ERROR; > + if (tuple =3D=3D NULL) > goto abort_due_to_error; > - } > if ((int64_t)tuple_size > db->aLimit[SQL_LIMIT_LENGTH]) > goto too_big; >=20 >=20 > @@ -2918,8 +2898,10 @@ case OP_Savepoint: { > pSavepoint =3D pSavepoint->pNext > ); > if (!pSavepoint) { > - sqlVdbeError(p, "no such savepoint: %s", zName); > - rc =3D SQL_ERROR; > + const char *err =3D > + 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 =3D sqlCursorMovetoUnpacked(pC->uc.pCursor, pIdxKey, &res); > - if (pFree) sqlDbFree(db, pFree); > - if (rc!=3DSQL_OK) { > + if (pFree) !=3D NULL >=20 > @@ -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 =3D pCtx->isError; > + if (pCtx->isError !=3D SQL_TARANTOOL_ERROR) { > + diag_set(ClientError, ER_SQL_EXECUTE, > + sql_value_text(&t)); > + } > + rc =3D SQL_TARANTOOL_ERROR; > } > sqlVdbeMemRelease(&t); > - if (rc) goto abort_due_to_error; > + assert(rc =3D=3D SQL_OK || rc =3D=3D = SQL_TARANTOOL_ERROR); > + if (rc !=3D SQL_OK) > + goto abort_due_to_error; Looks like redundant diff > } else { > assert(t.flags=3D=3DMEM_Null); > } > @@ -5199,8 +5166,11 @@ case OP_AggFinal: { > pMem =3D &aMem[pOp->p1]; > assert((pMem->flags & ~(MEM_Null|MEM_Agg))=3D=3D0); > rc =3D sqlVdbeMemFinalize(pMem, pOp->p4.pFunc); > - if (rc) { > - sqlVdbeError(p, "%s", sql_value_text(pMem)); > + if (rc !=3D SQL_OK) { > + if (rc !=3D 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=E2=80=A6 The same relates to error in OP_AggStep. > @@ -5311,10 +5281,8 @@ case OP_IncMaxid: { > assert(pOp->p1 > 0); > pOut =3D &aMem[pOp->p1]; >=20 > - rc =3D tarantoolsqlIncrementMaxid((uint64_t*) &pOut->u.i); > - if (rc!=3DSQL_OK) { > + if (tarantoolsqlIncrementMaxid((uint64_t*) &pOut->u.i) !=3D = SQL_OK) > goto abort_due_to_error; > - } > pOut->flags =3D MEM_Int; > break; > } >=20 > too_big: > - sqlVdbeError(p, "string or blob too big"); > - rc =3D SQL_TOOBIG; > + diag_set(ClientError, ER_SQL_EXECUTE, "string or blob too = big=E2=80=9D); -> is too big > goto abort_due_to_error; >=20 > /* Jump to here if a malloc() fails. > */ > no_mem: > sqlOomFault(db); > - sqlVdbeError(p, "out of memory"); > - rc =3D 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 >=20 >=20 > @@ -550,13 +545,8 @@ void sqlVdbeMemPrettyPrint(Mem * pMem, char = *zBuf); > #endif > int sqlVdbeMemHandleBom(Mem * pMem); >=20 > -#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 >=20 > /** > * Perform comparison of two keys: one is packed and one is not. >=20 >=20 > 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 !=3D NULL && txn->psql_txn !=3D NULL && > txn->psql_txn->fk_deferred_count > 0) || > (!deferred && p->nFkConstraint > 0)) { > - p->rc =3D SQL_CONSTRAINT_FOREIGNKEY; > + p->rc =3D SQL_TARANTOOL_ERROR; > p->errorAction =3D ON_CONFLICT_ACTION_ABORT; > - sqlVdbeError(p, "FOREIGN KEY constraint failed"); > - return SQL_ERROR; > + diag_set(ClientError, ER_SQL_EXECUTE, "FOREIGN KEY = constraint "\ > + "failed=E2=80=9D); Please, reserve separate error code for this violation. > + return SQL_TARANTOOL_ERROR; > } > return SQL_OK; Return 0/-1