Re: [Tarantool-patches] [tarantool-patches] [PATCH v4 07/20] refactoring: remove exceptions from index_def_new_from_tuple

["Ilya Kosarev" <i.kosarev@tarantool.org>]

[-- Attachment #1: Type: text/plain, Size: 2763 bytes --]


Hi!

Thanks for your review.
We are using diag_raise here as far as index_def_check_sequence is already refactored and we need to process the error it returns, while sequence_field_from_tuple itself is not being refactored in this patch, so we can't return an error here, only raise an exception.

Sincerely,
Ilya Kosarev

>Среда, 30 октября 2019, 11:59 +03:00 от Sergey Ostanevich <sergos@tarantool.org>:
>
>Hi!
>
>LGTM, just a few nits in comments below
>
>> index 8d565e189..10a5f8d64 100644
>> --- a/src/box/alter.cc
>> +++ b/src/box/alter.cc
>> @@ -118,10 +118,10 @@ access_check_ddl(const char *name, uint32_t object_id, uint32_t owner_uid,
>>  }
>> 
>>  /**
>> - * Throw an exception if the given index definition
>> + * Return an error if the given index definition
>>   * is incompatible with a sequence.
>>   */
>> -static void
>> +static int
>>  index_def_check_sequence(struct index_def *index_def, uint32_t sequence_fieldno,
>>  			 const char *sequence_path, uint32_t sequence_path_len,
>>  			 const char *space_name)
>> @@ -142,24 +142,27 @@ index_def_check_sequence(struct index_def *index_def, uint32_t sequence_fieldno,
>>  		}
>>  	}
>>  	if (sequence_part == NULL) {
>> -		tnt_raise(ClientError, ER_MODIFY_INDEX, index_def->name,
>> -			  space_name, "sequence field must be a part of "
>> -			  "the index");
>> +		diag_set(ClientError, ER_MODIFY_INDEX, index_def->name,
>> +			 space_name, "sequence field must be a part of "
>> +				     "the index");
>> +		return -1;
>>  	}
>>  	enum field_type type = sequence_part->type;
>>  	if (type != FIELD_TYPE_UNSIGNED && type != FIELD_TYPE_INTEGER) {
>> -		tnt_raise(ClientError, ER_MODIFY_INDEX, index_def->name,
>> -			  space_name, "sequence cannot be used with "
>> -			  "a non-integer key");
>> +		diag_set(ClientError, ER_MODIFY_INDEX, index_def->name,
>> +			 space_name, "sequence cannot be used with "
>> +				     "a non-integer key");
>> +		return -1;
>>  	}
>> +	return 0;
>>  }
>> 
>>  /**
>>   * Support function for index_def_new_from_tuple(..)
>> - * Checks tuple (of _index space) and throws a nice error if it is invalid
>> + * Checks tuple (of _index space) and sets a nice diag if it is invalid
>Should be 'Return an error', same as for the case above.
>
>> @@ -4066,8 +4099,9 @@ sequence_field_from_tuple(struct space *space, struct tuple *tuple,
>>  		if (path_len == 0)
>>  			path_raw = NULL;
>>  	}
>> -	index_def_check_sequence(pk->def, fieldno, path_raw, path_len,
>> -				 space_name(space));
>> +	if (index_def_check_sequence(pk->def, fieldno, path_raw, path_len,
>> +				 space_name(space)) != 0)
>> +		diag_raise();
>
>I wonder why we leave some diag_raise() and not diag_set() plus return
>-1 or NULL?
>
>
>regards,
>Sergos
>


-- 
Ilya Kosarev

[-- Attachment #2: Type: text/html, Size: 4130 bytes --]