[tarantool-patches] Re: [PATCH v1 01/12] sql: remove errors SQL_TARANTOOL_*_FAIL

[n.pettik]

> On 5 May 2019, at 15:17, imeevma at tarantool.org wrote:
> 
> Errors SQL_TARANTOOL_DELETE_FAIL, SQL_TARANTOOL_ITERATOR_FAIL and
> SQL_TARANTOOL_INSERT_FAIL have almost no functionality, but can

Nit: they are not errors, but rather error codes.

> diff --git a/src/box/sql.c b/src/box/sql.c
> index 1fb93e1..3593242 100644
> --- a/src/box/sql.c
> +++ b/src/box/sql.c
> @@ -162,15 +162,6 @@ const char *tarantoolErrorMessage()
> 	return box_error_message(box_error_last());
> }
> 
> const void *tarantoolsqlPayloadFetch(BtCursor *pCur, u32 *pAmt)
> {
> 	assert(pCur->curFlags & BTCF_TaCursor ||
> @@ -421,7 +412,7 @@ int tarantoolsqlEphemeralInsert(struct space *space, const char *tuple,
> 	assert(space != NULL);
> 	mp_tuple_assert(tuple, tuple_end);
> 	if (space_ephemeral_replace(space, tuple, tuple_end) != 0)
> -		return SQL_TARANTOOL_INSERT_FAIL;
> +		return SQL_TARANTOOL_ERROR;

Let’s return -1 and set tarantool_error only in VDBE.
Later, we are going to replace tarantool_error with -1 everywhere.
The same concerns other tarantool_error usages in sql.c

> diff --git a/src/box/sql/tarantoolInt.h b/src/box/sql/tarantoolInt.h
> index 2b04d96..f15e147 100644
> --- a/src/box/sql/tarantoolInt.h
> +++ b/src/box/sql/tarantoolInt.h
> @@ -13,8 +13,6 @@ struct fk_constraint_def;
> /* Misc */
> const char *tarantoolErrorMessage();
> 
> -int is_tarantool_error(int rc);
> -
> /* Storage interface. */
> const void *tarantoolsqlPayloadFetch(BtCursor * pCur, u32 * pAmt);
> 
> @@ -51,8 +49,7 @@ int tarantoolsqlDelete(BtCursor * pCur, u8 flags);
>  * @param key Key of record to be deleted.
>  * @param key_size Size of key.
>  *
> - * @retval SQL_OK on success, SQL_TARANTOOL_DELETE_FAIL
> - *         otherwise.
> + * @retval SQL_OK on success, SQL_TARANTOOL_ERROR otherwise.
>  */
> int
> sql_delete_by_key(struct space *space, uint32_t iid, char *key,
> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index 3bd8223..5222a4e 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -5469,7 +5469,7 @@ abort_due_to_error:
> 		/* Avoiding situation when Tarantool error is set,
> 		 * but error message isn't.
> 		 */
> -		if (is_tarantool_error(rc) && tarantoolErrorMessage()) {
> +		if (rc == SQL_TARANTOOL_ERROR && tarantoolErrorMessage()) {

Isn’t tarantoolErrorMessage redundant check? Is it possible
to use SQL_TARANTOOL_ERROR without set diag?