[tarantool-patches] Re: [PATCH v1 12/12] sql: use diag_set() to set an error in SQL functions

["n.pettik" <korablev@tarantool.org>]

> After this patch, all errors in the SQL functions will be set
> using diag_set().
> 
> Closes #4074
> ---
> src/box/lua/lua_sql.c |  13 +++--
> src/box/sql/analyze.c |   6 +--
> src/box/sql/func.c    | 104 ++++++++++++++++++----------------------
> src/box/sql/sqlInt.h  |  13 -----
> src/box/sql/vdbe.c    |  34 +++----------
> src/box/sql/vdbeInt.h |  28 ++---------
> src/box/sql/vdbeapi.c | 129 +++++---------------------------------------------
> src/box/sql/vdbeaux.c |  46 ------------------
> src/box/sql/vdbemem.c |   9 ++--
> 9 files changed, 84 insertions(+), 298 deletions(-)
> 
> diff --git a/src/box/lua/lua_sql.c b/src/box/lua/lua_sql.c
> index d28045a..afe4732 100644
> --- a/src/box/lua/lua_sql.c
> +++ b/src/box/lua/lua_sql.c
> @@ -77,13 +77,15 @@ lua_sql_call(sql_context *pCtx, int nVal, sql_value **apVal) {
> 			lua_pushboolean(L, sql_value_boolean(param));
> 			break;
> 		default:
> -			sql_result_error(pCtx, "Unsupported type passed "
> -					     "to Lua", -1);

Please, remove sql_result_error at all: I grepped several
usages among dead code.

> +			diag_set(ClientError, ER_SQL_EXECUTE, "Unsupported "\
> +				 "type passed to Lua");
> +			pCtx->is_error = true;
> 			goto error;
> 		}
> 
> diff --git a/src/box/sql/func.c b/src/box/sql/func.c
> index bb7405e..16c02f1 100644
> --- a/src/box/sql/func.c
> +++ b/src/box/sql/func.c
> @@ -181,13 +181,9 @@ absFunc(sql_context * context, int argc, sql_value ** argv)
> 			i64 iVal = sql_value_int64(argv[0]);
> 			if (iVal < 0) {
> 				if (iVal == SMALLEST_INT64) {
> -					/* IMP: R-31676-45509 If X is the integer -9223372036854775808
> -					 * then abs(X) throws an integer overflow error since there is no
> -					 * equivalent positive 64-bit two complement value.
> -					 */
> -					sql_result_error(context,
> -							     "integer overflow",
> -							     -1);
> +					diag_set(ClientError, ER_SQL_EXECUTE,
> +						 "integer overflow”);

-> integer is overflowed.

> +					context->is_error = true;
> 					return;
> 				}
> 				iVal = -iVal;
> 
> 
> 
> @@ -591,11 +577,11 @@ case_type##ICUFunc(sql_context *context, int argc, sql_value **argv)   \
> 	 * does not invalidate the _text() pointer.                            \
> 	 */                                                                    \
> 	assert(z2 == (char *)sql_value_text(argv[0]));                     \
> -	if (!z2)                                                               \
> +	if (z2 == NULL)                                                        \
> 		return;                                                        \

Redundant diff.

> src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h
> index 6aadca2..70a0bab 100644
> --- a/src/box/sql/vdbeInt.h
> +++ b/src/box/sql/vdbeInt.h
> 
> @@ -300,21 +296,6 @@ mem_apply_numeric_type(struct Mem *record);
> #endif
> 
> 
> -/*
>  * The "context" argument for an installable function.  A pointer to an
>  * instance of this structure is the first argument to the routines used
>  * implement the SQL functions.
> @@ -333,9 +314,12 @@ struct sql_context {
> 	Mem *pMem;		/* Memory cell used to store aggregate context */
> 	Vdbe *pVdbe;		/* The VM that owns this context */
> 	int iOp;		/* Instruction number of OP_Function */
> -	int isError;		/* Error code returned by the function. */
> +	/*
> +	 * True, if an error occurred during the execution of the
> +	 * function.
> +	 */
> +	bool is_error;

I’d better use is_abotred name.

> diff --git a/src/box/sql/vdbemem.c b/src/box/sql/vdbemem.c
> index f73ea0a..ce0c641 100644
> --- a/src/box/sql/vdbemem.c
> +++ b/src/box/sql/vdbemem.c
> @@ -321,7 +321,6 @@ sqlVdbeMemStringify(Mem * pMem, u8 bForce)
> int
> sqlVdbeMemFinalize(Mem * pMem, FuncDef * pFunc)
> {
> -	int rc = SQL_OK;
> 	if (ALWAYS(pFunc && pFunc->xFinalize)) {
> 		sql_context ctx;
> 		Mem t;
> @@ -338,9 +337,9 @@ sqlVdbeMemFinalize(Mem * pMem, FuncDef * pFunc)
> 		if (pMem->szMalloc > 0)
> 			sqlDbFree(pMem->db, pMem->zMalloc);
> 		memcpy(pMem, &t, sizeof(t));
> -		rc = ctx.isError;
> +		return ctx.is_error ? SQL_TARANTOOL_ERROR : SQL_OK;
> 	}
> -	return rc;
> +	return SQL_OK;

Return 0/-1