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 03E3928F2E for ; Tue, 26 Mar 2019 17:48:31 -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 W7Drlm9dgCb9 for ; Tue, 26 Mar 2019 17:48:30 -0400 (EDT) Received: from smtp52.i.mail.ru (smtp52.i.mail.ru [94.100.177.112]) (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 AFE9928EE1 for ; Tue, 26 Mar 2019 17:48:30 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v9 2/7] sql: fix error code for SQL errors in execute.c References: From: Vladislav Shpilevoy Message-ID: Date: Wed, 27 Mar 2019 00:48:27 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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, imeevma@tarantool.org Hi! Thanks for the patch! On 22/03/2019 13:50, imeevma@tarantool.org wrote: > Currently, functions sql_execute() and sql_prepare_and_execute() > set the ER_SQL_EXECUTE code for all errors that occur during the > execution of a SQL command. This is considered incorrect because > some of these errors may have their own error code. > > In addition, all errors that do not have an error code are VDBE > errors due to issue #3965, so it makes sense to set the error > code ER_VDBE_EXECUTE for all errors without an error code. > > Part of #3505 > --- > src/box/errcode.h | 1 + > src/box/execute.c | 14 ++++++++-- > test/box/misc.result | 1 + > test/sql/collation.result | 2 +- > test/sql/gh-2362-select-access-rights.result | 12 +++------ > test/sql/iproto.result | 39 ++++++++++++---------------- > 6 files changed, 35 insertions(+), 34 deletions(-) > > diff --git a/src/box/execute.c b/src/box/execute.c > index 7c77df2..5810086 100644 > --- a/src/box/execute.c > +++ b/src/box/execute.c > @@ -541,7 +546,12 @@ sql_prepare_and_execute(const char *sql, int len, const struct sql_bind *bind, > return -1; > } > if (sql_prepare_v2(db, sql, len, &stmt, NULL) != SQL_OK) { > - diag_set(ClientError, ER_SQL_EXECUTE, sql_errmsg(db)); > + if (db->errCode != SQL_TARANTOOL_ERROR) { > + const char *err = (char *)sql_value_text(db->pErr); > + if (err == NULL) > + err = sqlErrStr(db->errCode); > + diag_set(ClientError, ER_VDBE_EXECUTE, err); Prepare has nothing to do with Vdbe except allocation. Error says "Error during execution of VDBE", but VDBE is not executed here. Also, not sure if 'VDBE' should be showed to a user. He is likely not to be aware about what it is. In my opinion, ER_VDBE_EXECUTE should be replaced with ER_SQL_EXECUTE, and the message should be "Error during execution of SQL bytecode: ...". > + } > return -1; > } > assert(stmt != NULL);