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 5E7BB2A019 for ; Fri, 15 Mar 2019 11:49:23 -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 pWSutOM_su1K for ; Fri, 15 Mar 2019 11:49:23 -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 E434F29D95 for ; Fri, 15 Mar 2019 11:49:22 -0400 (EDT) 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 v4 7/8] sql: rework semantic errors From: "n.pettik" In-Reply-To: <6dca562ff5163f5579f45d175b6cf188db52a09f.1552494059.git.imeevma@gmail.com> Date: Fri, 15 Mar 2019 18:49:20 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <53DBABF2-8730-4FF6-9C23-64BF3F9986D1@tarantool.org> References: <6dca562ff5163f5579f45d175b6cf188db52a09f.1552494059.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 > diff --git a/src/box/errcode.h b/src/box/errcode.h > index 7764aa3..a0d8f4ae 100644 > --- a/src/box/errcode.h > +++ b/src/box/errcode.h > @@ -122,7 +122,7 @@ struct errcode_record { > /* 67 */_(ER_REPLICA_ID_IS_RESERVED, "Can't initialize = replica id with a reserved value %u") \ > /* 68 */_(ER_INVALID_ORDER, "Invalid LSN order for = instance %u: previous LSN =3D %llu, new lsn =3D %llu") \ > /* 69 */_(ER_MISSING_REQUEST_FIELD, "Missing mandatory field = '%s' in request") \ > - /* 70 */_(ER_IDENTIFIER, "Invalid identifier '%s' = (expected printable symbols only)") \ > + /* 70 */_(ER_IDENTIFIER, "Invalid identifier '%s' = (expected printable symbols only or it is too long)") \ > /* 71 */_(ER_DROP_FUNCTION, "Can't drop function %u: = %s") \ > /* 72 */_(ER_ITERATOR_TYPE, "Unknown iterator type = '%s'") \ > /* 73 */_(ER_REPLICA_MAX, "Replica count limit = reached: %u") \ > @@ -240,6 +240,13 @@ struct errcode_record { > /*185 */_(ER_SQL_UNKNOWN_TOKEN, "Syntax error: = unrecognized token: '%.*s'") \ > /*186 */_(ER_SQL_PARSER_GENERIC, "%s") \ > /*187 */_(ER_SQL_ANALYZE_ARGUMENT, "ANALYZE statement = argument %s is not a base table") \ > + /*188 */_(ER_SQL_COLUMN_COUNT_MAX, "Failed to create space = '%s': space column count %d exceeds the limit (%d)") \ > + /*189 */_(ER_HEX_LITERAL_MAX, "Hex literal %s%s length = %d exceeds the supported limit (%d)") \ > + /*190 */_(ER_INT_LITERAL_MAX, "Integer literal %s%s = exceeds the supported range %lld - %lld") \ > + /*191 */_(ER_SQL_PARSER_LIMIT, "%s%.*s %d exceeds the = limit (%d)") \ You use auxiliary params only in one place. I guess it is worth removing them. > + /*192 */_(ER_INDEX_DEF, "%s prohibited in an = index definition") \ Why not ER_UNSUPPORTED? ER_INDEX_DEF -> ER_INDEX_DEF_UNSUPPORTED Also, quite unnatural message pattern: it is hard to understand diags like: diag_set(ClientError, ER_INDEX_DEF, "Parameter markers are"); > + /*193 */_(ER_CHECK_CONSTRAINT_DEF, "%s prohibited in a = CHECK constraint definition") \ The same question. ER_CHECK_CONSTRAINT_DEF -> ER_CK_DEF_UNSUPPORTED > + /*194 */_(ER_PRIMARY_KEY_DEF, "Expressions are = prohibited in a primary key definition") \ Just substitute it with ER_INDEX_DEF_UNSUPPORTED: expressions are prohibited in secondary index definition as well. >=20 > /* > * !IMPORTANT! Please follow instructions at start of the file > diff --git a/src/box/sql/build.c b/src/box/sql/build.c > index 0c06555..26434b1 100644 > --- a/src/box/sql/build.c > +++ b/src/box/sql/build.c > @@ -267,14 +267,14 @@ int > sqlCheckIdentifierName(Parse *pParse, char *zName) > { > ssize_t len =3D strlen(zName); > - > - if (len > BOX_NAME_MAX || identifier_check(zName, len) !=3D 0) { > - sqlErrorMsg(pParse, > - "identifier name is invalid: %s", > - zName); > - return SQL_ERROR; > + if (len <=3D BOX_NAME_MAX && identifier_check(zName, len) =3D=3D = 0) > + return SQL_OK; return 0; > + if (len > BOX_NAME_MAX) { > + diag_set(ClientError, ER_IDENTIFIER, > + tt_cstr(zName, BOX_INVALID_NAME_MAX)); > } > - return SQL_OK; > + pParse->is_aborted =3D true; > + return SQL_ERROR; return -1; ssize_t len =3D strlen(zName); - if (len <=3D BOX_NAME_MAX && identifier_check(zName, len) =3D=3D = 0) - return SQL_OK; if (len > BOX_NAME_MAX) { diag_set(ClientError, ER_IDENTIFIER, tt_cstr(zName, BOX_INVALID_NAME_MAX)); + pParse->is_aborted =3D true; + return -1; } - pParse->is_aborted =3D true; - return SQL_ERROR; + if (identifier_check(zName, len) !=3D 0) { + pParse->is_aborted =3D true; + return -1; + } + return 0; > @@ -588,9 +590,9 @@ sqlAddPrimaryKey(Parse * pParse, /* Parsing = context */ > if (space =3D=3D NULL) > goto primary_key_exit; > if (sql_space_primary_key(space) !=3D NULL) { > - sqlErrorMsg(pParse, > - "table \"%s\" has more than one primary = key", > - space->def->name); > + diag_set(ClientError, ER_CREATE_SPACE, space->def->name, > + "too many primary keys=E2=80=9D); too many primary keys -> primary key already exists > + pParse->is_aborted =3D true; > goto primary_key_exit; > } > if (pList =3D=3D NULL) { >=20 > @@ -1775,6 +1778,11 @@ sql_create_foreign_key(struct Parse = *parse_context, struct SrcList *child, > } > if (constraint_name =3D=3D NULL) > goto exit_create_fk; > + if (!is_self_referenced && parent_space->def->opts.is_view) { > + diag_set(ClientError, ER_CREATE_FK_CONSTRAINT, = constraint_name, > + "referenced space can't be VIEW"); > + goto tnt_error; > + } This check is already processed in on_replace_dd_fk_constraint() Could you remove this check to avoid code duplication? > @@ -2942,11 +2958,8 @@ sqlSavepoint(Parse * pParse, int op, Token * = pName) > return; > } > if (op =3D=3D SAVEPOINT_BEGIN && > - sqlCheckIdentifierName(pParse, zName) > - !=3D SQL_OK) { > - sqlErrorMsg(pParse, "bad savepoint name"); > + sqlCheckIdentifierName(pParse, zName) !=3D SQL_OK) !=3D 0 > diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c > index 87d4ed4..e8a5f0d 100644 > --- a/src/box/sql/delete.c > +++ b/src/box/sql/delete.c > @@ -157,8 +157,9 @@ sql_table_delete_from(struct Parse *parse, struct = SrcList *tab_list, > goto delete_from_cleanup; >=20 > if (trigger_list =3D=3D NULL) { > - sqlErrorMsg(parse, "cannot modify %s because it = is a" > - " view", space->def->name); > + diag_set(ClientError, ER_ALTER_SPACE, = space->def->name, > + "it is a view=E2=80=9D); Nit: it is a view -> space is a view (sounds better IMHO). > + parse->is_aborted =3D true; > goto delete_from_cleanup; > } > } > diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c > index 4193596..de06ee0 100644 > --- a/src/box/sql/expr.c > +++ b/src/box/sql/expr.c > @@ -750,15 +750,14 @@ codeVectorCompare(Parse * pParse, /* Code = generator context */ > int > sqlExprCheckHeight(Parse * pParse, int nHeight) > { > - int rc =3D SQL_OK; > int mxHeight =3D pParse->db->aLimit[SQL_LIMIT_EXPR_DEPTH]; > if (nHeight > mxHeight) { > - sqlErrorMsg(pParse, > - "Expression tree is too large (maximum = depth %d)", > - mxHeight); > - rc =3D SQL_ERROR; > + diag_set(ClientError, ER_SQL_PARSER_LIMIT, "Number of = nodes "\ > + "in expression tree", 0, "", nHeight, = mxHeight); > + pParse->is_aborted =3D true; > + return SQL_ERROR; return -1; > } > - return rc; > + return SQL_OK; return 0; >=20 > @@ -690,9 +674,12 @@ resolveExprStep(Walker * pWalker, Expr * pExpr) > * that might change over time = cannot be used > * in an index. > */ > - notValid(pParse, pNC, > - "non-deterministic = functions", > - NC_IdxExpr); > + if (pNC->ncFlags & NC_IdxExpr) { > + diag_set(ClientError, = ER_INDEX_DEF, > + = "Non-deterministic functions "\ > + "are=E2=80=9D); This code seems to be unreachable: functional indexes are filtered a way earlier. > @@ -754,8 +741,16 @@ resolveExprStep(Walker * pWalker, Expr * pExpr) > testcase(pExpr->op =3D=3D TK_IN); > if (ExprHasProperty(pExpr, EP_xIsSelect)) { > int nRef =3D pNC->nRef; > - notValid(pParse, pNC, "subqueries", > - NC_IsCheck | NC_IdxExpr); > + if (pNC->ncFlags & NC_IdxExpr) { > + diag_set(ClientError, = ER_INDEX_DEF, > + "Subqueries are=E2=80=9D)= ; The same: managed to grep this error only for ck constraint. > @@ -1042,27 +1044,34 @@ resolveCompoundOrderBy(Parse * pParse, = /* Parsing context. Leave error messages > * field) then convert that term into a copy of the corresponding = result set > * column. > * > - * If any errors are detected, add an error message to pParse and > - * return non-zero. Return zero if no errors are seen. > + * @param pParse Parsing context. > + * @param pSelect The SELECT statement containing the clause. > + * @param pOrderBy The ORDER BY or GROUP BY clause to be > + * processed. > + * @param is_order_by True if pOrderBy is ORDER BY, false if > + * pOrderBy is GROUP BY > + * @retval 0 On success, not 0 elsewhere. > */ > int > -sqlResolveOrderGroupBy(Parse * pParse, /* Parsing context. = Leave error messages here */ > - Select * pSelect, /* The SELECT statement = containing the clause */ > - ExprList * pOrderBy, /* The ORDER BY or GROUP = BY clause to be processed */ > - const char *zType /* "ORDER" or "GROUP" */ > - ) > +sqlResolveOrderGroupBy(Parse *pParse, Select *pSelect, ExprList = *pOrderBy, > + bool is_order_by) Why did you decide to fix code style here? Anyway, you didn't finished = it (struct prefixes, param naming and so on and so forth) > { > int i; > sql *db =3D pParse->db; > ExprList *pEList; > struct ExprList_item *pItem; > + const char *zType =3D is_order_by ? "ORDER" : "GROUP"; >=20 > if (pOrderBy =3D=3D 0 || pParse->db->mallocFailed) > return 0; >=20 > @@ -1096,22 +1105,23 @@ sqlResolveOrderGroupBy(Parse * pParse, = /* Parsing context. Leave error messages > * result-set expression. Otherwise, the expression is resolved in > * the usual way - using sqlResolveExprNames(). > * > - * This routine returns the number of errors. If errors occur, then > - * an appropriate error message might be left in pParse. (OOM errors > - * excepted.) > + * @param pNC The name context of the SELECT statement. > + * @param pSelect The SELECT statement containing the clause. > + * @param pOrderBy An ORDER BY or GROUP BY clause to resolve. > + * @param is_order_by True if pOrderBy is ORDER BY, false if > + * pOrderBy is GROUP BY > + * @retval 0 On success, not 0 elsewhere. > */ > static int > -resolveOrderGroupBy(NameContext * pNC, /* The name context of = the SELECT statement */ > - Select * pSelect, /* The SELECT statement holding = pOrderBy */ > - ExprList * pOrderBy, /* An ORDER BY or GROUP = BY clause to resolve */ > - const char *zType /* Either "ORDER" or "GROUP", as = appropriate */ > - ) The same question. > +resolveOrderGroupBy(NameContext *pNC, Select *pSelect, ExprList = *pOrderBy, > + bool is_order_by) > { > int i, j; /* Loop counters */ > int iCol; /* Column number */ > struct ExprList_item *pItem; /* A term of the ORDER BY clause = */ > Parse *pParse; /* Parsing context */ > int nResult; /* Number of terms in the result set */ > + const char *zType =3D is_order_by ? "ORDER" : "GROUP"; >=20 > if (pOrderBy =3D=3D 0) > return 0; >=20 > /* Compute the limit registers */ > computeLimitRegisters(pParse, p, labelEnd); > diff --git a/src/box/sql/update.c b/src/box/sql/update.c > index 05ceeb4..bc0ab66 100644 > --- a/src/box/sql/update.c > +++ b/src/box/sql/update.c > @@ -139,8 +139,9 @@ sqlUpdate(Parse * pParse, /* The parser = context */ > goto update_cleanup; > } > if (is_view && tmask =3D=3D 0) { > - sqlErrorMsg(pParse, "cannot modify %s because it is a = view", > - space->def->name); > + diag_set(ClientError, ER_ALTER_SPACE, space->def->name, > + "it is a view=E2=80=9D); -> space is a view > + pParse->is_aborted =3D true; > goto update_cleanup; > } >=20 >=20 > diff --git a/test/sql-tap/check.test.lua b/test/sql-tap/check.test.lua > index 0d8bf15..dd2de92 100755 > --- a/test/sql-tap/check.test.lua > +++ b/test/sql-tap/check.test.lua > @@ -319,7 +319,7 @@ test:do_catchsql_test( > ); > ]], { > -- > - 1, "Failed to create space 'T3': subqueries prohibited in = CHECK constraints" > + 1, "Failed to create space 'T3': Subqueries are prohibited in = a CHECK constraint definition=E2=80=9D Don=E2=80=99t use capital letter after =E2=80=98:=E2=80=99, it would = allow you to avoid fixing tests. The same is applied to the fixes below. > -- > }) >=20 > diff --git a/test/sql-tap/colname.test.lua = b/test/sql-tap/colname.test.lua > index 29fdf13..77c4280 100755 > --- a/test/sql-tap/colname.test.lua > +++ b/test/sql-tap/colname.test.lua > @@ -637,7 +637,7 @@ test:do_test( > test:do_catchsql_test( > "colname-11.1", > [[ create table t1(a INT, b INT, c INT, primary key('A'))]], > - {1, "expressions prohibited in PRIMARY KEY"}) > + {1, "Expressions are prohibited in a primary key definition"}) >=20 > test:do_catchsql_test( > "colname-11.2", > diff --git a/test/sql-tap/default.test.lua = b/test/sql-tap/default.test.lua > index 67151b0..fbe285b 100755 > --- a/test/sql-tap/default.test.lua > +++ b/test/sql-tap/default.test.lua > @@ -66,7 +66,7 @@ test:do_catchsql_test( > ); > ]], { > -- > - 1, "default value of column [Y] is not constant" > + 1, "Failed to create space 'T3': default value of column is not = constant=E2=80=9D Is it possible to print also name of column (like in original version)? > diff --git a/test/sql-tap/engine.cfg b/test/sql-tap/engine.cfg > index 413dd3f..cc56592 100644 > --- a/test/sql-tap/engine.cfg > +++ b/test/sql-tap/engine.cfg > @@ -11,6 +11,9 @@ > "sort.test.lua": { > "memtx": {"engine": "memtx"} > }, > + "sql-errors.test.lua": { > + "memtx": {"engine": "memtx"} > + }, I would run this test both for vinyl and memtx.