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 EAC052747D for ; Fri, 15 Feb 2019 07:44:54 -0500 (EST) 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 KbC3VydXzsrd for ; Fri, 15 Feb 2019 07:44:54 -0500 (EST) Received: from smtp43.i.mail.ru (smtp43.i.mail.ru [94.100.177.103]) (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 9CC9620A40 for ; Fri, 15 Feb 2019 07:44:54 -0500 (EST) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 12.2 \(3445.102.3\)) Subject: [tarantool-patches] Re: [PATCH v1 1/1] sql: rework "no such object" and "object exists" errors From: "n.pettik" In-Reply-To: Date: Fri, 15 Feb 2019 15:44:51 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <168FF7F0-A4AA-42DD-AF9A-FA95098ADBBA@tarantool.org> References: 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 , Konstantin Osipov > diff --git a/src/box/errcode.h b/src/box/errcode.h > index f7dbb94..e25e05c 100644 > --- a/src/box/errcode.h > +++ b/src/box/errcode.h > @@ -87,7 +87,7 @@ struct errcode_record { > /* 32 */_(ER_PROC_LUA, "%s") \ > /* 33 */_(ER_NO_SUCH_PROC, "Procedure '%.*s' is not = defined") \ > /* 34 */_(ER_NO_SUCH_TRIGGER, "Trigger '%s' doesn't = exist") \ > - /* 35 */_(ER_NO_SUCH_INDEX, "No index #%u is defined = in space '%s'") \ > + /* 35 */_(ER_NO_SUCH_INDEX_ID, "No index #%u is defined = in space '%s'") \ > /* 36 */_(ER_NO_SUCH_SPACE, "Space '%s' does not = exist") \ > /* 37 */_(ER_NO_SUCH_FIELD, "Field %d was not found = in the tuple") \ > /* 38 */_(ER_EXACT_FIELD_COUNT, "Tuple field count %u = does not match space field count %u") \ > @@ -200,12 +200,12 @@ struct errcode_record { > /*145 */_(ER_NO_SUCH_SEQUENCE, "Sequence '%s' does not = exist") \ > /*146 */_(ER_SEQUENCE_EXISTS, "Sequence '%s' already = exists") \ > /*147 */_(ER_SEQUENCE_OVERFLOW, "Sequence '%s' has = overflowed") \ > - /*148 */_(ER_UNUSED5, "") \ > + /*148 */_(ER_NO_SUCH_INDEX_NAME, "No index '%s' is = defined in space '%s'") \ I=E2=80=99d better say =E2=80=99No index with name =E2=80=A6=E2=80=99. But it is to be discussed. > /*149 */_(ER_SPACE_FIELD_IS_DUPLICATE, "Space field '%s' is = duplicate") \ > /*150 */_(ER_CANT_CREATE_COLLATION, "Failed to initialize = collation: %s.") \ > /*151 */_(ER_WRONG_COLLATION_OPTIONS, "Wrong collation options = (field %u): %s") \ > /*152 */_(ER_NULLABLE_PRIMARY, "Primary index of the = space '%s' can not contain nullable parts") \ > - /*153 */_(ER_UNUSED, "") \ > + /*153 */_(ER_NO_SUCH_FIELD_NAME, "Field '%s' doesn't = exist") \ Mb it is better to split this error into two: =E2=80=9CSpace %s doesn=E2=80=99t feature field with name %s=E2=80=9D = and =E2=80=9CUnresolved column name %s=E2=80=9D Again, I=E2=80=99m neither SQL expert nor technical writer, so it is not = up to me. > diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h > index 7f17fd8..daeb02c 100644 > --- a/src/box/sql/sqlInt.h > +++ b/src/box/sql/sqlInt.h > @@ -3270,6 +3270,14 @@ int sqlKeywordCode(const unsigned char *, int); > int sqlRunParser(Parse *, const char *, char **); >=20 > /** > + * Increment error counter if error suppression isn't set. > + * > + * @param parse_context Current parsing context. > + */ > +void > +sql_parser_error(struct Parse *parse_context); I suggest to incapsulate call of diag set inside this function. To achieve this, lets make function take variadic params. It would allow us to reduce size of refactored code, at least. > + > +/** > * This routine is called after a single SQL statement has been > * parsed and a VDBE program to execute that statement has been > * prepared. This routine puts the finishing touches on the > diff --git a/src/box/sql/update.c b/src/box/sql/update.c > index ba24283..d203604 100644 > --- a/src/box/sql/update.c > +++ b/src/box/sql/update.c > @@ -191,8 +191,9 @@ sqlUpdate(Parse * pParse, /* The parser = context */ > } > } > if (j >=3D (int)def->field_count) { > - sqlErrorMsg(pParse, "no such column: %s", > - pChanges->a[i].zName); > + diag_set(ClientError, ER_NO_SUCH_FIELD_NAME, > + pChanges->a[i].zName); > + sql_parser_error(pParse); > goto update_cleanup; > } > } > diff --git a/src/box/sql/util.c b/src/box/sql/util.c > index dadae18..ad7bb1e 100644 > --- a/src/box/sql/util.c > +++ b/src/box/sql/util.c > @@ -246,6 +246,16 @@ sqlErrorMsg(Parse * pParse, const char *zFormat, = ...) > } > } >=20 > +void > +sql_parser_error(struct Parse *parse_context) > +{ > + if (parse_context->db->suppressErr) > + return; As Konstantin pointed out, seems like we don=E2=80=99t need taking into consideration suppressErr here. > + parse_context->nErr++; > + parse_context->rc =3D SQL_TARANTOOL_ERROR; > +} > + > + Extra blank line. > diff --git a/test/sql/checks.result b/test/sql/checks.result > index 12a3aa1..2eafae8 100644 > --- a/test/sql/checks.result > +++ b/test/sql/checks.result > @@ -111,11 +111,11 @@ s =3D box.space._space:insert(t) > -- > box.sql.execute("CREATE TABLE w2 (s1 INT PRIMARY KEY, CHECK ((SELECT = COUNT(*) FROM w2) =3D 0));") > --- > -- error: 'Failed to create space ''W2'': SQL error: no such table: = W2' > +- error: 'Failed to create space ''W2'': Space ''W2'' does not = exist=E2=80=99 Hm, IMHO looks a bit misleading. Could we improve error reporting in this case? It would be nice to see smth like =E2=80=9Cname cannot be resolved=E2=80=9D= or =E2=80=9CMisuse of space forward declaration=E2=80=9D.