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 4EB842B823 for ; Mon, 3 Jun 2019 08:10:19 -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 7ziWtuLcyeIc for ; Mon, 3 Jun 2019 08:10:19 -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 9DF5D2AEC3 for ; Mon, 3 Jun 2019 08:10:18 -0400 (EDT) Date: Mon, 3 Jun 2019 15:10:10 +0300 From: Mergen Imeev Subject: [tarantool-patches] Re: [PATCH v1 9/9] sql: set errors in VDBE using diag_set() Message-ID: <20190603121010.GC24317@tarantool.org> References: <2fb295d4-473a-eb96-1f32-c12b2a0e96aa@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <2fb295d4-473a-eb96-1f32-c12b2a0e96aa@tarantool.org> 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: Vladislav Shpilevoy Cc: tarantool-patches@freelists.org On Sun, Jun 02, 2019 at 06:34:12PM +0200, Vladislav Shpilevoy wrote: > > > diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c > > index 43d7329..5bf5e6e 100644 > > --- a/src/box/sql/vdbe.c > > +++ b/src/box/sql/vdbe.c > > @@ -3903,7 +3852,8 @@ case OP_SorterCompare: { > > pIn3 = &aMem[pOp->p3]; > > nKeyCol = pOp->p4.i; > > res = 0; > > - rc = sqlVdbeSorterCompare(pC, pIn3, nKeyCol, &res); > > + if (sqlVdbeSorterCompare(pC, pIn3, nKeyCol, &res) != 0) > > + rc = SQL_TARANTOOL_ERROR; > > In all the similar places you remove SQL_TARANTOOL_ERROR, > but here you added it. Why? > I thought the next command was doing something useful and should be executed in any case. Now I see that I was wrong. > > VdbeBranchTaken(res!=0,2); > > if (rc) goto abort_due_to_error; > > if (res) goto jump_to_p2; > > @@ -4137,15 +4085,18 @@ case OP_Rewind: { /* jump */ > > pC->seekOp = OP_Rewind; > > #endif > > if (isSorter(pC)) { > > - rc = sqlVdbeSorterRewind(pC, &res); > > + if (sqlVdbeSorterRewind(pC, &res) != SQL_OK) > > + goto abort_due_to_error; > > } else { > > assert(pC->eCurType==CURTYPE_TARANTOOL); > > pCrsr = pC->uc.pCursor; > > assert(pCrsr); > > - rc = tarantoolsqlFirst(pCrsr, &res); > > + if (tarantoolsqlFirst(pCrsr, &res) != SQL_OK) > > + rc = SQL_TARANTOOL_ERROR; > > The same. > As in the previous case, I thought that pC->cacheStatus should be set, but now I see that it is checked only in OP_Column, and in case of an error, it will not be checked at all. > Consider my review fixes on the branch and below. They are motivated > by several points: > > 1) in the original code there were places comparing 'rc' with 0, but > you replaced them with 'rc ==/!= SQL_OK'. I rolled back such changes, > because we move towards removal of SQL_OK. So when you need to choose > between 0 and SQL_OK - use 0. > > 2) there were places using SQL_OK, but very easy to fix and compare > with 0. I did it. > > 3) in a couple of places you kept 'rc = SQL_TARANTOOL_ERROR'. I don't > know why, but I dropped it, and the tests passed. If I was wrong, tell > me, please. > Thank you, I squashed your patch. Actually, all of SQL errcodes are removed in the follow-up patch-set.