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

[Imeev Mergen <imeevma@tarantool.org>]

Hi! Thank you for review! My answers below.

On 5/15/19 4:18 PM, n.pettik wrote:
>
>> On 5 May 2019, at 15:17, imeevma@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.
Fixed.
>
>> 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
I think it's better to leave it as it is. A bit later there will
be a patch-set, in which all SQL error codes will be replaced with
either 0 or -1.

>
>> 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?
It is technically possible to install SQL_TARANTOOL_ERROR without
set diag. This code will be fixed in one of the following patches
of this patch-set.