[tarantool-patches] Re: [PATCH v9 2/7] sql: fix error code for SQL errors in execute.c

[Vladislav Shpilevoy]

Hi! Thanks for the patch!

On 22/03/2019 13:50, imeevma at 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);