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 7FA9F2F131 for ; Sat, 25 May 2019 05:16:48 -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 xmq03gLT7PZJ for ; Sat, 25 May 2019 05:16:48 -0400 (EDT) Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (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 BDE642F07D for ; Sat, 25 May 2019 05:16:47 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v1 01/12] sql: remove errors SQL_TARANTOOL_*_FAIL References: <4e95d48acff0665630f766d394a36763582ef00b.1557056617.git.imeevma@gmail.com> <31ACAB6A-811A-46AC-8D35-B30524A85FE8@tarantool.org> From: Imeev Mergen Message-ID: <33554197-bc36-1792-75a1-027fb0d8dce0@tarantool.org> Date: Sat, 25 May 2019 12:16:44 +0300 MIME-Version: 1.0 In-Reply-To: <31ACAB6A-811A-46AC-8D35-B30524A85FE8@tarantool.org> Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Transfer-Encoding: 8bit Content-Language: en-US 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: "n.pettik" , tarantool-patches@freelists.org Hi! Thank you for review! My answers below. On 5/15/19 4:18 PM, n.pettik wrote: > >> On 5 May 2019, at 15:17, imeevma@tarantool.org wrote: >> >> Errors SQL_TARANTOOL_DELETE_FAIL, SQL_TARANTOOL_ITERATOR_FAIL and >> SQL_TARANTOOL_INSERT_FAIL have almost no functionality, but can > Nit: they are not errors, but rather error codes. Fixed. > >> diff --git a/src/box/sql.c b/src/box/sql.c >> index 1fb93e1..3593242 100644 >> --- a/src/box/sql.c >> +++ b/src/box/sql.c >> @@ -162,15 +162,6 @@ const char *tarantoolErrorMessage() >> return box_error_message(box_error_last()); >> } >> >> const void *tarantoolsqlPayloadFetch(BtCursor *pCur, u32 *pAmt) >> { >> assert(pCur->curFlags & BTCF_TaCursor || >> @@ -421,7 +412,7 @@ int tarantoolsqlEphemeralInsert(struct space *space, const char *tuple, >> assert(space != NULL); >> mp_tuple_assert(tuple, tuple_end); >> if (space_ephemeral_replace(space, tuple, tuple_end) != 0) >> - return SQL_TARANTOOL_INSERT_FAIL; >> + return SQL_TARANTOOL_ERROR; > Let’s return -1 and set tarantool_error only in VDBE. > Later, we are going to replace tarantool_error with -1 everywhere. > The same concerns other tarantool_error usages in sql.c I think it's better to leave it as it is. A bit later there will be a patch-set, in which all SQL error codes will be replaced with either 0 or -1. > >> diff --git a/src/box/sql/tarantoolInt.h b/src/box/sql/tarantoolInt.h >> index 2b04d96..f15e147 100644 >> --- a/src/box/sql/tarantoolInt.h >> +++ b/src/box/sql/tarantoolInt.h >> @@ -13,8 +13,6 @@ struct fk_constraint_def; >> /* Misc */ >> const char *tarantoolErrorMessage(); >> >> -int is_tarantool_error(int rc); >> - >> /* Storage interface. */ >> const void *tarantoolsqlPayloadFetch(BtCursor * pCur, u32 * pAmt); >> >> @@ -51,8 +49,7 @@ int tarantoolsqlDelete(BtCursor * pCur, u8 flags); >> * @param key Key of record to be deleted. >> * @param key_size Size of key. >> * >> - * @retval SQL_OK on success, SQL_TARANTOOL_DELETE_FAIL >> - * otherwise. >> + * @retval SQL_OK on success, SQL_TARANTOOL_ERROR otherwise. >> */ >> int >> sql_delete_by_key(struct space *space, uint32_t iid, char *key, >> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c >> index 3bd8223..5222a4e 100644 >> --- a/src/box/sql/vdbe.c >> +++ b/src/box/sql/vdbe.c >> @@ -5469,7 +5469,7 @@ abort_due_to_error: >> /* Avoiding situation when Tarantool error is set, >> * but error message isn't. >> */ >> - if (is_tarantool_error(rc) && tarantoolErrorMessage()) { >> + if (rc == SQL_TARANTOOL_ERROR && tarantoolErrorMessage()) { > Isn’t tarantoolErrorMessage redundant check? Is it possible > to use SQL_TARANTOOL_ERROR without set diag? It is technically possible to install SQL_TARANTOOL_ERROR without set diag. This code will be fixed in one of the following patches of this patch-set.