[tarantool-patches] Re: [PATCH v4 7/8] sql: rework semantic errors
n.pettik
korablev at tarantool.org
Fri Mar 15 18:49:20 MSK 2019
> 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 = %llu, new lsn = %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.
>
> /*
> * !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 = strlen(zName);
> -
> - if (len > BOX_NAME_MAX || identifier_check(zName, len) != 0) {
> - sqlErrorMsg(pParse,
> - "identifier name is invalid: %s",
> - zName);
> - return SQL_ERROR;
> + if (len <= BOX_NAME_MAX && identifier_check(zName, len) == 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 = true;
> + return SQL_ERROR;
return -1;
ssize_t len = strlen(zName);
- if (len <= BOX_NAME_MAX && identifier_check(zName, len) == 0)
- return SQL_OK;
if (len > BOX_NAME_MAX) {
diag_set(ClientError, ER_IDENTIFIER,
tt_cstr(zName, BOX_INVALID_NAME_MAX));
+ pParse->is_aborted = true;
+ return -1;
}
- pParse->is_aborted = true;
- return SQL_ERROR;
+ if (identifier_check(zName, len) != 0) {
+ pParse->is_aborted = true;
+ return -1;
+ }
+ return 0;
> @@ -588,9 +590,9 @@ sqlAddPrimaryKey(Parse * pParse, /* Parsing context */
> if (space == NULL)
> goto primary_key_exit;
> if (sql_space_primary_key(space) != 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”);
too many primary keys -> primary key already exists
> + pParse->is_aborted = true;
> goto primary_key_exit;
> }
> if (pList == NULL) {
>
> @@ -1775,6 +1778,11 @@ sql_create_foreign_key(struct Parse *parse_context, struct SrcList *child,
> }
> if (constraint_name == 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 == SAVEPOINT_BEGIN &&
> - sqlCheckIdentifierName(pParse, zName)
> - != SQL_OK) {
> - sqlErrorMsg(pParse, "bad savepoint name");
> + sqlCheckIdentifierName(pParse, zName) != SQL_OK)
!= 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;
>
> if (trigger_list == 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”);
Nit: it is a view -> space is a view (sounds better IMHO).
> + parse->is_aborted = 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 = SQL_OK;
> int mxHeight = pParse->db->aLimit[SQL_LIMIT_EXPR_DEPTH];
> if (nHeight > mxHeight) {
> - sqlErrorMsg(pParse,
> - "Expression tree is too large (maximum depth %d)",
> - mxHeight);
> - rc = SQL_ERROR;
> + diag_set(ClientError, ER_SQL_PARSER_LIMIT, "Number of nodes "\
> + "in expression tree", 0, "", nHeight, mxHeight);
> + pParse->is_aborted = true;
> + return SQL_ERROR;
return -1;
> }
> - return rc;
> + return SQL_OK;
return 0;
>
> @@ -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”);
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 == TK_IN);
> if (ExprHasProperty(pExpr, EP_xIsSelect)) {
> int nRef = pNC->nRef;
> - notValid(pParse, pNC, "subqueries",
> - NC_IsCheck | NC_IdxExpr);
> + if (pNC->ncFlags & NC_IdxExpr) {
> + diag_set(ClientError, ER_INDEX_DEF,
> + "Subqueries are”);
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 = pParse->db;
> ExprList *pEList;
> struct ExprList_item *pItem;
> + const char *zType = is_order_by ? "ORDER" : "GROUP";
>
> if (pOrderBy == 0 || pParse->db->mallocFailed)
> return 0;
>
> @@ -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 = is_order_by ? "ORDER" : "GROUP";
>
> if (pOrderBy == 0)
> return 0;
>
> /* 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 == 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”);
-> space is a view
> + pParse->is_aborted = true;
> goto update_cleanup;
> }
>
>
> 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(
> );
> ]], {
> -- <check-3.1>
> - 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”
Don’t use capital letter after ‘:’, it would allow you to avoid fixing tests.
The same is applied to the fixes below.
> -- </check-3.1>
> })
>
> 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"})
>
> 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(
> );
> ]], {
> -- <default-1.3>
> - 1, "default value of column [Y] is not constant"
> + 1, "Failed to create space 'T3': default value of column is not constant”
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.
More information about the Tarantool-patches
mailing list